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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]