Libguestfs - May 2012 - [PATCH 0/4] fish: Allow the glob command to expand device patterns (RHBZ#635971).

This patch set fixes a two year old bug in guestfish, namely that the
'glob' command does not expand /dev/* patterns.

https://bugzilla.redhat.com/show_bug.cgi?id=635971

Rich.

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

This commit tidies up the code for the 'glob' command.  It also makes
the command return an error if malloc fails (previously it would just
print a message but not return any error indication).
---
 fish/glob.c |   93 ++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 54 insertions(+), 39 deletions(-)

diff --git a/fish/glob.c b/fish/glob.c
index b3cfa57..88e8927 100644
--- a/fish/glob.c
+++ b/fish/glob.c
@@ -29,6 +29,9 @@
 /* A bit tricky because in the case where there are multiple
  * paths we have to perform a Cartesian product.
  */
+
+static char **expand_pathname (guestfs_h *g, const char *path);
+static char **single_element_list (const char *element);
 static void glob_issue (char *cmd, size_t argc, char ***globs, size_t *posn,
size_t *count, int *r);
 
 int
@@ -69,50 +72,19 @@ run_glob (const char *cmd, size_t argc, char *argv[])
 
     /* Only if it begins with '/' can it possibly be a globbable path.
*/
     if (argv[i][0] == '/') {
-      pp = guestfs_glob_expand (g, argv[i]);
-      if (pp == NULL) {		/* real error in glob_expand */
-        fprintf (stderr, _("glob: guestfs_glob_expand call failed:
%s\n"),
-                 argv[i]);
-        goto error0;
-      }
-
-      /* If there were no matches, then we add a single element list
-       * containing just the original argv[i] string.
-       */
-      if (pp[0] == NULL) {
-        char **pp2;
-
-        pp2 = realloc (pp, sizeof (char *) * 2);
-        if (pp2 == NULL) {
-          perror ("realloc");
-          free (pp);
-          goto error0;
-        }
-        pp = pp2;
-
-        pp[0] = strdup (argv[i]);
-        if (pp[0] == NULL) {
-          perror ("strdup");
-          free (pp);
-          goto error0;
-        }
-        pp[1] = NULL;
+      pp = expand_pathname (g, argv[i]);
+      if (pp == NULL) {
+        r = -1;
+        goto error;
       }
     }
     /* Doesn't begin with '/' */
     else {
-      pp = malloc (sizeof (char *) * 2);
+      pp = single_element_list (argv[i]);
       if (pp == NULL) {
-        perror ("malloc");
-        goto error0;
-      }
-      pp[0] = strdup (argv[i]);
-      if (pp[0] == NULL) {
-        perror ("strdup");
-        free (pp);
-        goto error0;
+        r = -1;
+        goto error;
       }
-      pp[1] = NULL;
     }
 
     globs[i] = pp;
@@ -123,13 +95,56 @@ run_glob (const char *cmd, size_t argc, char *argv[])
   glob_issue (argv[0], argc, globs, posn, count, &r);
 
   /* Free resources. */
- error0:
+ error:
   for (i = 1; i < argc; ++i)
     if (globs[i])
       free_strings (globs[i]);
   return r;
 }
 
+static char **
+expand_pathname (guestfs_h *g, const char *path)
+{
+  char **pp;
+
+  pp = guestfs_glob_expand (g, path);
+  if (pp == NULL) {		/* real error in glob_expand */
+    fprintf (stderr, _("glob: guestfs_glob_expand call failed:
%s\n"), path);
+    return NULL;
+  }
+
+  if (pp[0] != NULL)
+    return pp; /* Return the non-empty list of matches. */
+
+  /* If there were no matches, then we add a single element list
+   * containing just the original string.
+   */
+  free (pp);
+  return single_element_list (path);
+}
+
+/* Return a single element list containing 'element'. */
+static char **
+single_element_list (const char *element)
+{
+  char **pp;
+
+  pp = malloc (sizeof (char *) * 2);
+  if (pp == NULL) {
+    perror ("malloc");
+    return NULL;
+  }
+  pp[0] = strdup (element);
+  if (pp[0] == NULL) {
+    perror ("strdup");
+    free (pp);
+    return NULL;
+  }
+  pp[1] = NULL;
+
+  return pp;
+}
+
 static void
 glob_issue (char *cmd, size_t argc,
             char ***globs, size_t *posn, size_t *count,
-- 
1.7.10

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

This is just code motion.
---
 fish/edit.c |   20 --------------------
 fish/fish.c |   19 +++++++++++++++++++
 fish/fish.h |    1 +
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/fish/edit.c b/fish/edit.c
index a411180..b26a2dd 100644
--- a/fish/edit.c
+++ b/fish/edit.c
@@ -33,7 +33,6 @@
 
 static char *generate_random_name (const char *filename);
 static int copy_attributes (const char *src, const char *dest);
-static int feature_available (guestfs_h *g, const char *feature);
 
 /* guestfish edit command, suggested by J?n Ondrej, implemented by RWMJ */
 
@@ -242,22 +241,3 @@ copy_attributes (const char *src, const char *dest)
 
   return 0;
 }
-
-static int
-feature_available (guestfs_h *g, const char *feature)
-{
-  /* If there's an error we should ignore it, so to do that we have to
-   * temporarily replace the error handler with a null one.
-   */
-  guestfs_error_handler_cb old_error_cb;
-  void *old_error_data;
-  old_error_cb = guestfs_get_error_handler (g, &old_error_data);
-  guestfs_set_error_handler (g, NULL, NULL);
-
-  const char *groups[] = { feature, NULL };
-  int r = guestfs_available (g, (char * const *) groups);
-
-  guestfs_set_error_handler (g, old_error_cb, old_error_data);
-
-  return r == 0 ? 1 : 0;
-}
diff --git a/fish/fish.c b/fish/fish.c
index 63fb30f..80b3364 100644
--- a/fish/fish.c
+++ b/fish/fish.c
@@ -1790,3 +1790,22 @@ progress_callback (guestfs_h *g, void *data,
 
   progress_bar_set (bar, position, total);
 }
+
+int
+feature_available (guestfs_h *g, const char *feature)
+{
+  /* If there's an error we should ignore it, so to do that we have to
+   * temporarily replace the error handler with a null one.
+   */
+  guestfs_error_handler_cb old_error_cb;
+  void *old_error_data;
+  old_error_cb = guestfs_get_error_handler (g, &old_error_data);
+  guestfs_set_error_handler (g, NULL, NULL);
+
+  const char *groups[] = { feature, NULL };
+  int r = guestfs_available (g, (char * const *) groups);
+
+  guestfs_set_error_handler (g, old_error_cb, old_error_data);
+
+  return r == 0 ? 1 : 0;
+}
diff --git a/fish/fish.h b/fish/fish.h
index 69a7640..cc4065a 100644
--- a/fish/fish.h
+++ b/fish/fish.h
@@ -76,6 +76,7 @@ extern void free_file_in (char *s);
 extern char *file_out (const char *arg);
 extern void extended_help_message (void);
 extern void progress_callback (guestfs_h *g, void *data, uint64_t event, int
event_handle, int flags, const char *buf, size_t buf_len, const uint64_t *array,
size_t array_len);
+extern int feature_available (guestfs_h *g, const char *feature);
 
 /* in cmds.c (auto-generated) */
 extern void list_commands (void);
-- 
1.7.10

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

For example:
><fs> glob echo /dev/*
/dev/vda
/dev/vda1
/dev/vda2
/dev/vda3
><fs> glob echo /dev/v*/*
/dev/vg_f16x64/lv_root
/dev/vg_f16x64/lv_swap
---
 fish/glob.c                    |  143 +++++++++++++++++++++++++++++++++++++++-
 generator/generator_actions.ml |    6 +-
 2 files changed, 146 insertions(+), 3 deletions(-)

diff --git a/fish/glob.c b/fish/glob.c
index 88e8927..075f69a 100644
--- a/fish/glob.c
+++ b/fish/glob.c
@@ -22,6 +22,8 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <errno.h>
+#include <fnmatch.h>
 #include <libintl.h>
 
 #include "fish.h"
@@ -31,6 +33,9 @@
  */
 
 static char **expand_pathname (guestfs_h *g, const char *path);
+static char **expand_devicename (guestfs_h *g, const char *device);
+static int add_strings_matching (char **pp, const char *glob, char ***ret,
size_t *size_r);
+static int add_string (const char *str, char ***ret, size_t *size_r);
 static char **single_element_list (const char *element);
 static void glob_issue (char *cmd, size_t argc, char ***globs, size_t *posn,
size_t *count, int *r);
 
@@ -70,8 +75,18 @@ run_glob (const char *cmd, size_t argc, char *argv[])
   for (i = 1; i < argc; ++i) {
     char **pp;
 
-    /* Only if it begins with '/' can it possibly be a globbable path.
*/
-    if (argv[i][0] == '/') {
+    /* If it begins with "/dev/" then treat it as a globbable device
+     * name.
+     */
+    if (STRPREFIX (argv[i], "/dev/")) {
+      pp = expand_devicename (g, argv[i]);
+      if (pp == NULL) {
+        r = -1;
+        goto error;
+      }
+    }
+    /* If it begins with "/" it might be a globbable pathname. */
+    else if (argv[i][0] == '/') {
       pp = expand_pathname (g, argv[i]);
       if (pp == NULL) {
         r = -1;
@@ -123,6 +138,130 @@ expand_pathname (guestfs_h *g, const char *path)
   return single_element_list (path);
 }
 
+/* Glob-expand device patterns, such as "/dev/sd*" (RHBZ#635971).
+ *
+ * There is no 'guestfs_glob_expand_device' function because the
+ * equivalent can be implemented using functions like
+ * 'guestfs_list_devices'.
+ *
+ * It's not immediately clear what it means to expand a pattern like
+ * "/dev/sd*".  Should that include device name translation?  Should
+ * the result include partitions as well as devices?
+ *
+ * Should "/dev/" + "*" return every possible device and
filesystem?
+ * How about VGs?  LVs?
+ *
+ * To solve this what we do is build up a list of every device,
+ * partition, etc., then glob against that list.
+ *
+ * Notes for future work (XXX):
+ * - This doesn't handle device name translation.  It wouldn't be
+ *   too hard to add.
+ * - Could have an API function for returning all device-like things.
+ */
+static char **
+expand_devicename (guestfs_h *g, const char *device)
+{
+  char **pp = NULL;
+  char **ret = NULL;
+  size_t size = 0;
+
+  pp = guestfs_list_devices (g);
+  if (pp == NULL) goto error;
+  if (add_strings_matching (pp, device, &ret, &size) == -1) goto error;
+  free_strings (pp);
+
+  pp = guestfs_list_partitions (g);
+  if (pp == NULL) goto error;
+  if (add_strings_matching (pp, device, &ret, &size) == -1) goto error;
+  free_strings (pp);
+
+  pp = guestfs_list_md_devices (g);
+  if (pp == NULL) goto error;
+  if (add_strings_matching (pp, device, &ret, &size) == -1) goto error;
+  free_strings (pp);
+
+  if (feature_available (g, "lvm2")) {
+    pp = guestfs_lvs (g);
+    if (pp == NULL) goto error;
+    if (add_strings_matching (pp, device, &ret, &size) == -1) goto
error;
+    free_strings (pp);
+    pp = NULL;
+  }
+
+  /* None matched?  Add the original glob pattern. */
+  if (ret == NULL)
+    ret = single_element_list (device);
+  return ret;
+
+ error:
+  if (pp)
+    free_strings (pp);
+  if (ret)
+    free_strings (ret);
+
+  return NULL;
+}
+
+/* Using fnmatch, find strings in the list 'pp' which match pattern
+ * 'glob'.  Add strings which match to the 'ret' array. 
'*size_r' is
+ * the current size of the 'ret' array, which is updated with the new
+ * size.
+ */
+static int
+add_strings_matching (char **pp, const char *glob,
+                      char ***ret, size_t *size_r)
+{
+  size_t i;
+  int r;
+
+  for (i = 0; pp[i] != NULL; ++i) {
+    errno = 0;
+    r = fnmatch (glob, pp[i], FNM_PATHNAME);
+    if (r == 0) {               /* matches - add it */
+      if (add_string (pp[i], ret, size_r) == -1)
+        return -1;
+    }
+    else if (r != FNM_NOMATCH) { /* error */
+      /* I checked the glibc impl and it returns random negative
+       * numbers for errors.  It doesn't always set errno.  Do our
+       * best here to record the error state.
+       */
+      fprintf (stderr, "glob: fnmatch: error (r = %d, errno = %d)\n",
+               r, errno);
+      return -1;
+    }
+  }
+
+  return 0;
+}
+
+static int
+add_string (const char *str, char ***ret, size_t *size_r)
+{
+  char **new_ret = *ret;
+  size_t size = *size_r;
+
+  new_ret = realloc (new_ret, (size + 2) * (sizeof (char *)));
+  if (!new_ret) {
+    perror ("realloc");
+    return -1;
+  }
+  *ret = new_ret;
+
+  new_ret[size] = strdup (str);
+  if (new_ret[size] == NULL) {
+    perror ("strdup");
+    return -1;
+  }
+
+  size++;
+  new_ret[size] = NULL;
+  *size_r = size;
+
+  return 0;
+}
+
 /* Return a single element list containing 'element'. */
 static char **
 single_element_list (const char *element)
diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml
index ace46cb..6948dab 100644
--- a/generator/generator_actions.ml
+++ b/generator/generator_actions.ml
@@ -3700,7 +3700,11 @@ If no paths match, then this returns an empty list
 
 It is just a wrapper around the C L<glob(3)> function
 with flags C<GLOB_MARK|GLOB_BRACE>.
-See that manual page for more details.");
+See that manual page for more details.
+
+Notice that there is no equivalent command for expanding a device
+name (eg. C</dev/sd*>).  Use C<guestfs_list_devices>,
+C<guestfs_list_partitions> etc functions instead.");
 
   ("scrub_device", (RErr, [Device "device"], []), 114,
[Optional "scrub"],
    [InitNone, Always, TestRun (	(* use /dev/sdc because it's smaller *)
-- 
1.7.10

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

---
 fish/Makefile.am  |    2 +
 fish/test-glob.sh |  105 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+)
 create mode 100755 fish/test-glob.sh

diff --git a/fish/Makefile.am b/fish/Makefile.am
index d809ba1..6b52820 100644
--- a/fish/Makefile.am
+++ b/fish/Makefile.am
@@ -248,6 +248,7 @@ TESTS += \
 	test-copy.sh \
 	test-edit.sh \
 	test-find0.sh \
+	test-glob.sh \
 	test-mount-local.sh \
 	test-read_file.sh \
 	test-remote.sh \
@@ -265,6 +266,7 @@ EXTRA_DIST += \
 	test-escapes.sh \
 	test-events.sh \
 	test-find0.sh \
+	test-glob.sh \
 	test-mount-local.sh \
 	test-read_file.sh \
 	test-remote.sh \
diff --git a/fish/test-glob.sh b/fish/test-glob.sh
new file mode 100755
index 0000000..0d54dbe
--- /dev/null
+++ b/fish/test-glob.sh
@@ -0,0 +1,105 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2012 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test guestfish glob command.
+
+set -e
+
+rm -f test.img test.out
+
+./guestfish > test.out <<EOF
+
+sparse test.img 1G
+run
+
+pvcreate /dev/sda
+# Because glob doesn't do device name translation, we cannot test
+# matching on /dev/sd* paths, only on LVs.  So choose a volume group
+# name that cannot possibly be a device name.
+vgcreate abc /dev/sda
+lvcreate lv1 abc 64
+lvcreate lv2 abc 64
+lvcreate lv3 abc 64
+
+glob mkfs ext2 /dev/abc/*
+mount /dev/abc/lv1 /
+
+mkdir /foo
+touch /abc
+touch /foo/bar1
+touch /foo/bar2
+
+# Regular file globbing.
+echo files
+glob echo /f*
+glob echo /foo/*
+glob echo /foo/not*
+glob echo /foo/b??1
+glob echo /abc
+
+# Device globbing.
+echo devices
+glob echo /dev/a*
+glob echo /dev/a*/*
+glob echo /dev/a*/not*
+glob echo /dev/a*/lv?
+glob echo /dev/a*/lv
+glob echo /dev/a*/*3
+glob echo /dev/a*/* /dev/a*
+glob echo /dev/a*/* /dev/a*/*
+
+echo end
+EOF
+
+if [ "$(cat test.out)" != "files
+/foo/
+/foo/bar1
+/foo/bar2
+/foo/not*
+/foo/bar1
+/abc
+devices
+/dev/a*
+/dev/abc/lv1
+/dev/abc/lv2
+/dev/abc/lv3
+/dev/a*/not*
+/dev/abc/lv1
+/dev/abc/lv2
+/dev/abc/lv3
+/dev/a*/lv
+/dev/abc/lv3
+/dev/abc/lv1 /dev/a*
+/dev/abc/lv2 /dev/a*
+/dev/abc/lv3 /dev/a*
+/dev/abc/lv1 /dev/abc/lv1
+/dev/abc/lv1 /dev/abc/lv2
+/dev/abc/lv1 /dev/abc/lv3
+/dev/abc/lv2 /dev/abc/lv1
+/dev/abc/lv2 /dev/abc/lv2
+/dev/abc/lv2 /dev/abc/lv3
+/dev/abc/lv3 /dev/abc/lv1
+/dev/abc/lv3 /dev/abc/lv2
+/dev/abc/lv3 /dev/abc/lv3
+end" ]; then
+    echo "$0: error: unexpected output from glob command"
+    cat test.out
+    exit 1
+fi
+
+rm -f test.img test.out
-- 
1.7.10