On Mon, Oct 03, 2016 at 10:46:07 -0400, John Ferlan wrote:
[...]
Since you didn't explicitly state which checks, I'm assuming you mean either:
if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info reply was missing device list")); goto cleanup; }
I did consider moving this into the helper and returning devices but ran into the problem that the helper would virJSONValueFree(reply) while returning devices which is then used by the for loop to get each device (dev = virJSONValueArrayGet(devices, i)).
What's the problem wit that? Creating a function that steals the value from a JSON object should be pretty simple. We have one that steals array members: virJSONValueArraySteal.
Since devices points into reply, I didn't think that would be such a good idea to free reply and then return devices. Returning reply anda
Why not?
Let's assume the helper is:
+static virJSONValuePtr +qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) +{ + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr devices = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-block", NULL))) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0 || + qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-block reply was missing device list")); + goto cleanup; + } + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return devices; +}
Wouldn't "reply" being free'd cause access problems for devices? Is there other code somewhere else that gets the array, free's the object, then parses the array? I didn't find any (for good reason)...
It would as virJSONValueFree is recursive and thus everything would be freed. Hence my comment of adding the "steal" function.