[libvirt] [REPOST PATCH v2 6/9] qemu: Add support for parsing iotune group setting

Tue Nov 29 14:01:50 UTC 2016

On Tue, Nov 29, 2016 at 07:28:51AM -0500, John Ferlan wrote:
> 
> 
> 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"}'
> 
> ?
> 

The returned JSON doesn't indicate any default on my qemu 2.6.0:

{"return":[{"io-status":"ok","device":"drive-virtio-disk0","locked":false,"removable":false,"inserted":{"iops_rd":0,"detect_zeroes":"off","image":{"virtual-size":21474836480,"filename":"/var/lib/libvirt/images/f23-a.qcow2","cluster-size":65536,"format":"qcow2","actual-size":8880848896,"format-specific":{"type":"qcow2","data":{"compat":"1.1","lazy-refcounts":false,"refcount-bits":16,"corrupt":false}},"dirty-flag":false},"iops_wr":0,"ro":false,"node-name":"#block190","backing_file_depth":0,"drv":"qcow2","iops":0,"bps_wr":0,"write_threshold":0,"encrypted":false,"bps":0,"bps_rd":0,"cache":{"no-flush":false,"direct":false,"writeback":true},"file":"/var/lib/libvirt/images/f23-a.qcow2","encryption_key_missing":false},"type":"unknown"},{"io-status":"ok","device":"drive-virtio-disk1","locked":false,"removable":false,"inserted":{"iops_rd":0,"detect_zeroes":"off","image":{"virtual-size":41943040000,"filename":"/var/lib/libvirt/images/dummy.img","format":"raw","actual-size":41943048192,"dirty-flag":false},"iops_wr":0,"ro":false,"node-name":"#block398","backing_file_depth":0,"drv":"raw","iops":0,"bps_wr":0,"write_threshold":0,"encrypted":false,"bps":0,"bps_rd":0,"cache":{"no-flush":false,"direct":false,"writeback":true},"file":"/var/lib/libvirt/images/dummy.img","encryption_key_missing":false},"type":"unknown"},{"io-status":"ok","device":"drive-virtio-disk2","locked":false,"removable":false,"inserted":{"iops_rd":0,"detect_zeroes":"off","image":{"virtual-size":52428800,"filename":"/var/lib/libvirt/images/vol-qcow2.img","cluster-size":65536,"format":"qcow2","actual-size":274432,"format-specific":{"type":"qcow2","data":{"compat":"1.1","lazy-refcounts":false,"refcount-bits":16,"corrupt":false}},"dirty-flag":false},"iops_wr":0,"ro":false,"node-name":"#block549","backing_file_depth":0,"drv":"qcow2","iops":0,"bps_wr":0,"write_threshold":0,"encrypted":false,"bps":0,"bps_rd":0,"cache":{"no-flush":false,"direct":false,"writeback":true},"file":"/var/lib/libvirt/images/vol-qcow2.img","encryption_key_missing":false},"type":"unknown"}],"id":"libvirt-9"}

What I did was just to start a domain that had no block iotuning turned on
whatsoever and when I saw the code I was like, wait, I need to try that as it
looked suspicious at first glance (since at that moment I'd already known about
the missing initialization) so I tried it and it crashed.

> In any case, I'll add a memset(&reply, 0, sizeof(reply)) in the qemu

Yeah, that would do, sure, but is there a problem with a plain static
initialization at the function entry point? I mean that's where we (most of the
time) put all of our initializations and also where everyone would actually
expect such an assignment to take place...

Erik

> 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
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161129/4ba5a8e1/attachment-0001.sig>