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