From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35152) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsINq-0000JA-LG for qemu-devel@nongnu.org; Tue, 12 May 2015 18:10:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsINn-0001Va-D1 for qemu-devel@nongnu.org; Tue, 12 May 2015 18:10:38 -0400 Received: from omzsmtpe04.verizonbusiness.com ([199.249.25.207]:10676) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsINn-0001VW-0n for qemu-devel@nongnu.org; Tue, 12 May 2015 18:10:35 -0400 From: Don Slutz Message-ID: <55527A43.10306@one.verizon.com> Date: Tue, 12 May 2015 18:10:11 -0400 MIME-Version: 1.0 References: <1431457421-31877-1-git-send-email-dslutz@verizon.com> <1431457421-31877-5-git-send-email-dslutz@verizon.com> <5552598A.30701@redhat.com> In-Reply-To: <5552598A.30701@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="XOkiWI0MRoJPRwpXqbOGwGQfdf3i5MoAN" Subject: Re: [Qemu-devel] [PATCH v6 4/7] vmport_rpc: Add QMP access to vmport_rpc object. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , "qemu-devel@nongnu.org" Cc: "Michael S. Tsirkin" , Markus Armbruster , Luiz Capitulino , Anthony Liguori , Paolo Bonzini , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= , Richard Henderson This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --XOkiWI0MRoJPRwpXqbOGwGQfdf3i5MoAN Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 05/12/15 15:50, Eric Blake wrote: > On 05/12/2015 01:03 PM, Don Slutz wrote: >> This adds one new inject command: >> >> inject-vmport-action >> >> And three guest info commands: >> >> vmport-guestinfo-set >> vmport-guestinfo-get >> query-vmport-guestinfo >>vmport-guestinfo-get key=3Dfoo >> More details in qmp-commands.hx >> >> Signed-off-by: Don Slutz >> --- >=20 >> +/* >> + * Run func() for every VMPortRpc device, traverse the tree for >> + * everything else. Note: This routine expects that opaque is a >> + * VMPortRpcFind pointer and not NULL. >> + */ >> +static int find_VMPortRpc_device(Object *obj, void *opaque) >> +{ >> + VMPortRpcFind *find =3D opaque; >> + Object *dev; >> + VMPortRpcState *s; >> + >> + if (find->found) { >=20 > Why not assert(find) instead of leaving it to the comment? My memory says that someone complained about too many asserts used, so I just commented it. I have no issue with adding an assert() and dropping the comment, and so plan on doing this. >=20 >> +/* >> + * Loop through all dynamically created VMPortRpc devices and call >> + * func() for each instance. >> + */ >> +static int foreach_dynamic_vmport_rpc_device(FindVMPortRpcDeviceFunc = *func, >> + void *arg) >> +{ >> + VMPortRpcFind find =3D { >> + .func =3D func, >> + .arg =3D arg, >=20 > Is it worth marking arg const here and in the VMPortRpcFind struct... See below >=20 >=20 >> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp)= >> +{ >> + int rc; >> + >> + switch (action) { >> + case VMPORT_ACTION_REBOOT: >> + rc =3D foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send= , >> + (void *)"OS_Reboot"); >=20 > ...so that you don't have to cast away const here? >=20 Not sure. I still need 2 casts: -static int vmport_rpc_find_list(VMPortRpcState *s, void *arg) +static int vmport_rpc_find_list(VMPortRpcState *s, const void *arg) { - g_hash_table_foreach(s->guestinfo, vmport_rpc_find_list_one, arg); + g_hash_table_foreach(s->guestinfo, vmport_rpc_find_list_one, (gpointer)arg); return 0; } and -static int find_get(VMPortRpcState *s, void *arg) +static int find_get(VMPortRpcState *s, const void *arg) { - keyValue *key_value =3D arg; + keyValue *key_value =3D (void *)arg; get_qmp_guestinfo() does change parts of key_value. So are these casts better or worse? >> + break; >> + case VMPORT_ACTION_HALT: >> + rc =3D foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send= , >> + (void *)"OS_Halt"); >> + break; >> + case VMPORT_ACTION_MAX: >> + assert(action !=3D VMPORT_ACTION_MAX); >> + rc =3D 0; /* Should be impossible to get here. */ >=20 > I'd rather abort() if someone compiled with -NDEBUG. Ok, will change to abort. >=20 >> + break; >> + } >> + convert_local_rc(errp, rc); >> +} >> + >> +typedef struct keyValue { >> + void *key_data; >> + void *value_data; >> + unsigned int key_len; >> + unsigned int value_len; >=20 > Should these be size_t? There is no need to. There is a limit on the max values that can be used here (because without the max check it is possible for the guest to request a lot of memory outside of the memory provided to the guest as ram. Currently using unsigned char for key_len and unsigned short for value_len will work, but I went with unsigned int to avoid strange issues if someone in the future changes the allowed maxes. However this is not important to me and so if you want them to be size_t, I am happy to change them. >=20 >> +void qmp_vmport_guestinfo_set(const char *key, const char *value, >> + bool has_format, enum DataFormat format= , >> + Error **errp) >> +{ >> + int rc; >> + keyValue key_value; >> + >> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) =3D=3D 0) { >> + key_value.key_data =3D (void *)(key + strlen("guestinfo.")); >> + key_value.key_len =3D strlen(key) - strlen("guestinfo."); >> + } else { >> + key_value.key_data =3D (void *)key; >=20 > Casting to (void*) looks awkward; should key_data should be typed 'cons= t > void *' to avoid the need for a cast? For that matter, why is it void*= , > why not 'const char *'? Not sure. Best guess is that I was focused on the value being binary data, and just did the same for the key. The change to "const char *key_data;" works, and so I will do this. >=20 >=20 >> +++ b/qmp-commands.hx >> @@ -490,6 +490,126 @@ Note: inject-nmi fails when the guest doesn't su= pport injecting. >> EQMP >=20 >> +SQMP >> +vmport-guestinfo-set >> +---------- >=20 > Still mismatches on ---- line length (several sites). Sigh, will fix. >=20 >> +-> { "execute": "vmport-guestinfo-get", >> + "arguments": { "key": "guestinfo.foo", >> + "format": "utf8" } } >> +<- {"return": {"value": "abcdefgh"}} >> + >> + >> +EQMP >> + >> + { >> + .name =3D "query-vmport-guestinfo", >> + .args_type =3D "", >> + .mhandler.cmd_new =3D qmp_marshal_input_query_vmport_guestinf= o, >> + }, >> + >> +SQMP >> +query-vmport-guestinfo >> +---------- >> + >> +Returns information about VMWare Tools guestinfo. The returned value= is a json-array >> +of all keys. >> + >> +Example: >> + >> +-> { "execute": "query-vmport-guestinfo" } >> +<- { >> + "return": [ >> + { >> + "key": "guestinfo.ip" >> + }, >=20 > So if I'm following this correctly, a user has to call > query-vmport-guestinfo to learn the key names, and then once per key > call vmport-guetsinfo-get to learn the key values. Why not just return= > key names and values all at once, to save the multiple call overhead? >=20 You are following this correctly. The issue is with "*format" (i.e. DataFormat). So the format arg would need to be added, and while I expect most uses will all be base64, I do not see this as hard requiremen= t. So if the user of QMP what to use base64 for some keys and utf8 for others, there is no way to express this in 1 arg. This is the way I came up with to handle this case. -Don Slutz --XOkiWI0MRoJPRwpXqbOGwGQfdf3i5MoAN Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (GNU/Linux) iQIcBAEBAgAGBQJVUnpUAAoJEHkofi8zcwBkEpYP/3EtLApTFKjF+pHf4lsYbUjk 3VrILoU1SxWwni2ZR/TYfWyAwDOwPVrVpeZ+QU8uXw4lAXQC90scnh3kE8oeKa0Q uAcawOPDw7LVi+ToeTzWyQgxQO959fRDfWE/M48hin4l6joPbBW/QBZnObCshBlT Q2yo2EXrov1CajS06fLi3unUzyNSKkikCYAgEs917sh9YGZuePi3SaQJaVm5u+vy XNkiPkKABsS1Qrj3FzMPY/EItP1lt2aWUcQ/8YY9JQi1n8pn2sPg2CKLtis9qDO8 rWOrSh3ND1hmZyqRBaMhfrjMqmCQPrPLcKAuM5YPtZUILGnPlZf+4h2jxvImKr0V qNEyaiY/24oIOwkGXDpQ2SGNs1sKIWDep5qGMRpW9sFAkdZchfbMsNRxYImJDcF/ E57eL7OL49qMSusJGhmGK1LDiy1fGhmAWs/53cpK0CPm1AjUHtIir958GvZdHGSF XVsEMq3W3cVxORmMVAlp45uuOVQ1EK1fF+PBYEKKa5BjGRKEPRw0UCN41x9wZ3UK JvnJUL/B8zFxAvqyTWGhRV/9R/WJO7/qas8KNjRYgiIogXAyt6LnhcRXNYHfRGRW Iz70F+B+HBEo08XTQXJ/h27hr4b3WL/AJGDu7wklTSbeiEBrPy9GhUuyo0atMQ5S JBCPO4uSxN8FGxtHJmvO =7z4P -----END PGP SIGNATURE----- --XOkiWI0MRoJPRwpXqbOGwGQfdf3i5MoAN--