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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list