[libvirt] [PATCH] Add 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: src/util/virutil.c *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>") */ --- daemon/libvirtd.c | 1 + src/libvirt_private.syms | 2 ++ src/util/virutil.c | 25 +++++++++++++++++++++++++ src/util/virutil.h | 3 +++ 4 files changed, 31 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 5904036..a112e6e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1989,6 +1989,7 @@ virGetGroupList; virGetGroupName; virGetHostname; virGetSelfLastChanged; +virGetUninstalledDir; virGetUnprivSGIOSysfsPath; virGetUserCacheDirectory; virGetUserConfigDirectory; @@ -2017,6 +2018,7 @@ virSetInherit; virSetNonBlock; virSetUIDGID; virSetUIDGIDWithCaps; +virSetUninstalledDir; virStrIsPrint; virUpdateSelfLastChanged; virValidateWWN; diff --git a/src/util/virutil.c b/src/util/virutil.c index 733cdff..46f6c75 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2204,3 +2204,28 @@ void virUpdateSelfLastChanged(const char *path) selfLastChanged = sb.st_ctime; } } + +static char *uninstalledDir = NULL; +/** + * virSetUninstalledDir: + * @path: location from which libvirtd is running without + * installation + * + * Set a pointer to the path which can be accessed by all + * other APIs using virGetUninstalledDir(). + */ +void virSetUninstalledDir(char *path) +{ + uninstalledDir = path; +} + +/** + * virGetUninstalledDir: + * + * If libvirtd is running without installation, return the + * path from which it was invoked, otherwise, return NULL + */ +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 03/23/2014 02:22 AM, Nehal J Wani 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:
src/util/virutil.c *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>") */
Hmm - we already have virFDStreamSetIOHelper() for overriding the location of libvirt_iohelper, but I like your approach as something a bit more generic. Do we really need both mechanisms, or should your series include a followup patch to nuke virFDStreamSetIOHelper() and instead teach the testsuite to use your new functions (that is, testutils.c should probably call virSetUninstalledDir). I'm not sure if there is a better name for the concept (I'm a bit biased, because I helped you design this on IRC) - so I'll wait for other reviewers.
+static char *uninstalledDir = NULL; +/** + * virSetUninstalledDir: + * @path: location from which libvirtd is running without + * installation
Grammar is awkward. How about: @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) +{ + uninstalledDir = path;
Dangerous to not strdup this argument - if the caller modifies the argument (and libvirtd.c main() DOES modify the argument) then you are not pointing to the same string that you thought you were.
+} + +/** + * virGetUninstalledDir: + * + * If libvirtd is running without installation, return the + * path from which it was invoked, otherwise, return NULL
How about: 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 relying on <configmake.h> locations. Series is incomplete. It's not worth adding the new function unless you also have followup patches that use the new function. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/24/2014 02:54 PM, Eric Blake wrote:
On 03/23/2014 02:22 AM, Nehal J Wani 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:
src/util/virutil.c *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>") */
Hmm - we already have virFDStreamSetIOHelper() for overriding the location of libvirt_iohelper, but I like your approach as something a bit more generic. Do we really need both mechanisms, or should your series include a followup patch to nuke virFDStreamSetIOHelper() and instead teach the testsuite to use your new functions (that is, testutils.c should probably call virSetUninstalledDir).
A bit more to help others not following the IRC conversation. As it is, virFDStreamSetIOHelper() is not as useful as it could be. Observe this 'git grep' result: ./src/lxc/lxc_conf.c:95: LIBEXECDIR "/libvirt_lxc", ./src/lxc/lxc_conf.c:114: LIBEXECDIR "/libvirt_lxc", ./src/fdstream.c:81:static const char *iohelper_path = LIBEXECDIR "/libvirt_iohelper"; ./src/fdstream.c:86: iohelper_path = LIBEXECDIR "/libvirt_iohelper"; ./src/util/virfile.c:239: ret->cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper", ./src/storage/storage_backend_disk.c:43:#define PARTHELPER LIBEXECDIR "/libvirt_parthelper" Note that the testsuite knows how to run uninstalled libvirt_iohelper when testing fdstream.c directly, but also note that virfile.c uses libvirt_iohelper but never uses its overridden location - our existing override is only half-baked. And even though we have the override hook for iohelper, daemon/libvirt.c main() is not taking advantage of the hook - which means if you build libvirt.git on a system where libvirt is not installed, and try to use ./run to test your just-built binary, it will fail anywhere it tries to use libvirt_iohelper (because it will be trying to use the installed location, instead of the just-built in-tree location). So I'm definitely in favor of this patch, but think it needs more patches in the series before it is ready to push.
Series is incomplete. It's not worth adding the new function unless you also have followup patches that use the new function.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/24/2014 03:19 PM, Eric Blake wrote:
Note that the testsuite knows how to run uninstalled libvirt_iohelper when testing fdstream.c directly, but also note that virfile.c uses libvirt_iohelper but never uses its overridden location - our existing override is only half-baked. And even though we have the override hook for iohelper, daemon/libvirt.c main() is not taking advantage of the hook - which means if you build libvirt.git on a system where libvirt is not installed, and try to use ./run to test your just-built binary, it will fail anywhere it tries to use libvirt_iohelper (because it will be trying to use the installed location, instead of the just-built in-tree location). So I'm definitely in favor of this patch, but think it needs more patches in the series before it is ready to push.
I tried to actually reproduce this scenario in a VM (by attempting to play with 'virsh pool' commands for creating a use of libvirt_parthelper), but was foiled by hitting an even earlier failure. In window 1 as root: # yum uninstall -y libvirt\* ... # ./run daemon/libvirtd In window 2 as normal user: $ ./run tools/virsh -c qemu:///system pool-list --all error: failed to connect to the hypervisor error: authentication failed: polkit: Error checking for authorization org.libvirt.unix.manage: GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: Action org.libvirt.unix.manage is not registered Logs in window 1: 2014-03-24 22:46:59.321+0000: 7615: info : libvirt version: 1.2.3 2014-03-24 22:46:59.321+0000: 7615: error : virCommandWait:2426 : internal error: Child process (/usr/bin/firewall-cmd --direct --passthrough ipv4 --table filter --delete OUTPUT --out-interface virbr0 --protocol udp --destination-port 68 --jump ACCEPT) unexpected exit status 13 2014-03-24 22:47:06.341+0000: 7615: error : x86Decode:1573 : internal error: Cannot find suitable CPU model for given data 2014-03-24 22:47:06.341+0000: 7615: warning : virQEMUCapsInit:918 : Failed to get host CPU 2014-03-24 22:47:29.861+0000: 7599: warning : remoteDispatchAuthPolkit:3210 : No support for caller UID with pkcheck. This deployment is known to be insecure. 2014-03-24 22:47:30.204+0000: 7599: error : remoteDispatchAuthPolkit:3229 : Policy kit denied action org.libvirt.unix.manage from pid 7730, uid 1000 with status 127 2014-03-24 22:47:30.204+0000: 7599: error : remoteDispatchAuthPolkit:3257 : authentication failed: polkit: Error checking for authorization org.libvirt.unix.manage: GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: Action org.libvirt.unix.manage is not registered 2014-03-24 22:47:30.205+0000: 7566: error : virNetSocketReadWire:1456 : End of file while reading data: Input/output error And sure enough, installing libvirt-daemon-qemu then retrying the test let things succeed, so we definitely have an up-front failure situation where we are relying on installed files instead of ./run just working from an uninstalled tree (it's just that most developers, including myself, already have libvirtd installed, and normally don't run into situations where a mismatch between the older libexec helper utilities and the current libvirt development tree make a difference). I might eventually spend some debug time trying to figure out why we fail and how to work around it when running uninstalled (it may just be a matter of missing .conf files in /etc causing different default behavior), but it's not my current highest priority. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Nehal J Wani