Re: [libvirt] [REPOST PATCH v2 6/9] qemu: Add support for parsing iotune group setting
On 11/29/2016 03:50 AM, Erik Skultety wrote:
On Mon, Nov 28, 2016 at 05:14:54PM -0500, John Ferlan wrote:
>
> [...]
>
>>> @@ -4535,6 +4535,11 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr
result,
>>> GET_THROTTLE_STATS_OPTIONAL("iops_rd_max",
read_iops_sec_max);
>>> GET_THROTTLE_STATS_OPTIONAL("iops_wr_max",
write_iops_sec_max);
>>> GET_THROTTLE_STATS_OPTIONAL("iops_size", size_iops_sec);
>>> +
>>> + if ((group_name = virJSONValueObjectGetString(inserted,
"group")) &&
>>> + VIR_STRDUP(reply->group_name, group_name) < 0)
>>> + goto cleanup;
>>> +
>>
>> One more thing, you can make the GetString call directly from VIR_STRDUP and
>> then get rid of the group_name variable.
>>
>> Erik
>>
>
> Ewww.... too overloaded for my taste... I'm sure behind the scenes the
> compiler will optimize anyway.
>
> FWIW: Doing so results in compiler errors:
>
> if (VIR_STRDUP(reply->group_name,
> virJSONValueObjectGetString(inserted, "group") < 0) :
>
> qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo':
> qemu/qemu_monitor_json.c:4539:67: error: ordered comparison of pointer
> with integer zero [-Werror=extra]
> virJSONValueObjectGetString(inserted, "group") < 0)
> ^
> ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP'
> # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \
> ^~~
> qemu/qemu_monitor_json.c:4539:20: error: passing argument 2 of
> 'virStrdup' makes pointer from integer without a cast
> [-Werror=int-conversion]
>
>
> OR:
>
> if (VIR_STRDUP(reply->group_name,
> (virJSONValueObjectGetString(inserted, "group")) < 0)
>
> In file included from qemu/qemu_monitor_json.c:46:0:
> qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo':
> qemu/qemu_monitor_json.c:4539:73: error: ordered comparison of pointer
> with integer zero [-Werror=extra]
> (virJSONValueObjectGetString(inserted, "group"))
> < 0)
> ^
> ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP'
> # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \
> ^~~
>
You've got the parentheses wrong...in both cases...
ah, right - my eyes just weren't seeing it, I changed to the following.
if (VIR_STRDUP(reply->group_name,
virJSONValueObjectGetString(inserted, "group")) < 0)
goto cleanup;
Also, once you do what I'm suggesting, the crash from 6/9 will be resolved,
however I still would like to see the @reply structure in
qemuDomainGetBlockIoTune initialized properly on the stack.
Weird how that crash happens for you, but not me especially since you
show 20 nparams which would seem to imply you're getting something back
from qemu. Even though it's not set by libvirt, qemu does default to
something (the id/alias for the object - in my case "drive-virtio-disk0"
for my qemu 2.6.2). What do you get from a command:
virsh qemu-monitor-command $domain
'{"execute":"query-block"}'
?
In any case, I'll add a memset(&reply, 0, sizeof(reply)) in the qemu
path from the caller, which I believe should suffice the review comment.
The other path is covered with the copy from persistent XML.
Still since we're in freeze, I'll wait until after the release in order
to push. That'll mean an adjustment to the version in formatdomain from
2.5.0 to 3.0.0 since the "next" release won't be until January
(http://libvirt.org/downloads.html#schedule)
Thanks for the review -
John