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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list