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 <wolfg...@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
signature.asc
Description: OpenPGP digital signature
