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