qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Wolfgang
Subject: Re: [Qemu-devel] [PATCH] qmp: implement a qmp command get_link_status
Date: Wed, 17 Dec 2014 11:18:07 +0100

Tanks for your tips.

2014-12-17 10:02 GMT+01:00 Markus Armbruster <address@hidden>:
> +    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?


Normally there should be only one nic with the name of @name.
I could check the number of queues tho verify this.

> +
> +    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.

I thought get is more precise the only link_status;
 

> +#
> +# 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)?


It should only nic (just one).
First concept was to get both and the I forgot to change it.
 
Doesn't your code above support any network client, not just NICs?

I didn't test it with network clients!
 

> +#
> +# 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?"

Yes.
 

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"?

I thought in fact of completeness.
 

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

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

Really?

I pushed it first to this Open Source Project and forgot to erase it.
 

> +##
> +{ '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,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]