Re: [libvirt] [PATCH 0/5] Combine various query-block json call paths
On Fri, Sep 30, 2016 at 08:53:49 -0400, John Ferlan wrote:
Based off a recent review and discussion regarding adding a new
field
to qemuDomainDiskInfo:
http://www.redhat.com/archives/libvir-list/2016-September/msg00193.html
I decided to take the plunge and rework the query-block code in order
to combine the various ways callers return the data and well hopefully
allow for an easier "future mechanism" to call the query-block returning
just the required/requested data for any caller (with a little bit of
code wrangling).
I had a look at the patches and I'm not quite sure that you've succeeded
in the goal above.
John Ferlan (5):
qemu: Create common code for JSON "query-block" call
This patch is worthwhile it reduces some of the duplicated code that is
necessary for all callers.
Another similar cleanup could add a checker that validates that the data
returned by qemu are in the expected format which was duplicated.
qemu: Split out filling of JSONGetBlockInfo data
qemu: Split out filling of JSONBlockStats data
qemu: Split out filling of JSONDiskNameLookup data
qemu: Combine the various ways to call query-block
Those above are a bit hairy though. I don't see much value in
not-duplicating the loop iterating the replies.
I must say I don't like the arguments structure for the callback at all.
It really destroys readability of the callback functions.
Expanding at least the common arguments like the current device object
would help massively since you don't need to look into the structure,
but just the function header.
As the patches progressively add more and more stuff into the callback
data it really kills the advantage of using the callback at all. It's
just separate functions hidden by a structure.
Extracting the loop into a function really doesn't make sense.
src/qemu/qemu_monitor_json.c | 345
++++++++++++++++++++++++-------------------
1 file changed, 193 insertions(+), 152 deletions(-)
This also indicates that it didn't really help.
Peter