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

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

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 :|

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