Re: [libvirt] [PATCH v2 0/5] Combine various query-block json call paths
On 10/03/2016 03:22 AM, Peter Krempa wrote:
On Fri, Sep 30, 2016 at 14:39:16 -0400, John Ferlan wrote:
> v1: http://www.redhat.com/archives/libvir-list/2016-September/msg01446.html
>
> NOTE: Patch 1 already ACK'd
>
> Patches 2-4 adjusted slightly to not create/use qemuMonitorJSONQueryBlockArgs
> instead opting to pass all the args (also shortened helper names slightly).
>
> Patch 5 adjusted to receive all args in qemuMonitorJSONQueryBlock and then
> call one of two callbacks based on whether the table or search is being used.
> I did have a version that used ATTRIBUTE_UNUSED and one generic callback, but
> I thought that was uglier.
It is quite ugly, but that does not differ much from v1. At least with
this versions you don't have to jump around to find the arguments for
the callbacks.
As I've said in v1 review I don't quite see a value in introducing
callbacks just to be able to extract 3 repetitions of a for loop.
Especially since the worker functions take pretty diverse parameters.
And as I said in my response - it's primarily to cut down on cut-n-paste
(code repetition) of the 3 methods that are parsing the same returned
data in the same way, but looking for different things.
Two of the workers take essentially the same parameters (one takes
backingChain) while the third worker is quite different, but that's
because it's looking for more specific data. As also noted in my
response, it's the PITA.
The goal/purpose was a reaction to changes proposed in the introduce
pull backups patch series which modified the returned GetBlockInfo to
add a fetch/store of 'virtual-size'. Those changes introduced some error
logic into a function that concerned me. IIRC, the logic took what could
be a non static value and treated it as static. So rather than fetch the
value just once at startup, the thought was introduce a means that would
allow "new code" to fetch what was desired as it was needed. In fact, I
think my response indicated that code should mimic how the
FillDiskNameLookup code works although perhaps misinterpreted to being
add the virtual-size field to that code.
What would make sense to extract is the checks that validate that the
returned data is in somewhat sane format in the helper that you've added
in patch 1. (also mentioned in previous review).
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)).
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 and
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.
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.
John