Re: [Qemu-devel] [PATCH] qmp: implement a qmp command get_link_status

[Markus Armbruster]

Copying Eric for additional QAPI schema expertise.

Wolfgang Link <address@hidden> writes:

> this qmp command returns the current link state from the given nic
> this is impotent if the set_link failed or get an timeout.

Please start your sentences with a capital letter, and end them with a
period :)

If "link status" was a QOM property, we could fetch it with existing
command qom-get.  It isn't, as far as I can tell.

>
> Signed-off-by: Wolfgang Link <address@hidden>
> ---
>  net/net.c        |   26 ++++++++++++++++++++++++++
>  qapi-schema.json |   15 +++++++++++++++
>  qmp-commands.hx  |   22 ++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index 7acc162..57c6f1e 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1141,6 +1141,32 @@ void do_info_network(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +int64_t qmp_get_link_status(const char *name, Error **errp)
> +{
> +    NetClientState *ncs[MAX_QUEUE_NUM];
> +    NetClientState *nc;
> +    int queues;
> +    bool ret;
> +
> +    queues = qemu_find_net_clients_except(name, ncs,
> +                                          NET_CLIENT_OPTIONS_KIND_MAX,
> +                                          MAX_QUEUE_NUM);

This finds the NetClientState named @name.  The third parameter lets you
exclude a certain kind, but your NET_CLIENT_OPTIONS_KIND_MAX argument
excludes none.  Keep this in mind for further down.

> +
> +    if (queues == 0) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, name);
> +        return (int64_t) -1;
> +    }
> +
> +    nc = ncs[0];
> +    ret = ncs[0]->link_down;
> +
> +    if (nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
> +        ret = ncs[0]->peer->link_down;
> +    }

Can you explain why examining only the first queue suffices?

> +
> +    return (int64_t) ret ? 0 : 1;

Superfluous cast of ret.

> +}
> +
>  void qmp_set_link(const char *name, bool up, Error **errp)
>  {
>      NetClientState *ncs[MAX_QUEUE_NUM];
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9ffdcf8..c5321e6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1200,6 +1200,21 @@
>  { 'command': 'inject-nmi' }
>  
>  ##
> +# @get_link_status

We have two informational commands named like "get":
trace-event-set-state and qom-get.  We have many named query-FOO.  But
they usually don't take an argument.  Hmm.

> +#
> +# Get the current link state of the nics or nic.
> +#
> +# @name: name of the nic you get the state of

"nics or nic" (one or more) or "the nic" (just one)?

Doesn't your code above support any network client, not just NICs?

> +#
> +# Return: If link is up 1
> +#         If link is down 0
> +#         If an error occure an empty string.

A QMP command makes the server send either a success response, of the
type declared in the schema (here: 'returns': 'int'), or an error
response.

If we have nothing special to say about the error response in the
schema's command documentation, we say nothing at all.

Bool is a more natural type then int for the answer to "is the link up?"

We usually make return value objects to facilitate future extensions.
Perhaps not an issue for a command that answers the "is the link up?"
question.  Begs the question whether we should we have a more general
command to query NIC properties.

Should link status be visible in HMP's "info network"?

Stefan, what's the QMP equivalent to "info network"?

> +#
> +# Notes: this is an Proxmox VE extension and not offical part of Qemu.

Really?

> +##
> +{ 'command': 'get_link_status', 'data': {'name': 'str'}, 'returns': 'int'}
> +
> +##
>  # @set_link:
>  #
>  # Sets the link status of a virtual network adapter.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 718dd92..ecc501a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1431,6 +1431,28 @@ Example:
>  EQMP
>  
>      {
> +     .name       = "get_link_status",
> +     .args_type  = "name:s",
> +             .mhandler.cmd_new = qmp_marshal_input_get_link_status,
> +    },
> +
> +SQMP
> +get_link_status
> +--------
> +
> +Get the link status of a network adapter.
> +
> +Arguments:
> +
> +- "name": network device name (json-string)
> +
> +Example:
> +
> +-> { "execute": "set_link", "arguments": { "name": "e1000.0" } }
> +<- { "return": {1} }
> +EQMP
> +
> +    {
>          .name       = "set_link",
>          .args_type  = "name:s,up:b",
>          .mhandler.cmd_new = qmp_marshal_input_set_link,