Libguestfs - Oct 2016 - [PATCH] handle: Improve error messaging if XDG_RUNTIME_DIR path does not exist.

If an environment variable such as XDG_RUNTIME_DIR or one of the
tmpdirs or cachedir is set to a non-existent directory, improve the
error message that the user will see so that (where possible) it
includes the environment variable or API call.

This is still not bullet-proof because it's hard to display the
environment variable if it is LIBGUESTFS_TMPDIR or
LIBGUESTFS_CACHEDIR, but the main problem is with XDG_RUNTIME_DIR
(because of systemd bugs).

Thanks: Hilko Bengen for identifying the bug.
---
 src/guestfs-internal.h |  4 ++--
 src/handle.c           |  4 ++--
 src/tmpdirs.c          | 30 ++++++++++++++++++++----------
 3 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 2261f49..f2f2a97 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -803,8 +803,8 @@ extern void guestfs_int_call_callbacks_message (guestfs_h
*g, uint64_t event, co
 extern void guestfs_int_call_callbacks_array (guestfs_h *g, uint64_t event,
const uint64_t *array, size_t array_len);
 
 /* tmpdirs.c */
-extern int guestfs_int_set_env_tmpdir (guestfs_h *g, const char *tmpdir);
-extern int guestfs_int_set_env_runtimedir (guestfs_h *g, const char
*runtimedir);
+extern int guestfs_int_set_env_tmpdir (guestfs_h *g, const char *envname, const
char *tmpdir);
+extern int guestfs_int_set_env_runtimedir (guestfs_h *g, const char *envname,
const char *runtimedir);
 extern int guestfs_int_lazy_make_tmpdir (guestfs_h *g);
 extern int guestfs_int_lazy_make_sockdir (guestfs_h *g);
 extern char *guestfs_int_lazy_make_supermin_appliance_dir (guestfs_h *g);
diff --git a/src/handle.c b/src/handle.c
index b28a1e0..0796015 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -218,7 +218,7 @@ parse_environment (guestfs_h *g,
   }
 
   str = do_getenv (data, "TMPDIR");
-  if (guestfs_int_set_env_tmpdir (g, str) == -1)
+  if (guestfs_int_set_env_tmpdir (g, "TMPDIR", str) == -1)
     return -1;
 
   str = do_getenv (data, "LIBGUESTFS_PATH");
@@ -277,7 +277,7 @@ parse_environment (guestfs_h *g,
   }
 
   str = do_getenv (data, "XDG_RUNTIME_DIR");
-  if (guestfs_int_set_env_runtimedir (g, str) == -1)
+  if (guestfs_int_set_env_runtimedir (g, "XDG_RUNTIME_DIR", str) ==
-1)
     return -1;
 
   return 0;
diff --git a/src/tmpdirs.c b/src/tmpdirs.c
index 6c8fe0d..725f683 100644
--- a/src/tmpdirs.c
+++ b/src/tmpdirs.c
@@ -39,9 +39,14 @@
  * We need to make all tmpdir paths absolute because lots of places in
  * the code assume this.  Do it at the time we set the path or read
  * the environment variable (L<https://bugzilla.redhat.com/882417>).
+ *
+ * The C<ctxstr> parameter is a string displayed in error messages
+ * giving the context of the operation (eg. name of environment
+ * variable being used, or API function being called).
  */
 static int
-set_abs_path (guestfs_h *g, const char *tmpdir, char **tmpdir_ret)
+set_abs_path (guestfs_h *g, const char *ctxstr,
+              const char *tmpdir, char **tmpdir_ret)
 {
   char *ret;
   struct stat statbuf;
@@ -57,17 +62,20 @@ set_abs_path (guestfs_h *g, const char *tmpdir, char
**tmpdir_ret)
 
   ret = realpath (tmpdir, NULL);
   if (ret == NULL) {
-    perrorf (g, _("failed to set temporary directory: %s"), tmpdir);
+    perrorf (g, _("converting path to absolute path: %s: %s:
realpath"),
+             ctxstr, tmpdir);
     return -1;
   }
 
   if (stat (ret, &statbuf) == -1) {
-    perrorf (g, _("failed to set temporary directory: %s"), tmpdir);
+    perrorf (g, "%s: %s: %s: stat",
+             _("setting temporary directory"), ctxstr, tmpdir);
     return -1;
   }
 
   if (!S_ISDIR (statbuf.st_mode)) {
-    error (g, _("temporary directory '%s' is not a
directory"), tmpdir);
+    error (g, _("%s: %s: '%s' is not a directory"),
+           _("setting temporary directory"), ctxstr, tmpdir);
     return -1;
   }
 
@@ -76,21 +84,23 @@ set_abs_path (guestfs_h *g, const char *tmpdir, char
**tmpdir_ret)
 }
 
 int
-guestfs_int_set_env_tmpdir (guestfs_h *g, const char *tmpdir)
+guestfs_int_set_env_tmpdir (guestfs_h *g, const char *envname,
+                            const char *tmpdir)
 {
-  return set_abs_path (g, tmpdir, &g->env_tmpdir);
+  return set_abs_path (g, envname, tmpdir, &g->env_tmpdir);
 }
 
 int
-guestfs_int_set_env_runtimedir (guestfs_h *g, const char *runtimedir)
+guestfs_int_set_env_runtimedir (guestfs_h *g, const char *envname,
+                                const char *runtimedir)
 {
-  return set_abs_path (g, runtimedir, &g->env_runtimedir);
+  return set_abs_path (g, envname, runtimedir, &g->env_runtimedir);
 }
 
 int
 guestfs_impl_set_tmpdir (guestfs_h *g, const char *tmpdir)
 {
-  return set_abs_path (g, tmpdir, &g->int_tmpdir);
+  return set_abs_path (g, "set_tmpdir", tmpdir,
&g->int_tmpdir);
 }
 
 /**
@@ -117,7 +127,7 @@ guestfs_impl_get_tmpdir (guestfs_h *g)
 int
 guestfs_impl_set_cachedir (guestfs_h *g, const char *cachedir)
 {
-  return set_abs_path (g, cachedir, &g->int_cachedir);
+  return set_abs_path (g, "set_cachedir", cachedir,
&g->int_cachedir);
 }
 
 /**
-- 
2.9.3

On Monday, 31 October 2016 08:50:34 CET Richard W.M. Jones
wrote:
> If an environment variable such as XDG_RUNTIME_DIR or one of the
> tmpdirs or cachedir is set to a non-existent directory, improve the
> error message that the user will see so that (where possible) it
> includes the environment variable or API call.
> 
> This is still not bullet-proof because it's hard to display the
> environment variable if it is LIBGUESTFS_TMPDIR or
> LIBGUESTFS_CACHEDIR, but the main problem is with XDG_RUNTIME_DIR
> (because of systemd bugs).
> 
> Thanks: Hilko Bengen for identifying the bug.
> ---
LGTM.

Thanks,
-- 
Pino Toscano