Re: [libvirt] [PATCH v2 0/5] Combine various query-block json call paths

[...] >> 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)...