Re: [libvirt] [PATCH 0/5] Combine various query-block json call paths
On 09/30/2016 09:25 AM, Peter Krempa wrote:
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.
To me the advantage was from the aspect of cut-copy-paste to pull out
devices and then walk it doing the same error checking for all 3
methods, but I certainly understand your point.
The PITA is DiskNameLookup which has different args and wanting to exit
as soon as something is found.
I did consider making a 1 entry hash table and passing a bool to quit
when something was found, but didn't think it would be worth the
effort... I can look into that option again.
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.
While going through the exercise it just didn't feel right to have some
arguments non in the args structure just as much as it didn't feel right
to have arguments to a call that would require ATTRIBUTE_UNUSED just so
that I could have a common cb function argument list.
I'll give one more shot at reworking things and then not worry about it.
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.
How much of this is additional function entry comments though...
John