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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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

NACK to 2 - 5. 

Peter

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]