On 10/03/2016 10:49 AM, Peter Krempa wrote:
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.
I find it ironic that it's more desirable to add code to virjson.c (1 external and 2 static), virjson.h, and libvirt.private_syms for the single purpose that it's more desirable to have 3 API's follow the same steps and it's less desirable to combine those API's in some manner. We have a few API's that will pass/receive many parameters including multiple NULL's and make the proper decision based on some key for the function. We have a difference of opinion on the way to do this - that's fine. I'm moving on. No sense in beating this dead horse anymore. John