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

[Peter Krempa]

On Mon, Oct 03, 2016 at 07:49:44 -0400, John Ferlan wrote:
> 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.

That should be perhaps a hint that they should not be merged.

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

If it's a problem to add it to the current structures there are two
options:

1) The design of the new code is bad
2) The design of the libvirt code currently in is bad

Looking at the code, the design of the feature is bad. The checks don't
necessarily need to be in the monitor extractor, but the usage place can
check that the required data is present and valid.

Currently the libvirt block job code is not really how it should be. We
are not able to properly track the bakcing chains in the XML and map
them to qemu block information nor are we using node names. Until we fix
this all block job code won't work properly

Either way we should not try to find ways how to hack it around but make
it properly.

I plan to finally get around and fix all the block job and backing chain
problems we have after I finish with the vcpu stuff so I'd really
appreciate if I don't have to remove hacks like this in the near future.

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

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?

> 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