[libvirt] [PATCHv2 0/6] Utility functions for storing uninstalled location
When libvirtd is run from a build directory without being installed, it should not depend on files from a libvirt package installed in the system. Currently, APIs defined in src/ don't know whether libvirtd is being run from the build dir or the installed dir. The following additions provide the functionality to do so: virSetUninstalledDir virGetUninstalledDir Example usage (utility = lxc|iohelper): char *path_tmp = virGetUninstalledDir(); if (path_tmp) /* do stuff with ("%s/../../src/libvirt_<utility>", path_tmp) */ else /* do stuff with (LIBEXECDIR "/libvirt_<utility>") */ v1: Refer: https://www.redhat.com/archives/libvir-list/2014-March/msg01427.html Nehal J Wani (6): Add utility functions for storing uninstalled location Use virGetUninstalledDir() in src/util/virfile.c Use virGetUninstalledDir() in src/lxc/lxc_conf.c Use virGetUninstalledDir() in src/storage/storage_backend_disk.c Use virGetUninstalledDir() in src/fdstream.c Remove obsolete function virFDStreamSetIOHelper()
src/libvirt_private.syms *Add symbols daemon/libvirtd.c *Set uninstallDir when libvirtd is running uninstalled from a build tree src/util/virutil.c *Introduce virSetUninstalledDir *Introduce virGetUninstalledDir --- daemon/libvirtd.c | 1 + src/libvirt_private.syms | 2 ++ src/util/virutil.c | 27 +++++++++++++++++++++++++++ src/util/virutil.h | 3 +++ 4 files changed, 33 insertions(+), 0 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 4179147..dc3da2a 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1165,6 +1165,7 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } *tmp = '\0'; + virSetUninstalledDir(argv[0]); char *driverdir; if (virAsprintfQuiet(&driverdir, "%s/../../src/.libs", argv[0]) < 0 || virAsprintfQuiet(&cpumap, "%s/../../src/cpu/cpu_map.xml", diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2357f95..3549868 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1997,6 +1997,7 @@ virGetGroupList; virGetGroupName; virGetHostname; virGetSelfLastChanged; +virGetUninstalledDir; virGetUnprivSGIOSysfsPath; virGetUserCacheDirectory; virGetUserConfigDirectory; @@ -2025,6 +2026,7 @@ virSetInherit; virSetNonBlock; virSetUIDGID; virSetUIDGIDWithCaps; +virSetUninstalledDir; virStrIsPrint; virUpdateSelfLastChanged; virValidateWWN; diff --git a/src/util/virutil.c b/src/util/virutil.c index 733cdff..47afbc4 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2204,3 +2204,30 @@ void virUpdateSelfLastChanged(const char *path) selfLastChanged = sb.st_ctime; } } + +static char *uninstalledDir = NULL; +/** + * virSetUninstalledDir: + * @path: directory containing an uninstalled binary, for use in locating + * other uninstalled files + * + * Set a pointer to the path which can be accessed by all other APIs using + * virGetUninstalledDir(). + */ +void virSetUninstalledDir(char *path) +{ + ignore_value(VIR_STRDUP(uninstalledDir, path)); +} + +/** + * virGetUninstalledDir: + * + * If libvirtd (or other binary) is running uninstalled from a build tree, + * return the directory containing the binary. A NULL return implies that + * the binary has been installed and paths are relying on <configmake.h> + * locations. + */ +char *virGetUninstalledDir(void) +{ + return uninstalledDir; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 1f2efd5..3e6ba47 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -202,4 +202,7 @@ bool virIsSUID(void); time_t virGetSelfLastChanged(void); void virUpdateSelfLastChanged(const char *path); +void virSetUninstalledDir(char *path); +char *virGetUninstalledDir(void); + #endif /* __VIR_UTIL_H__ */ -- 1.7.1
On Tue, Mar 25, 2014 at 01:53:11PM +0530, Nehal J Wani wrote:
src/libvirt_private.syms *Add symbols
daemon/libvirtd.c *Set uninstallDir when libvirtd is running uninstalled from a build tree
src/util/virutil.c *Introduce virSetUninstalledDir *Introduce virGetUninstalledDir
--- daemon/libvirtd.c | 1 + src/libvirt_private.syms | 2 ++ src/util/virutil.c | 27 +++++++++++++++++++++++++++ src/util/virutil.h | 3 +++ 4 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 4179147..dc3da2a 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1165,6 +1165,7 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } *tmp = '\0'; + virSetUninstalledDir(argv[0]);
argv[0] is not a directory - it is the path to the libvirtd file. Either this is a mistake, or this is a very misleading function name. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Mar 25, 2014 at 10:11:14AM +0000, Daniel P. Berrange wrote:
On Tue, Mar 25, 2014 at 01:53:11PM +0530, Nehal J Wani wrote:
src/libvirt_private.syms *Add symbols
daemon/libvirtd.c *Set uninstallDir when libvirtd is running uninstalled from a build tree
src/util/virutil.c *Introduce virSetUninstalledDir *Introduce virGetUninstalledDir
--- daemon/libvirtd.c | 1 + src/libvirt_private.syms | 2 ++ src/util/virutil.c | 27 +++++++++++++++++++++++++++ src/util/virutil.h | 3 +++ 4 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 4179147..dc3da2a 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1165,6 +1165,7 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } *tmp = '\0'; + virSetUninstalledDir(argv[0]);
argv[0] is not a directory - it is the path to the libvirtd file. Either this is a mistake, or this is a very misleading function name.
Eww, gross, I see we're modifying argv[0] in place, ignore my comment above. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/25/2014 02:23 AM, Nehal J Wani wrote:
src/libvirt_private.syms *Add symbols
daemon/libvirtd.c *Set uninstallDir when libvirtd is running uninstalled from a build tree
src/util/virutil.c *Introduce virSetUninstalledDir *Introduce virGetUninstalledDir
--- daemon/libvirtd.c | 1 + src/libvirt_private.syms | 2 ++ src/util/virutil.c | 27 +++++++++++++++++++++++++++ src/util/virutil.h | 3 +++ 4 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 4179147..dc3da2a 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1165,6 +1165,7 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } *tmp = '\0'; + virSetUninstalledDir(argv[0]);
Given Dan's complaint about us abusing this variable, I'll squash in something to make the naming more obvious.
+++ b/src/util/virutil.c @@ -2204,3 +2204,30 @@ void virUpdateSelfLastChanged(const char *path) selfLastChanged = sb.st_ctime; } } + +static char *uninstalledDir = NULL;
Static variables are guaranteed 0-initialization by the C standard; explicitly assigning NULL makes some compilers actually bloat the executable size because they stick the variable into .data instead of .bss (gcc knows better, though).
+/**
Style - we've been using 2 blank lines between functions (not enforced, but does make it visually easier to spot breaks when scrolling through files).
{ ignore_value(VIR_STRDUP(uninstalledDir, path));
Memory leak if this gets called more than once (not that it does; we call it 0 times when installed and exactly once when uninstalled) - plugging it will make sure we don't get false positives from Coverity and the like.
+/** + * virGetUninstalledDir: + * + * If libvirtd (or other binary) is running uninstalled from a build tree, + * return the directory containing the binary. A NULL return implies that + * the binary has been installed and paths are relying on <configmake.h> + * locations. + */ +char *virGetUninstalledDir(void)
Needs a const. Potential ACK. Here's what I'll squash in, and I'll push the result after reviewing the rest of the series if I don't spot anything else major. Hmm, in re-reading things, I noticed that the one place you are calling the new set function, you are passing in /path/to/builddir/daemon/.libs/ as the directory. I think it might work better to pass in /path/to/builddir, and teach all clients to specify paths directly relative to builddir, than having to do things relative to /path/to/builddir/daemon/.libs/../..; I'll have to see how you actually use the directory in later patches. diff --git i/daemon/libvirtd.c w/daemon/libvirtd.c index f0faf1a..0754fb5 100644 --- i/daemon/libvirtd.c +++ w/daemon/libvirtd.c @@ -1158,23 +1158,26 @@ int main(int argc, char **argv) { if (strstr(argv[0], "lt-libvirtd") || strstr(argv[0], "/daemon/.libs/libvirtd")) { + char *dir; char *tmp = strrchr(argv[0], '/'); char *cpumap; - if (!tmp) { - fprintf(stderr, _("%s: cannot identify driver directory\n"), argv[0]); + char *driverdir; + + if (!tmp || VIR_STRNDUP(dir, argv[0], tmp - argv[0]) < 0) { + fprintf(stderr, _("%s: cannot identify driver directory\n"), + argv[0]); exit(EXIT_FAILURE); } - *tmp = '\0'; - virSetUninstalledDir(argv[0]); - char *driverdir; - if (virAsprintfQuiet(&driverdir, "%s/../../src/.libs", argv[0]) < 0 || + virSetUninstalledDir(dir); + if (virAsprintfQuiet(&driverdir, "%s/../../src/.libs", dir) < 0 || virAsprintfQuiet(&cpumap, "%s/../../src/cpu/cpu_map.xml", - argv[0]) < 0) { + dir) < 0) { fprintf(stderr, _("%s: initialization failed\n"), argv[0]); exit(EXIT_FAILURE); } if (access(driverdir, R_OK) < 0) { - fprintf(stderr, _("%s: expected driver directory '%s' is missing\n"), + fprintf(stderr, + _("%s: expected driver directory '%s' is missing\n"), argv[0], driverdir); exit(EXIT_FAILURE); } @@ -1184,7 +1187,7 @@ int main(int argc, char **argv) { #endif cpuMapOverride(cpumap); VIR_FREE(cpumap); - *tmp = '/'; + VIR_FREE(dir); /* Must not free 'driverdir' - it is still used */ } diff --git i/src/util/virutil.c w/src/util/virutil.c index 244b9e9..dabd4f8 100644 --- i/src/util/virutil.c +++ w/src/util/virutil.c @@ -2205,7 +2205,9 @@ void virUpdateSelfLastChanged(const char *path) } } -static char *uninstalledDir = NULL; + +static char *uninstalledDir; + /** * virSetUninstalledDir: * @path: directory containing an uninstalled binary, for use in locating @@ -2214,8 +2216,9 @@ static char *uninstalledDir = NULL; * Set a pointer to the path which can be accessed by all other APIs using * virGetUninstalledDir(). */ -void virSetUninstalledDir(char *path) +void virSetUninstalledDir(const char *path) { + VIR_FREE(uninstalledDir); ignore_value(VIR_STRDUP(uninstalledDir, path)); } @@ -2227,7 +2230,7 @@ void virSetUninstalledDir(char *path) * the binary has been installed and paths are relying on <configmake.h> * locations. */ -char *virGetUninstalledDir(void) +const char *virGetUninstalledDir(void) { return uninstalledDir; } diff --git i/src/util/virutil.h w/src/util/virutil.h index 3e6ba47..437167a 100644 --- i/src/util/virutil.h +++ w/src/util/virutil.h @@ -202,7 +202,7 @@ bool virIsSUID(void); time_t virGetSelfLastChanged(void); void virUpdateSelfLastChanged(const char *path); -void virSetUninstalledDir(char *path); -char *virGetUninstalledDir(void); +void virSetUninstalledDir(const char *path); +const char *virGetUninstalledDir(void); #endif /* __VIR_UTIL_H__ */ -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Mar 25, 2014 at 01:53:11PM +0530, Nehal J Wani wrote:
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 4179147..dc3da2a 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1165,6 +1165,7 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } *tmp = '\0'; + virSetUninstalledDir(argv[0]);
Should check for failure
diff --git a/src/util/virutil.c b/src/util/virutil.c index 733cdff..47afbc4 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2204,3 +2204,30 @@ void virUpdateSelfLastChanged(const char *path) selfLastChanged = sb.st_ctime; } } + +static char *uninstalledDir = NULL; +/** + * virSetUninstalledDir: + * @path: directory containing an uninstalled binary, for use in locating + * other uninstalled files + * + * Set a pointer to the path which can be accessed by all other APIs using + * virGetUninstalledDir(). + */ +void virSetUninstalledDir(char *path)
Arg shoud be 'const char *'
+{ + ignore_value(VIR_STRDUP(uninstalledDir, path)); +}
We shouldn't ignore OOM - we should propagate it.
+ +/** + * virGetUninstalledDir: + * + * If libvirtd (or other binary) is running uninstalled from a build tree, + * return the directory containing the binary. A NULL return implies that + * the binary has been installed and paths are relying on <configmake.h> + * locations. + */ +char *virGetUninstalledDir(void)
Return value should be const * ACK, but I'm squashing in diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index f0faf1a..021fdc7 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1165,7 +1165,10 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } *tmp = '\0'; - virSetUninstalledDir(argv[0]); + if (virSetUninstalledDir(argv[0]) < 0) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } char *driverdir; if (virAsprintfQuiet(&driverdir, "%s/../../src/.libs", argv[0]) < 0 || virAsprintfQuiet(&cpumap, "%s/../../src/cpu/cpu_map.xml", diff --git a/src/util/virutil.c b/src/util/virutil.c index 244b9e9..db94a56 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2213,10 +2213,13 @@ static char *uninstalledDir = NULL; * * Set a pointer to the path which can be accessed by all other APIs using * virGetUninstalledDir(). + * + * Returns -1 on error, 0 on success */ -void virSetUninstalledDir(char *path) +int virSetUninstalledDir(const char *path) { - ignore_value(VIR_STRDUP(uninstalledDir, path)); + VIR_FREE(uninstalledDir); + return VIR_STRDUP(uninstalledDir, path); } /** @@ -2227,7 +2230,7 @@ void virSetUninstalledDir(char *path) * the binary has been installed and paths are relying on <configmake.h> * locations. */ -char *virGetUninstalledDir(void) +const char *virGetUninstalledDir(void) { return uninstalledDir; } diff --git a/src/util/virutil.h b/src/util/virutil.h index 3e6ba47..13db8c8 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -202,7 +202,7 @@ bool virIsSUID(void); time_t virGetSelfLastChanged(void); void virUpdateSelfLastChanged(const char *path); -void virSetUninstalledDir(char *path); -char *virGetUninstalledDir(void); +int virSetUninstalledDir(const char *path) ATTRIBUTE_RETURN_CHECK; +const char *virGetUninstalledDir(void); #endif /* __VIR_UTIL_H__ */ Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index fefc3fb..813ad86 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -193,6 +193,8 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) bool output = false; int pipefd[2] = { -1, -1 }; int mode = -1; + char *uninstalledDir = NULL; + char *iohelper_path = NULL; if (!flags) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -236,8 +238,20 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) goto error; } - ret->cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper", - name, "0", NULL); + uninstalledDir = virGetUninstalledDir(); + + if (uninstalledDir && virAsprintf(&iohelper_path, + "%s/../../src/libvirt_iohelper", + uninstalledDir) < 0) + goto error; + else if (virAsprintf(&iohelper_path, + LIBEXECDIR "/libvirt_iohelper") < 0) + goto error; + + ret->cmd = virCommandNewArgList(iohelper_path, name, "0", NULL); + + VIR_FREE(iohelper_path); + if (output) { virCommandSetInputFD(ret->cmd, pipefd[0]); virCommandSetOutputFD(ret->cmd, fd); @@ -268,6 +282,7 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) return ret; error: + VIR_FREE(iohelper_path); VIR_FORCE_CLOSE(pipefd[0]); VIR_FORCE_CLOSE(pipefd[1]); virFileWrapperFdFree(ret); -- 1.7.1
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
On Tue, Mar 25, 2014 at 01:53:12PM +0530, 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(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index fefc3fb..813ad86 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -193,6 +193,8 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) bool output = false; int pipefd[2] = { -1, -1 }; int mode = -1; + char *uninstalledDir = NULL;
Should be 'const char *' since we don't own this pointer ACK will fix when pushing Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
src/lxc/lxc_conf.c: *Check if libvirtd is running uninstalled from a build tree and change lxc_path accordingly --- src/lxc/lxc_conf.c | 19 +++++++++++++++++-- 1 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index d4432cf..ad8f78a 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -66,6 +66,8 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver) virCapsPtr caps; virCapsGuestPtr guest; virArch altArch; + char *uninstalledDir = NULL; + char *lxc_path = NULL; if ((caps = virCapabilitiesNew(virArchFromHost(), 0, 0)) == NULL) @@ -89,10 +91,20 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver) goto error; } + uninstalledDir = virGetUninstalledDir(); + + if (uninstalledDir && virAsprintf(&lxc_path, + "%s/../../src/libvirt_lxc", + uninstalledDir) < 0) + goto error; + else if (virAsprintf(&lxc_path, + LIBEXECDIR "/libvirt_lxc") < 0) + goto error; + if ((guest = virCapabilitiesAddGuest(caps, "exe", caps->host.arch, - LIBEXECDIR "/libvirt_lxc", + lxc_path, NULL, 0, NULL)) == NULL) @@ -111,7 +123,7 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver) if ((guest = virCapabilitiesAddGuest(caps, "exe", altArch, - LIBEXECDIR "/libvirt_lxc", + lxc_path, NULL, 0, NULL)) == NULL) @@ -126,6 +138,8 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver) goto error; } + VIR_FREE(lxc_path); + if (driver) { /* Security driver data */ const char *doi, *model, *label, *type; @@ -158,6 +172,7 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver) return caps; error: + VIR_FREE(lxc_path); virObjectUnref(caps); return NULL; } -- 1.7.1
On Tue, Mar 25, 2014 at 01:53:13PM +0530, Nehal J Wani wrote:
src/lxc/lxc_conf.c: *Check if libvirtd is running uninstalled from a build tree and change lxc_path accordingly
--- src/lxc/lxc_conf.c | 19 +++++++++++++++++-- 1 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index d4432cf..ad8f78a 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -66,6 +66,8 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver) virCapsPtr caps; virCapsGuestPtr guest; virArch altArch; + char *uninstalledDir = NULL;
Again should have been const. ACK will fix that myself Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
src/storage/storage_backend_disk.c: *Check if libvirtd is running uninstalled from a build tree and change parthelper_path accordingly --- src/storage/storage_backend_disk.c | 28 ++++++++++++++++++++++++++-- 1 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 81201fd..bb29d39 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -261,7 +261,18 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool, * - normal metadata 100027630080 100030242304 2612736 * */ - virCommandPtr cmd = virCommandNewArgList(PARTHELPER, + + char *uninstalledDir = virGetUninstalledDir(); + char *parthelper_path = NULL; + + if (uninstalledDir && virAsprintf(&parthelper_path, + "%s/../../src/libvirt_parthelper", + uninstalledDir) < 0) + return -1; + else if (virAsprintf(&parthelper_path, PARTHELPER) < 0) + return -1; + + virCommandPtr cmd = virCommandNewArgList(parthelper_path, pool->def->source.devices[0].path, NULL); struct virStorageBackendDiskPoolVolData cbdata = { @@ -277,6 +288,7 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool, virStorageBackendDiskMakeVol, &cbdata); virCommandFree(cmd); + VIR_FREE(parthelper_path); return ret; } @@ -301,7 +313,18 @@ virStorageBackendDiskMakePoolGeometry(size_t ntok ATTRIBUTE_UNUSED, static int virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool) { - virCommandPtr cmd = virCommandNewArgList(PARTHELPER, + char *uninstalledDir = virGetUninstalledDir(); + char *parthelper_path = NULL; + + if (uninstalledDir && virAsprintf(&parthelper_path, + "%s/../../src/libvirt_parthelper", + uninstalledDir) < 0) + return -1; + else if (virAsprintf(&parthelper_path, PARTHELPER) < 0) + return -1; + + + virCommandPtr cmd = virCommandNewArgList(parthelper_path, pool->def->source.devices[0].path, "-g", NULL); @@ -312,6 +335,7 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool) virStorageBackendDiskMakePoolGeometry, pool); virCommandFree(cmd); + VIR_FREE(parthelper_path); return ret; } -- 1.7.1
On Tue, Mar 25, 2014 at 01:53:14PM +0530, Nehal J Wani wrote:
src/storage/storage_backend_disk.c: *Check if libvirtd is running uninstalled from a build tree and change parthelper_path accordingly
--- src/storage/storage_backend_disk.c | 28 ++++++++++++++++++++++++++-- 1 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 81201fd..bb29d39 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -261,7 +261,18 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool, * - normal metadata 100027630080 100030242304 2612736 * */ - virCommandPtr cmd = virCommandNewArgList(PARTHELPER, + + char *uninstalledDir = virGetUninstalledDir(); + char *parthelper_path = NULL; + + if (uninstalledDir && virAsprintf(&parthelper_path, + "%s/../../src/libvirt_parthelper", + uninstalledDir) < 0) + return -1; + else if (virAsprintf(&parthelper_path, PARTHELPER) < 0) + return -1; + + virCommandPtr cmd = virCommandNewArgList(parthelper_path,
Hmm, it getting rather ugly & tedious to repeat this logic every place we need it. I think we need another helper API really virFileFindBinary(const char *srcdir, const char *bindir, const char *binaryname); which we'd call thus path = virFileFindBinary("../../src", LIBEXECDIR, "libvirt_parthelper") I think I will re-write this series todo that and re-post Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/24/2014 07:51 AM, Daniel P. Berrange wrote:
On Tue, Mar 25, 2014 at 01:53:14PM +0530, Nehal J Wani wrote:
src/storage/storage_backend_disk.c: *Check if libvirtd is running uninstalled from a build tree and change parthelper_path accordingly
Hmm, it getting rather ugly & tedious to repeat this logic every place we need it.
I came to the same conclusion yesterday :)
I think we need another helper API really
virFileFindBinary(const char *srcdir, const char *bindir, const char *binaryname);
Slightly different signature than my proposal - please read https://www.redhat.com/archives/libvir-list/2014-April/msg00902.html before doing too much duplicate work. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
src/storage/storage_backend_disk.c: *Check if libvirtd is running uninstalled from a build tree and change parthelper_path accordingly --- src/fdstream.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index d0435c7..031d9f5 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -593,6 +593,8 @@ virFDStreamOpenFileInternal(virStreamPtr st, struct stat sb; virCommandPtr cmd = NULL; int errfd = -1; + char *uninstalledDir = NULL; + char *iohelper_path = NULL; VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o", st, path, oflags, offset, length, mode); @@ -648,9 +650,22 @@ virFDStreamOpenFileInternal(virStreamPtr st, goto error; } + uninstalledDir = virGetUninstalledDir(); + + if (uninstalledDir && virAsprintf(&iohelper_path, + "%s/../../src/libvirt_iohelper", + uninstalledDir) < 0) + goto error; + else if (virAsprintf(&iohelper_path, + LIBEXECDIR "/libvirt_iohelper") < 0) + goto error; + cmd = virCommandNewArgList(iohelper_path, path, NULL); + + VIR_FREE(iohelper_path); + virCommandAddArgFormat(cmd, "%llu", length); virCommandPassFD(cmd, fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); @@ -683,6 +698,7 @@ error: VIR_FORCE_CLOSE(fd); VIR_FORCE_CLOSE(childfd); VIR_FORCE_CLOSE(errfd); + VIR_FREE(iohelper_path); if (oflags & O_CREAT) unlink(path); return -1; -- 1.7.1
Now that we have virSetUninstalledDir() and virGetUninstalledDir(), we no longer need virFDStreamSetIOHelper(). --- src/fdstream.c | 11 ----------- src/fdstream.h | 2 -- src/libvirt_private.syms | 1 - tests/fdstreamtest.c | 3 --- 4 files changed, 0 insertions(+), 17 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index 031d9f5..dbe2781 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -78,17 +78,6 @@ struct virFDStreamData { }; -static const char *iohelper_path = LIBEXECDIR "/libvirt_iohelper"; - -void virFDStreamSetIOHelper(const char *path) -{ - if (path == NULL) - iohelper_path = LIBEXECDIR "/libvirt_iohelper"; - else - iohelper_path = path; -} - - static int virFDStreamRemoveCallback(virStreamPtr stream) { struct virFDStreamData *fdst = stream->privateData; diff --git a/src/fdstream.h b/src/fdstream.h index 9c7295d..6292cd5 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -34,8 +34,6 @@ typedef void (*virFDStreamInternalCloseCbFreeOpaque)(void *opaque); /* Only for use by test suite */ -void virFDStreamSetIOHelper(const char *path); - int virFDStreamOpen(virStreamPtr st, int fd); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3549868..6ac54ea 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -809,7 +809,6 @@ virFDStreamCreateFile; virFDStreamOpen; virFDStreamOpenFile; virFDStreamOpenPTY; -virFDStreamSetIOHelper; # libvirt_internal.h diff --git a/tests/fdstreamtest.c b/tests/fdstreamtest.c index 92e7add..f1ef42a 100644 --- a/tests/fdstreamtest.c +++ b/tests/fdstreamtest.c @@ -321,9 +321,6 @@ mymain(void) { char scratchdir[] = SCRATCHDIRTEMPLATE; int ret = 0; - const char *iohelper = abs_builddir "/../src/libvirt_iohelper"; - - virFDStreamSetIOHelper(iohelper); if (!mkdtemp(scratchdir)) { virFilePrintf(stderr, "Cannot create fakesysfsdir"); -- 1.7.1
On 03/25/2014 02:23 AM, Nehal J Wani wrote:
Now that we have virSetUninstalledDir() and virGetUninstalledDir(), we no longer need virFDStreamSetIOHelper().
---
-static const char *iohelper_path = LIBEXECDIR "/libvirt_iohelper"; - -void virFDStreamSetIOHelper(const char *path) -{ - if (path == NULL) - iohelper_path = LIBEXECDIR "/libvirt_iohelper"; - else - iohelper_path = path; -}
Problem. Just because we have virGetUninstalldDir() doesn't mean we are using that, unless you ALSO fix the testsuite to call virSetUninstalledDir(). As written, your series only calls virSetUninstalledDir() in daemon/libvirtd.c,...
+++ b/tests/fdstreamtest.c @@ -321,9 +321,6 @@ mymain(void) { char scratchdir[] = SCRATCHDIRTEMPLATE; int ret = 0; - const char *iohelper = abs_builddir "/../src/libvirt_iohelper"; - - virFDStreamSetIOHelper(iohelper);
...but fdstreamtest.c is not using daemon/libvirtd.c, which means you just broke this file into running the pre-installed version instead of the in-tree version. For this series to work, you need to make testutils.c call virSetUninstalledDir on behalf of all test programs. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Mar 25, 2014 at 1:53 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
When libvirtd is run from a build directory without being installed, it should not depend on files from a libvirt package installed in the system. Currently, APIs defined in src/ don't know whether libvirtd is being run from the build dir or the installed dir. The following additions provide the functionality to do so:
virSetUninstalledDir virGetUninstalledDir
Example usage (utility = lxc|iohelper): char *path_tmp = virGetUninstalledDir(); if (path_tmp) /* do stuff with ("%s/../../src/libvirt_<utility>", path_tmp) */ else /* do stuff with (LIBEXECDIR "/libvirt_<utility>") */
v1: Refer: https://www.redhat.com/archives/libvir-list/2014-March/msg01427.html
Nehal J Wani (6): Add utility functions for storing uninstalled location Use virGetUninstalledDir() in src/util/virfile.c Use virGetUninstalledDir() in src/lxc/lxc_conf.c Use virGetUninstalledDir() in src/storage/storage_backend_disk.c Use virGetUninstalledDir() in src/fdstream.c Remove obsolete function virFDStreamSetIOHelper()
Ping! -- Nehal J Wani
On Tue, Mar 25, 2014 at 1:53 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
When libvirtd is run from a build directory without being installed, it should not depend on files from a libvirt package installed in the system. Currently, APIs defined in src/ don't know whether libvirtd is being run from the build dir or the installed dir. The following additions provide the functionality to do so:
virSetUninstalledDir virGetUninstalledDir
Example usage (utility = lxc|iohelper): char *path_tmp = virGetUninstalledDir(); if (path_tmp) /* do stuff with ("%s/../../src/libvirt_<utility>", path_tmp) */ else /* do stuff with (LIBEXECDIR "/libvirt_<utility>") */
v1: Refer: https://www.redhat.com/archives/libvir-list/2014-March/msg01427.html
Nehal J Wani (6): Add utility functions for storing uninstalled location Use virGetUninstalledDir() in src/util/virfile.c Use virGetUninstalledDir() in src/lxc/lxc_conf.c Use virGetUninstalledDir() in src/storage/storage_backend_disk.c Use virGetUninstalledDir() in src/fdstream.c Remove obsolete function virFDStreamSetIOHelper()
Ping? -- Nehal J Wani
Ping! Once this gets through, the custom leases API will get through, and then the virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC APIs will be introduced. On Tue, Mar 25, 2014 at 1:53 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
When libvirtd is run from a build directory without being installed, it should not depend on files from a libvirt package installed in the system. Currently, APIs defined in src/ don't know whether libvirtd is being run from the build dir or the installed dir. The following additions provide the functionality to do so:
virSetUninstalledDir virGetUninstalledDir
Example usage (utility = lxc|iohelper): char *path_tmp = virGetUninstalledDir(); if (path_tmp) /* do stuff with ("%s/../../src/libvirt_<utility>", path_tmp) */ else /* do stuff with (LIBEXECDIR "/libvirt_<utility>") */
v1: Refer: https://www.redhat.com/archives/libvir-list/2014-March/msg01427.html
Nehal J Wani (6): Add utility functions for storing uninstalled location Use virGetUninstalledDir() in src/util/virfile.c Use virGetUninstalledDir() in src/lxc/lxc_conf.c Use virGetUninstalledDir() in src/storage/storage_backend_disk.c Use virGetUninstalledDir() in src/fdstream.c Remove obsolete function virFDStreamSetIOHelper()
-- Nehal J Wani
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Nehal J Wani