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

[John Ferlan]

[...]

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

Sure we could still return reply... Then back in the callers make the
same virJSONValueObjectGetArray call without the error check to get
devices, but I didn't see the value in doing so.

Returning both reply and devices is possible, but similar such uses in
the past haven't been wildly popular.

And yes, I figured out what the code does. We differ in opinion on the
value of combining the duplicated code.

Since you plan on making further adjustments in this space, then fine as
I've already pointed out I'll drop patches 2-5.


John

>> fetching devices again seemed pointless.
>>
>> or you could also be referencing the for loop checks :
>>
>>         if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
>>             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>                            _("block info device entry was not in
>> expected format"));
>>             goto cleanup;
>>         }
>>
>>         if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) {
>>             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>                            _("block info device entry was not in
>> expected format"));
>>             goto cleanup;
>>         }
>>
>>
>> I see a very nominally small value in creating a helper to do those checks.
> 
> Well, that's basically the core of the duplicated code you are trying to
> delete ... There's not much left after that ...
> 
>>
>> In any case, I can drop patches 2-5. I do see value in them, but also
>> understand your point about the callbacks and arguments.
> 
> I fail to see the value.
> This is the "duplicated" code according to your patch. Let's deconstruct
> it:
> 
>     if (!(devices = virJSONValueObjectGetArray(reply, "return"))) {
>         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                        _("block info reply was missing device list"));
>         goto cleanup;
>     } [1]
> 
>     for (i = 0; i < virJSONValueArraySize(devices); i++) { [2]
>         virJSONValuePtr dev;
>         const char *thisdev;
>         int rc;
> 
>         dev = virJSONValueArrayGet(devices, i); [3]
>         if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
>             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                            _("block info device or type was not in "
>                              "expected format"));
>             goto cleanup;
>         } [4]
> 
>         if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) {
>             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                            _("block info device entry was not in "
>                              "expected format"));
>             goto cleanup;
>         } [5]
> 
> So let's tear it down piece by piece:
> [1] A global check which can easily be extracted after patch 1 into the
> common function.
> 
> [4] If this check bothers you, for a simple expense of adding a for loop
> this can be part of the common code calling the monitor command itself.
> One iteration over a rather short array, where you can make sure that
> the returned JSON array is valid.
> 
> [5] This extracts the name of the device. A simple static function can
> replace it and report the proper error.
>   if (!(thisdev = qemuMonitorQueryBlockGetDevice(dev)))
>       goto cleanup;
> 
> [2] and [3] This very complex (sarcasm) structure loops over the entries
> and extracts individual members. I really fail to see how extracting
> this at the price of callbacks and/or arguments hidden in a structure
> make the code any better or any further expansions simpler.
> 
> Peter
>