From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45140) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1BLT-0003a6-Cw for qemu-devel@nongnu.org; Wed, 17 Dec 2014 04:56:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y1BLP-00018V-1m for qemu-devel@nongnu.org; Wed, 17 Dec 2014 04:56:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:32905) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1BLO-00018N-QD for qemu-devel@nongnu.org; Wed, 17 Dec 2014 04:56:34 -0500 Message-ID: <5491534C.3010606@redhat.com> Date: Wed, 17 Dec 2014 02:56:28 -0700 From: Eric Blake MIME-Version: 1.0 References: <1418304974-15117-1-git-send-email-wolfgang@linksystems.org> <87k31q3ayk.fsf@blackfin.pond.sub.org> In-Reply-To: <87k31q3ayk.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="FqVr5LfqleALxXuCqoPH7NIWg3gSC3C6w" Subject: Re: [Qemu-devel] [PATCH] qmp: implement a qmp command get_link_status List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Wolfgang Link Cc: stefanha@redhat.com, qemu-devel@nongnu.org, aliguori@amazon.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --FqVr5LfqleALxXuCqoPH7NIWg3gSC3C6w Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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) >=20 > Wolfgang Link writes: >=20 >> 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/ >=20 > Please start your sentences with a capital letter, and end them with a > period :) >=20 > 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; >=20 > 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' } >> =20 >> ## >> +# @get_link_status >=20 > 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 '_'. >=20 >> +# >> +# Get the current link state of the nics or nic. >> +# >> +# @name: name of the nic you get the state of >=20 > "nics or nic" (one or more) or "the nic" (just one)? >=20 > Doesn't your code above support any network client, not just NICs? >=20 >> +# >> +# 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= =2E >=20 > 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. >=20 > If we have nothing special to say about the error response in the > schema's command documentation, we say nothing at all. >=20 > Bool is a more natural type then int for the answer to "is the link up?= " >=20 > 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). >=20 > Should link status be visible in HMP's "info network"? Yes. >=20 > 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. >=20 >> +# >> +# Notes: this is an Proxmox VE extension and not offical part of Qemu= =2E s/offical/official/ >=20 > Really? >=20 >> +## >> +{ '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 =3D "set_link", >> .args_type =3D "name:s,up:b", >> .mhandler.cmd_new =3D qmp_marshal_input_set_link, >=20 >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --FqVr5LfqleALxXuCqoPH7NIWg3gSC3C6w Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUkVNMAAoJEKeha0olJ0NqCVMH/3tjGVukLO+d0Uh3463MIEXs lcFAdmAQARC9D2Ucj0eP6fT48FXe5WWw4Us4h9gU3jK/vFsEzpFmiTlUGEs6FL+C ie4spAi5S6xABvEq3CFWlNTQ1nr/WU5XZhkBhCYZ1kUZVELtDSly0xowLsy27RM1 7wclpnPugf+YbPA9p8mYUs2XCOxFiQjHJRSnavoEMhgxJuA+lZAgOU3qtwSFrD3g odu79Hjmm8+jkoXkrYn3W2iWSX+B86lBYXDNw2VnpCzNgM+SJ1gkQUmuQezbnHBm /Nb7PMVPSMzqixzFUB0eCPvWYBKAvbGzQ8RAc92MmTacWRvKuGEp4hXRZx0Kzoc= =QW8s -----END PGP SIGNATURE----- --FqVr5LfqleALxXuCqoPH7NIWg3gSC3C6w--