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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org