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

[John Ferlan]

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