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

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.