On 03/25/2014 02:23 AM, Nehal J Wani wrote:
> src/util/virfile.c
> *Check if libvirtd is running uninstalled from a build tree and change
> iohelper_path accordingly
>
> ---
> src/util/virfile.c | 19 +++++++++++++++++--
> 1 files changed, 17 insertions(+), 2 deletions(-)
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.
> int mode = -1;
> + char *uninstalledDir = NULL;
Missing a const (more obvious once the .h in patch 1/6 documents const
in the function declaration).
> + uninstalledDir = virGetUninstalledDir();
> +
> + if (uninstalledDir && virAsprintf(&iohelper_path,
> + "%s/../../src/libvirt_iohelper",
> + uninstalledDir) < 0)
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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list