Re: [libvirt] [PATCHv2 2/6] Use virGetUninstalledDir() in src/util/virfile.c

On 03/25/2014 02:23 AM, Nehal J Wani wrote: That's a rather large diffstat for the convenience of running uninstalled. I still think we need this series; but it's making me wonder if we are putting too much repetitive burden on individual callers, which should instead be factored into the common helper routine. Missing a const (more obvious once the .h in patch 1/6 documents const in the function declaration). Yep, this confirms my suspicion that I very much dislike patch 1/6 supplying '/path/to/build/daemon/.libs' as the uninstalled dir; it's forcing the caller in a completely different file to have to guess that must supply a leading '../../'. I really do think it's better to have virSetUninstalledDir store the location of ${abs_builddir}, not ${abs_builddir}/daemon/.libs. The patch in isolation is fine, but I'm more worried about the bigger picture. That is, the high-level view of what we are doing here is: find me the correct 'libvirt_iohelper' binary, favoring ${builddir}/src if running in-tree, and LIBEXECDIR otherwise. In patch 3/6, we want: find me the correct 'libvirt_lxc' binary, favoring ${builddir}/src if running in-tree, and LIBEXECDIR otherwise. In daemon/libvirtd.c (touched in 1/6), we have pre-existing uses of virAsprintf, feeding into src/cpu/cpu_map.c, where we are in reality trying to express: find me the correct 'cpu_map.xml', favoring ${builddir}/src/cpu/ if running in-tree, and PKGDATADIR otherwise. [In fact, for cpu_map.xml, it's even worse - we did some makefile hackery to guarantee that in a VPATH build, ${builddir}/src/cpu/cpu_map.xml is a symlink to ${srcdir}/src/cpu/cpu_map.xml] I'm sensing a common pattern - I think a more useful interface might be: binary = virResourceLocate("libvirt_iohelper", LIBEXECDIR, "src"); virCommandNew(binary); VIR_FREE(binary); mapfile = virResourceLocate("cpu_map.xml", PKGDATADIR, "src/cpu"); xmlParseFile(mapfile); VIR_FREE(mapfile); I think that having virResourceLocate do the magic of deciding if we are running uninstalled, and do the virAsprintf, will make each call-site simpler. Furthermore, in the case of cpu_map.xml, we might even be able to figure out the srcdir vs. builddir tricks of VPATH builds without having to do quite so much makefile hackery. I also think that while your series is a good start, it feels incomplete until we have factored ALL of the disparate override calls in daemon/libvirtd.c to use the common theme of determining an in-tree resource when running uninstalled, instead of having 3 or 4 different approaches of which functions we call to register the fact that we are running in-tree. I'll see what I can do to help. Meanwhile, please don't let the slow review of this series stall you from submitting further patches. It's okay to submit patches that have prerequisites that still need review, and in fact, doing that sometimes gets the missing reviews supplied faster once people realize the missing reviews are holding up further development. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org