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