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

[Libguestfs] [PATCH] Clarify the error message when unavailable functions are called (RHBZ#679737).



From: "Richard W.M. Jones" <rjones redhat com>

Callers are supposed to use the availability API to check for
functions that may not be available in particular builds of
libguestfs.  If they don't do this, currently they tend to get obscure
error messages, eg:

  libguestfs: error: zerofree: /dev/vda1: zerofree: No such file or directory

This commit changes the error message to explain what callers ought to
be doing instead:

  libguestfs: error: zerofree: feature 'zerofree' is not available in this
  build of libguestfs.  Read 'AVAILABILITY' in the guestfs(3) man page for
  how to check for the availability of features.

This patch makes the stubs check for availability.  The stub code
changes to:

  static void
  zerofree_stub (XDR *xdr_in)
  {
  [...]

    /* The caller should have checked before calling this. */
    if (! optgroup_zerofree_available ()) {
      reply_with_error ("feature '%s' is not available in this\n"
                        "build of libguestfs.  Read 'AVAILABILITY' in the guestfs(3) man page for\n"
                        "how to check for the availability of features.",
                        "zerofree");
      goto done;
    }
  [...]
---
 daemon/augeas.c               |  158 +++++++++++++++++++++++++++--------------
 daemon/daemon.h               |   10 ---
 daemon/inotify.c              |   81 +++++++++++++--------
 daemon/mknod.c                |   45 +++++++++---
 daemon/realpath.c             |   26 +++++--
 daemon/selinux.c              |   40 ++++++++---
 daemon/xattr.c                |   23 ++++---
 generator/generator_daemon.ml |   24 ++++++-
 8 files changed, 271 insertions(+), 136 deletions(-)

diff --git a/daemon/augeas.c b/daemon/augeas.c
index f52c091..4a09f57 100644
--- a/daemon/augeas.c
+++ b/daemon/augeas.c
@@ -63,19 +63,11 @@ optgroup_augeas_available (void)
 {
   return 1;
 }
-#else /* !HAVE_AUGEAS */
-int
-optgroup_augeas_available (void)
-{
-  return 0;
-}
-#endif
 
 /* We need to rewrite the root path so it is based at /sysroot. */
 int
 do_aug_init (const char *root, int flags)
 {
-#ifdef HAVE_AUGEAS
   char *buf;
 
   if (aug) {
@@ -98,24 +90,17 @@ do_aug_init (const char *root, int flags)
   }
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
 do_aug_close (void)
 {
-#ifdef HAVE_AUGEAS
   NEED_AUG(-1);
 
   aug_close (aug);
   aug = NULL;
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
@@ -133,7 +118,8 @@ do_aug_defvar (const char *name, const char *expr)
   }
   return r;
 #else
-  NOT_AVAILABLE (-1);
+  reply_with_error ("function not available");
+  return -1;
 #endif
 }
 
@@ -163,14 +149,14 @@ do_aug_defnode (const char *name, const char *expr, const char *val)
 
   return r;
 #else
-  NOT_AVAILABLE (NULL);
+  reply_with_error ("function not available");
+  return NULL;
 #endif
 }
 
 char *
 do_aug_get (const char *path)
 {
-#ifdef HAVE_AUGEAS
   const char *value = NULL;
   char *v;
   int r;
@@ -204,15 +190,11 @@ do_aug_get (const char *path)
   }
 
   return v;			/* Caller frees. */
-#else
-  NOT_AVAILABLE (NULL);
-#endif
 }
 
 int
 do_aug_set (const char *path, const char *val)
 {
-#ifdef HAVE_AUGEAS
   int r;
 
   NEED_AUG (-1);
@@ -224,15 +206,11 @@ do_aug_set (const char *path, const char *val)
   }
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
 do_aug_clear (const char *path)
 {
-#ifdef HAVE_AUGEAS
   int r;
 
   NEED_AUG (-1);
@@ -244,15 +222,11 @@ do_aug_clear (const char *path)
   }
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
 do_aug_insert (const char *path, const char *label, int before)
 {
-#ifdef HAVE_AUGEAS
   int r;
 
   NEED_AUG (-1);
@@ -264,15 +238,11 @@ do_aug_insert (const char *path, const char *label, int before)
   }
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
 do_aug_rm (const char *path)
 {
-#ifdef HAVE_AUGEAS
   int r;
 
   NEED_AUG (-1);
@@ -284,15 +254,11 @@ do_aug_rm (const char *path)
   }
 
   return r;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
 do_aug_mv (const char *src, const char *dest)
 {
-#ifdef HAVE_AUGEAS
   int r;
 
   NEED_AUG (-1);
@@ -304,15 +270,11 @@ do_aug_mv (const char *src, const char *dest)
   }
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 char **
 do_aug_match (const char *path)
 {
-#ifdef HAVE_AUGEAS
   char **matches = NULL;
   void *vp;
   int r;
@@ -338,15 +300,11 @@ do_aug_match (const char *path)
   matches[r] = NULL;
 
   return matches;		/* Caller frees. */
-#else
-  NOT_AVAILABLE (NULL);
-#endif
 }
 
 int
 do_aug_save (void)
 {
-#ifdef HAVE_AUGEAS
   NEED_AUG (-1);
 
   if (aug_save (aug) == -1) {
@@ -355,9 +313,6 @@ do_aug_save (void)
   }
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
@@ -373,7 +328,8 @@ do_aug_load (void)
 
   return 0;
 #else
-  NOT_AVAILABLE (-1);
+  reply_with_error ("function not available");
+  return -1;
 #endif
 }
 
@@ -381,7 +337,6 @@ do_aug_load (void)
 char **
 do_aug_ls (const char *path)
 {
-#ifdef HAVE_AUGEAS
   char **matches;
   char *buf;
   int len;
@@ -420,7 +375,102 @@ do_aug_ls (const char *path)
 
   sort_strings (matches, count_strings ((void *) matches));
   return matches;		/* Caller frees. */
-#else
-  NOT_AVAILABLE (NULL);
-#endif
 }
+
+#else /* !HAVE_AUGEAS */
+
+/* Note that the wrapper code (daemon/stubs.c) ensures that the
+ * functions below are never called because optgroup_augeas_available
+ * returns false.
+ */
+int
+optgroup_augeas_available (void)
+{
+  return 0;
+}
+
+int
+do_aug_init (const char *root, int flags)
+{
+  abort ();
+}
+
+int
+do_aug_close (void)
+{
+  abort ();
+}
+
+int
+do_aug_defvar (const char *name, const char *expr)
+{
+  abort ();
+}
+
+guestfs_int_int_bool *
+do_aug_defnode (const char *name, const char *expr, const char *val)
+{
+  abort ();
+}
+
+char *
+do_aug_get (const char *path)
+{
+  abort ();
+}
+
+int
+do_aug_set (const char *path, const char *val)
+{
+  abort ();
+}
+
+int
+do_aug_clear (const char *path)
+{
+  abort ();
+}
+
+int
+do_aug_insert (const char *path, const char *label, int before)
+{
+  abort ();
+}
+
+int
+do_aug_rm (const char *path)
+{
+  abort ();
+}
+
+int
+do_aug_mv (const char *src, const char *dest)
+{
+  abort ();
+}
+
+char **
+do_aug_match (const char *path)
+{
+  abort ();
+}
+
+int
+do_aug_save (void)
+{
+  abort ();
+}
+
+int
+do_aug_load (void)
+{
+  abort ();
+}
+
+char **
+do_aug_ls (const char *path)
+{
+  abort ();
+}
+
+#endif
diff --git a/daemon/daemon.h b/daemon/daemon.h
index babe5bc..b518f92 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -331,16 +331,6 @@ is_zero (const char *buffer, size_t size)
   }									\
   while (0)
 
-/* Marks functions which are not available.
- * NB. Cannot be used for FileIn functions.
- */
-#define NOT_AVAILABLE(errcode)                                          \
-  do {									\
-    reply_with_error ("%s: function not available", __func__);          \
-    return (errcode);							\
-  }									\
-  while (0)
-
 #ifndef __attribute__
 # if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 8)
 #  define __attribute__(x) /* empty */
diff --git a/daemon/inotify.c b/daemon/inotify.c
index add1f14..df6b2e8 100644
--- a/daemon/inotify.c
+++ b/daemon/inotify.c
@@ -56,13 +56,6 @@ optgroup_inotify_available (void)
 {
   return 1;
 }
-#else /* !HAVE_SYS_INOTIFY_H */
-int
-optgroup_inotify_available (void)
-{
-  return 0;
-}
-#endif
 
 /* Because inotify_init does NEED_ROOT, NEED_INOTIFY implies NEED_ROOT. */
 #define NEED_INOTIFY(errcode)						\
@@ -78,7 +71,6 @@ optgroup_inotify_available (void)
 int
 do_inotify_init (int max_events)
 {
-#ifdef HAVE_SYS_INOTIFY_H
   FILE *fp;
 
   NEED_ROOT (, return -1);
@@ -129,15 +121,11 @@ do_inotify_init (int max_events)
 #endif
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
 do_inotify_close (void)
 {
-#ifdef HAVE_SYS_INOTIFY_H
   NEED_INOTIFY (-1);
 
   if (inotify_fd == -1) {
@@ -154,15 +142,11 @@ do_inotify_close (void)
   inotify_posn = 0;
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int64_t
 do_inotify_add_watch (const char *path, int mask)
 {
-#ifdef HAVE_SYS_INOTIFY_H
   int64_t r;
   char *buf;
 
@@ -182,15 +166,11 @@ do_inotify_add_watch (const char *path, int mask)
   }
 
   return r;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
 do_inotify_rm_watch (int wd)
 {
-#ifdef HAVE_SYS_INOTIFY_H
   NEED_INOTIFY (-1);
 
   if (inotify_rm_watch (inotify_fd, wd) == -1) {
@@ -199,15 +179,11 @@ do_inotify_rm_watch (int wd)
   }
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 guestfs_int_inotify_event_list *
 do_inotify_read (void)
 {
-#ifdef HAVE_SYS_INOTIFY_H
   int space;
   guestfs_int_inotify_event_list *ret;
 
@@ -313,15 +289,11 @@ do_inotify_read (void)
   xdr_free ((xdrproc_t) xdr_guestfs_int_inotify_event_list, (char *) ret);
   free (ret);
   return NULL;
-#else
-  NOT_AVAILABLE (NULL);
-#endif
 }
 
 char **
 do_inotify_files (void)
 {
-#ifdef HAVE_SYS_INOTIFY_H
   char **ret = NULL;
   int size = 0, alloc = 0;
   unsigned int i;
@@ -404,7 +376,54 @@ do_inotify_files (void)
 
   unlink (tempfile);
   return NULL;
-#else
-  NOT_AVAILABLE (NULL);
-#endif
 }
+
+#else /* !HAVE_SYS_INOTIFY_H */
+
+/* Note that the wrapper code (daemon/stubs.c) ensures that the
+ * functions below are never called because optgroup_inotify_available
+ * returns false.
+ */
+int
+optgroup_inotify_available (void)
+{
+  return 0;
+}
+
+int
+do_inotify_init (int max_events)
+{
+  abort ();
+}
+
+int
+do_inotify_close (void)
+{
+  abort ();
+}
+
+int64_t
+do_inotify_add_watch (const char *path, int mask)
+{
+  abort ();
+}
+
+int
+do_inotify_rm_watch (int wd)
+{
+  abort ();
+}
+
+guestfs_int_inotify_event_list *
+do_inotify_read (void)
+{
+  abort ();
+}
+
+char **
+do_inotify_files (void)
+{
+  abort ();
+}
+
+#endif
diff --git a/daemon/mknod.c b/daemon/mknod.c
index d5b8467..6e678fd 100644
--- a/daemon/mknod.c
+++ b/daemon/mknod.c
@@ -37,18 +37,10 @@ optgroup_mknod_available (void)
 {
   return 1;
 }
-#else
-int
-optgroup_mknod_available (void)
-{
-  return 0;
-}
-#endif
 
 int
 do_mknod (int mode, int devmajor, int devminor, const char *path)
 {
-#ifdef HAVE_MKNOD
   int r;
 
   if (mode < 0) {
@@ -66,9 +58,6 @@ do_mknod (int mode, int devmajor, int devminor, const char *path)
   }
 
   return 0;
-#else
-  NOT_AVAILABLE (-1);
-#endif
 }
 
 int
@@ -88,3 +77,37 @@ do_mknod_c (int mode, int devmajor, int devminor, const char *path)
 {
   return do_mknod (mode | S_IFCHR, devmajor, devminor, path);
 }
+
+#else
+
+int
+optgroup_mknod_available (void)
+{
+  return 0;
+}
+
+int
+do_mknod (int mode, int devmajor, int devminor, const char *path)
+{
+  abort ();
+}
+
+int
+do_mkfifo (int mode, const char *path)
+{
+  abort ();
+}
+
+int
+do_mknod_b (int mode, int devmajor, int devminor, const char *path)
+{
+  abort ();
+}
+
+int
+do_mknod_c (int mode, int devmajor, int devminor, const char *path)
+{
+  abort ();
+}
+
+#endif
diff --git a/daemon/realpath.c b/daemon/realpath.c
index 8ec9674..126ef19 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -36,20 +36,17 @@
 #define NAME_MAX FILENAME_MAX
 #endif
 
+#ifdef HAVE_REALPATH
+
 int
 optgroup_realpath_available (void)
 {
-#ifdef HAVE_REALPATH
   return 1;
-#else
-  return 0;
-#endif
 }
 
 char *
 do_realpath (const char *path)
 {
-#ifdef HAVE_REALPATH
   char *ret;
 
   CHROOT_IN;
@@ -61,11 +58,24 @@ do_realpath (const char *path)
   }
 
   return ret;			/* caller frees */
-#else
-  NOT_AVAILABLE (NULL);
-#endif
 }
 
+#else /* !HAVE_REALPATH */
+
+int
+optgroup_realpath_available (void)
+{
+  return 0;
+}
+
+char *
+do_realpath (const char *path)
+{
+  abort ();
+}
+
+#endif /* !HAVE_REALPATH */
+
 static int find_path_element (int fd_cwd, char *name, size_t *name_len_ret);
 
 char *
diff --git a/daemon/selinux.c b/daemon/selinux.c
index 2db05ee..40590e1 100644
--- a/daemon/selinux.c
+++ b/daemon/selinux.c
@@ -32,18 +32,12 @@
 #include "optgroups.h"
 
 #if defined(HAVE_LIBSELINUX)
+
 int
 optgroup_selinux_available (void)
 {
   return 1;
 }
-#else /* !HAVE_LIBSELINUX */
-int
-optgroup_selinux_available (void)
-{
-  return 0;
-}
-#endif /* !HAVE_LIBSELINUX */
 
 /* setcon is only valid under the following circumstances:
  * - single threaded
@@ -52,7 +46,7 @@ optgroup_selinux_available (void)
 int
 do_setcon (const char *context)
 {
-#if defined(HAVE_LIBSELINUX) && defined(HAVE_SETCON)
+#if defined(HAVE_SETCON)
   if (setcon ((char *) context) == -1) {
     reply_with_perror ("setcon");
     return -1;
@@ -60,14 +54,15 @@ do_setcon (const char *context)
 
   return 0;
 #else
-  NOT_AVAILABLE (-1);
+  reply_with_error ("function not available");
+  return -1;
 #endif
 }
 
 char *
 do_getcon (void)
 {
-#if defined(HAVE_LIBSELINUX) && defined(HAVE_GETCON)
+#if defined(HAVE_GETCON)
   security_context_t context;
   char *r;
 
@@ -85,6 +80,29 @@ do_getcon (void)
 
   return r;                     /* caller frees */
 #else
-  NOT_AVAILABLE (NULL);
+  reply_with_error ("function not available");
+  return NULL;
 #endif
 }
+
+#else /* !HAVE_LIBSELINUX */
+
+int
+optgroup_selinux_available (void)
+{
+  return 0;
+}
+
+int
+do_setcon (const char *context)
+{
+  abort ();
+}
+
+char *
+do_getcon (void)
+{
+  abort ();
+}
+
+#endif /* !HAVE_LIBSELINUX */
diff --git a/daemon/xattr.c b/daemon/xattr.c
index 2445748..92d0cf1 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -511,6 +511,11 @@ do_lgetxattr (const char *path, const char *name, size_t *size_r)
 }
 
 #else /* no xattr.h */
+
+/* Note that the wrapper code (daemon/stubs.c) ensures that the
+ * functions below are never called because
+ * optgroup_linuxxattrs_available returns false.
+ */
 int
 optgroup_linuxxattrs_available (void)
 {
@@ -520,55 +525,55 @@ optgroup_linuxxattrs_available (void)
 guestfs_int_xattr_list *
 do_getxattrs (const char *path)
 {
-  NOT_AVAILABLE (NULL);
+  abort ();
 }
 
 guestfs_int_xattr_list *
 do_lgetxattrs (const char *path)
 {
-  NOT_AVAILABLE (NULL);
+  abort ();
 }
 
 int
 do_setxattr (const char *xattr, const char *val, int vallen, const char *path)
 {
-  NOT_AVAILABLE (-1);
+  abort ();
 }
 
 int
 do_lsetxattr (const char *xattr, const char *val, int vallen, const char *path)
 {
-  NOT_AVAILABLE (-1);
+  abort ();
 }
 
 int
 do_removexattr (const char *xattr, const char *path)
 {
-  NOT_AVAILABLE (-1);
+  abort ();
 }
 
 int
 do_lremovexattr (const char *xattr, const char *path)
 {
-  NOT_AVAILABLE (-1);
+  abort ();
 }
 
 guestfs_int_xattr_list *
 do_lxattrlist (const char *path, char *const *names)
 {
-  NOT_AVAILABLE (NULL);
+  abort ();
 }
 
 char *
 do_getxattr (const char *path, const char *name, size_t *size_r)
 {
-  NOT_AVAILABLE (NULL);
+  abort ();
 }
 
 char *
 do_lgetxattr (const char *path, const char *name, size_t *size_r)
 {
-  NOT_AVAILABLE (NULL);
+  abort ();
 }
 
 #endif /* no xattr.h */
diff --git a/generator/generator_daemon.ml b/generator/generator_daemon.ml
index 88cc8bf..0eb2446 100644
--- a/generator/generator_daemon.ml
+++ b/generator/generator_daemon.ml
@@ -75,12 +75,14 @@ and generate_daemon_actions () =
   pr "#include \"c-ctype.h\"\n";
   pr "#include \"guestfs_protocol.h\"\n";
   pr "#include \"actions.h\"\n";
+  pr "#include \"optgroups.h\"\n";
   pr "\n";
 
   List.iter (
-    fun (name, (ret, args, optargs), _, _, _, _, _) ->
+    fun (name, (ret, args, optargs), _, flags, _, _, _) ->
       (* Generate server-side stubs. *)
-      pr "static void %s_stub (XDR *xdr_in)\n" name;
+      pr "static void\n";
+      pr "%s_stub (XDR *xdr_in)\n" name;
       pr "{\n";
       (match ret with
        | RErr | RInt _ -> pr "  int r;\n"
@@ -122,6 +124,24 @@ and generate_daemon_actions () =
       let is_filein =
         List.exists (function FileIn _ -> true | _ -> false) args in
 
+      (* Reject Optional functions that are not available (RHBZ#679737). *)
+      List.iter (
+        function
+        | Optional group ->
+          pr "  /* The caller should have checked before calling this. */\n";
+          pr "  if (! optgroup_%s_available ()) {\n" group;
+          if is_filein then
+            pr "    cancel_receive ();\n";
+          pr "    reply_with_error (\"feature '%%s' is not available in this\\n\"\n";
+          pr "                      \"build of libguestfs.  Read 'AVAILABILITY' in the guestfs(3) man page for\\n\"\n";
+          pr "                      \"how to check for the availability of features.\",\n";
+          pr "                      \"%s\");\n" group;
+          pr "    goto done;\n";
+          pr "  }\n";
+          pr "\n"
+        | _ -> ()
+      ) flags;
+
       (* Reject unknown optional arguments.
        * Note this code is included even for calls with no optional
        * args because the caller must not pass optargs_bitmask != 0
-- 
1.7.6


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