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