From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53599) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5GPs-0002AH-Ko for qemu-devel@nongnu.org; Wed, 17 Jun 2015 12:42:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5GPm-0002uX-JS for qemu-devel@nongnu.org; Wed, 17 Jun 2015 12:42:20 -0400 Received: from mail-qk0-x231.google.com ([2607:f8b0:400d:c09::231]:36590) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5GPm-0002uP-ET for qemu-devel@nongnu.org; Wed, 17 Jun 2015 12:42:14 -0400 Received: by qkfe185 with SMTP id e185so29281641qkf.3 for ; Wed, 17 Jun 2015 09:42:13 -0700 (PDT) From: Don Slutz Message-ID: <5581A362.4060909@Gmail.com> Date: Wed, 17 Jun 2015 12:42:10 -0400 MIME-Version: 1.0 References: <1434117956-4929-1-git-send-email-dslutz@verizon.com> <1434117956-4929-7-git-send-email-dslutz@verizon.com> <557EF500.3090409@redhat.com> In-Reply-To: <557EF500.3090409@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v7 6/9] vmport_rpc: Add QMP access to vmport_rpc object. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Don Slutz , qemu-devel@nongnu.org Cc: "Michael S. Tsirkin" , Markus Armbruster , Luiz Capitulino , Anthony Liguori , Paolo Bonzini , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= , Richard Henderson On 06/15/15 11:53, Eric Blake wrote: > On 06/12/2015 08:05 AM, 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 >> >> More details in qmp-commands.hx >> >> Signed-off-by: Don Slutz >> CC: Don Slutz >> --- > >> +typedef int FindVMPortRpcDeviceFunc(VMPortRpcState *s, const void *arg); >> + >> +/* >> + * The input and output argument that find_VMPortRpc_device() uses >> + * to do it's work. > > s/it's/its/ (the only time "it's" is correct is if "it is" would also be > correct) > Will fix. > >> >> +static int get_qmp_guestinfo(VMPortRpcState *s, >> + unsigned int a_key_len, const char *a_info_key, >> + unsigned int *a_value_len, void ** a_value_data) > > No space after **. > Will fix. > >> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp) >> +{ >> + int rc; >> + >> + switch (action) { >> + case VMPORT_ACTION_REBOOT: >> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send, >> + "OS_Reboot"); >> + break; >> + case VMPORT_ACTION_HALT: >> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send, >> + "OS_Halt"); >> + break; >> + case VMPORT_ACTION_MAX: >> + assert(action != VMPORT_ACTION_MAX); >> + abort(); /* Should be impossible to get here. */ >> + break; > > Don't know if any static checkers will complain about the break being > dead code. > > I do not know. However I can just delete it, and so will do so. >> + >> +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.")) == 0) { >> + key_value.key_data = (void *)(key + strlen("guestinfo.")); > > Cast here... > >> + key_value.key_len = strlen(key) - strlen("guestinfo."); >> + } else { >> + key_value.key_data = (void *)key; > > ...and here... > >> + key_value.key_len = strlen(key); >> + } >> + if (has_format && (format == DATA_FORMAT_BASE64)) { >> + gsize write_count; >> + >> + key_value.value_data = g_base64_decode(value, &write_count); >> + key_value.value_len = write_count; >> + } else { >> + key_value.value_data = (void *)value; > > ...and here is only needed to get rid of const. Would it help if struct > keyValue declared 'const char *key_data' instead of 'void *key_data'? This is value_data not key_data. So this cast is needed with 'const char *key_data', the other 2 do go away. > >> + key_value.value_len = strlen(value); >> + } >> + >> + rc = foreach_dynamic_vmport_rpc_device(find_set, (void *)&key_value); > > This cast is spurious (it does not get rid of const, and any non-const > pointer in C can already be passed to void* without a cast). > Yup fixed. >> +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key, >> + bool has_format, >> + enum DataFormat format, >> + Error **errp) >> +{ >> + int rc; >> + keyValue key_value; >> + VmportGuestInfoValue *ret_value; >> + >> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) { >> + key_value.key_data = (void *)(key + strlen("guestinfo.")); >> + key_value.key_len = strlen(key) - strlen("guestinfo."); >> + } else { >> + key_value.key_data = (void *)key; > > More casts that might be prevented with correct const in the callback > struct. > these 2 do get fixed. >> + key_value.key_len = strlen(key); >> + } >> + >> + rc = foreach_dynamic_vmport_rpc_device(find_get, (void *)&key_value); > > Another spurious cast. > Fixed. >> + if (rc) { >> + convert_local_rc(errp, rc); >> + return NULL; >> + } >> + >> + ret_value = g_malloc(sizeof(VmportGuestInfoKey)); > > Do you want g_malloc0, or even the additional type-safety (and less > typing) of g_new0(VmportGuestInfoKey, 1)? Otherwise, if you later add > fields, but forget to initialize them, you'll have random garbage. > Switched to g_new0. >> + if (has_format && (format == DATA_FORMAT_BASE64)) { >> + ret_value->value = g_base64_encode(key_value.value_data, >> + key_value.value_len); >> + } else { >> + /* >> + * FIXME should check for complete, valid UTF-8 characters. >> + * Invalid sequences should be replaced by a suitable >> + * replacement character. > > Or cause an error to be raised. Added that to the comment. > > Looking better, though. > -Don Slutz