Re: [libvirt] [PATCH v2 0/5] Combine various query-block json call paths
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
Attachments:
- signature.asc (application/pgp-signature — 801 bytes)