[libvirt] [PATCH v3 04/18] blockjob: split out block info monitor handling

[Peter Krempa]

On 09/04/14 21:30, Eric Blake wrote:
> On 09/04/2014 09:39 AM, Peter Krempa wrote:
>> On 08/31/14 06:02, Eric Blake wrote:
>>> Another layer of overly-multiplexed code that deserves to be
>>> split into obviously separate paths for query vs. modify.
>>> This continues the cleanup started in the previous patch.
>>>
>>> In the process, make some tweaks to simplify the logic when
>>> parsing the JSON reply.  There should be no user-visible
>>> semantic changes.
>>>
> 
>>> +int qemuMonitorBlockJobInfo(qemuMonitorPtr mon,
>>> +                            const char *device,
>>> +                            virDomainBlockJobInfoPtr info)
>>> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>>
>> Implementation of this function doesn't check for presence of the @info
>> structure and dereferences it always. You should mark argument 3 nonnull
>> too.
> 
> Good catch.
> 
> 
>>> -    for (i = 0; i < nr_results; i++) {
>>> +    for (i = 0, ret = 0; i < nr_results && ret == 0; i++) {
>>>          virJSONValuePtr entry = virJSONValueArrayGet(data, i);
>>>          if (!entry) {
>>>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>>                             _("missing array element"));
>>> -            return -1;
>>> +            ret = -1;
>>> +            goto cleanup;
>>>          }
>>> -        if (qemuMonitorJSONGetBlockJobInfoOne(entry, device, info) == 0)
>>> -            return 1;
>>> +        ret = qemuMonitorJSONGetBlockJobInfoOne(entry, device, info);
>>
>> This doesn't break the loop, thus if the device info isn't last in the
>> structure you'd overwrite the return code. I presume the clients don't
>> check that far but you've documented that semantics.
> 
> Actually, the loop DOES break, just on the next iteration.  I added '&&
> ret == 0' to the loop continuation condition.  So the semantics are:
> 
> InfoOne fails and returns -1, the next iteration is aborted and overall
> ret is -1
> 
> InfoOne returns 0 because the requested device didn't match, so the loop
> continues to look at the next array member. If all array members are
> exhausted, ret remains 0 and overall return is 0 for job not found.
> 
> InfoOne succeeds and returns 1, the next iteration is aborted and the
> overall ret is 1 for info populated.

I actually figured out that you probably changed the loop condition when
I walked out of the office :D Unfortunately I wasn't around a computer
to verify and correct myself.

ACK to the original patch if you add ATTRIBUTE_NONNUL to the correct places.

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140905/ce3e52dc/attachment-0001.sig>