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

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.