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

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

[Wolfgang Link]

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

Signed-off-by: Wolfgang Link <wolfgang@linksystems.org>
---
 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);
+
+    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;
+    }
+
+    return (int64_t) ret ? 0 : 1;
+}
+
 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
+#
+# Get the current link state of the nics or nic.
+#
+# @name: name of the nic you get the state of
+#
+# Return: If link is up 1
+#         If link is down 0
+#         If an error occure an empty string.
+#
+# Notes: this is an Proxmox VE extension and not offical part of Qemu.
+##
+{ '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,
-- 
1.7.10.4

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

[Markus Armbruster]

Copying Eric for additional QAPI schema expertise.

Wolfgang Link <wolfgang@linksystems.org> 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 <wolfgang@linksystems.org>
> ---
>  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,

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4559 bytes --]

On 12/17/2014 02:02 AM, Markus Armbruster wrote:
> Copying Eric for additional QAPI schema expertise.

Thanks (not sure why I didn't see the original, as I usually notice
emails that touch .json files)

> 
> Wolfgang Link <wolfgang@linksystems.org> 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.

s/impotent/important/ (very different meanings!)
s/get an timeout/had a 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.

Adding it as a QOM property would make it accessible but tedious to get
to; even better would be adding it to a general query command that can
tell all sorts of things about the network device.  That is, we probably
want some sort of 'query-netdev' that can return link status and more.


>> +
>> +    return (int64_t) ret ? 0 : 1;
> 
> Superfluous cast of ret.

Even shorter as:
return !ret;

but why return an integer when a bool will do?


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

Also, new commands should be named with dash '-', not underscore '_'.

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

s/occure/occurs,/
but as Markus correctly pointed out, that is not how an error is returned.

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

Yes, we should (if we don't already have it under some other name; a
quick search for query-net didn't find it).

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

Yes.

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

Alas, I think this is one place where QMP hasn't caught up, and HMP is
still open-coding things.

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

s/offical/official/

> 
> Really?
> 
>> +##
>> +{ 'command': 'get_link_status', 'data': {'name': 'str'}, 'returns': 'int'}

Markus is correct; 'returns':'int' is not extensible, and you should
instead be returning a struct (or better yet, like other query commands,
return an array of structs, where each array member describes a network
device, so that one command learns the state of all devices at once).

>> +SQMP
>> +get_link_status
>> +--------

Make the --- pad out to the length of the command name.

>> +
>> +Get the link status of a network adapter.
>> +
>> +Arguments:
>> +
>> +- "name": network device name (json-string)
>> +
>> +Example:
>> +
>> +-> { "execute": "set_link", "arguments": { "name": "e1000.0" } }

Example doesn't match the command it is describing (did you mean
"get_link_status" for the command name?)

>> +<- { "return": {1} }

Invalid JSON.  As spec'd by your patch, this would be { "return": 1 };
but ideally you want a struct, as in { "return": { "link": true } }, or
even an array of structs, as in { "return": [ { "device": "net0",
"link": true }, { "device": "net1", "link": false } ] }

>> +EQMP
>> +
>> +    {
>>          .name       = "set_link",
>>          .args_type  = "name:s,up:b",
>>          .mhandler.cmd_new = qmp_marshal_input_set_link,
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Wolfgang]

[-- Attachment #1: Type: text/plain, Size: 3753 bytes --]

Tanks for your tips.

2014-12-17 10:02 GMT+01:00 Markus Armbruster <armbru@redhat.com>:

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

[-- Attachment #2: Type: text/html, Size: 6355 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

On Thu, Dec 11, 2014 at 02:36:14PM +0100, Wolfgang Link wrote:
> this qmp command returns the current link state from the given nic
> this is impotent if the set_link failed or get an timeout.

s/impotent/important/

I don't understand the rationale for this patch:

set_link does not fail or time out asynchronously, and the caller
immediately knows whether set_link succeeded or failed.  So I think this
command is not strictly necessary as implied by the commit description,
but it is useful if the QMP client wishes to query the current 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

Please add a query-link-status command.  All QMP commands that fetch
data are called query-*.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]