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

[Peter Krempa]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161003/5b86a1a8/attachment-0001.sig>