Re: [PATCHv2 1/6] Add utility functions for storing uninstalled location

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]