Libguestfs - Nov 2011 - [PATCH] NFC: Cleanup iteration over fstab entries in inspect_fs_unix.c

Select non-comment labels using an augeas path to return the correct nodes in
the first instance, rather than applying a regular expression to all results.
Iterate over returned matches using a char** iterator.
Use asprintf() to ensure the path string buffer is the correct size.
---
 src/inspect_fs_unix.c |   50 +++++++++++++++++++++---------------------------
 1 files changed, 22 insertions(+), 28 deletions(-)

diff --git a/src/inspect_fs_unix.c b/src/inspect_fs_unix.c
index 0fa3e83..34d218f 100644
--- a/src/inspect_fs_unix.c
+++ b/src/inspect_fs_unix.c
@@ -110,7 +110,6 @@ compile_regexps (void)
   COMPILE (re_scientific_linux_no_minor,
            "Scientific Linux.*release (\\d+)", 0);
   COMPILE (re_major_minor, "(\\d+)\\.(\\d+)", 0);
-  COMPILE (re_aug_seq, "/\\d+$", 0);
   COMPILE (re_xdev, "^/dev/(h|s|v|xv)d([a-z]+)(\\d*)$", 0);
   COMPILE (re_cciss, "^/dev/(cciss/c\\d+d\\d+)(?:p(\\d+))?$", 0);
   COMPILE (re_mdN, "^(/dev/md\\d+)$", 0);
@@ -132,7 +131,6 @@ free_regexps (void)
   pcre_free (re_scientific_linux);
   pcre_free (re_scientific_linux_no_minor);
   pcre_free (re_major_minor);
-  pcre_free (re_aug_seq);
   pcre_free (re_xdev);
   pcre_free (re_cciss);
   pcre_free (re_mdN);
@@ -725,47 +723,43 @@ check_fstab (guestfs_h *g, struct inspect_fs *fs)
   Hash_table *md_map;
   if (!map_md_devices (g, &md_map)) return -1;
 
-  char **lines = guestfs_aug_ls (g, "/files/etc/fstab");
-  if (lines == NULL) goto error;
+  char *augpath = NULL;
+  char **entries = guestfs_aug_match (g, "/files/etc/fstab/*"
+                                         "[label() !=
'#comment']");
+  if (entries == NULL) goto error;
 
-  if (lines[0] == NULL) {
+  if (entries[0] == NULL) {
     error (g, _("could not parse /etc/fstab or empty file"));
     goto error;
   }
 
-  size_t i;
-  char augpath[256];
-  for (i = 0; lines[i] != NULL; ++i) {
-    /* Ignore comments.  Only care about sequence lines which
-     * match m{/\d+$}.
-     */
-    if (match (g, lines[i], re_aug_seq)) {
-      snprintf (augpath, sizeof augpath, "%s/spec", lines[i]);
-      char *spec = guestfs_aug_get (g, augpath);
-      if (spec == NULL) goto error;
-
-      snprintf (augpath, sizeof augpath, "%s/file", lines[i]);
-      char *mp = guestfs_aug_get (g, augpath);
-      if (mp == NULL) {
-        free (spec);
-        goto error;
-      }
+  for (char **entry = entries; *entry != NULL; entry++) {
+    if (asprintf (&augpath, "%s/spec", *entry) == -1)
g->abort_cb();
+    char *spec = guestfs_aug_get (g, augpath);
+    if (spec == NULL) goto error;
 
-      int r = add_fstab_entry (g, fs, spec, mp, md_map);
+    if (asprintf (&augpath, "%s/file", *entry) == -1)
g->abort_cb();
+    char *mp = guestfs_aug_get (g, augpath);
+    if (mp == NULL) {
       free (spec);
-      free (mp);
-
-      if (r == -1) goto error;
+      goto error;
     }
+
+    int r = add_fstab_entry (g, fs, spec, mp, md_map);
+    free (spec);
+    free (mp);
+
+    if (r == -1) goto error;
   }
 
   hash_free (md_map);
-  guestfs___free_string_list (lines);
+  guestfs___free_string_list (entries);
   return 0;
 
 error:
   hash_free (md_map);
-  if (lines) guestfs___free_string_list (lines);
+  if (entries) guestfs___free_string_list (entries);
+  free(augpath);
   return -1;
 }
 
-- 
1.7.7.3

On Thu, Nov 24, 2011 at 02:48:46PM +0000, Matthew Booth
wrote:
> Select non-comment labels using an augeas path to return the correct nodes
in
> the first instance, rather than applying a regular expression to all
results.
> Iterate over returned matches using a char** iterator.
> Use asprintf() to ensure the path string buffer is the correct size.
In general this is an obvious improvement.
> +  for (char **entry = entries; *entry != NULL; entry++) {
> +    if (asprintf (&augpath, "%s/spec", *entry) == -1)
g->abort_cb();
We have a safe_asprintf function defined already, and it would be
better to use that.

Is augpath supposed to be freed on the non-error path?

spec and mp should be freed on the error path, but don't seem to be at
the moment.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

Select non-comment labels using an augeas path to return the correct nodes in
the first instance, rather than applying a regular expression to all results.
Iterate over returned matches using a char** iterator.
Use asprintf() to ensure the path string buffer is the correct size.
---
 src/inspect_fs_unix.c |   53 +++++++++++++++++++++++-------------------------
 1 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/src/inspect_fs_unix.c b/src/inspect_fs_unix.c
index 0fa3e83..27a27e5 100644
--- a/src/inspect_fs_unix.c
+++ b/src/inspect_fs_unix.c
@@ -110,7 +110,6 @@ compile_regexps (void)
   COMPILE (re_scientific_linux_no_minor,
            "Scientific Linux.*release (\\d+)", 0);
   COMPILE (re_major_minor, "(\\d+)\\.(\\d+)", 0);
-  COMPILE (re_aug_seq, "/\\d+$", 0);
   COMPILE (re_xdev, "^/dev/(h|s|v|xv)d([a-z]+)(\\d*)$", 0);
   COMPILE (re_cciss, "^/dev/(cciss/c\\d+d\\d+)(?:p(\\d+))?$", 0);
   COMPILE (re_mdN, "^(/dev/md\\d+)$", 0);
@@ -132,7 +131,6 @@ free_regexps (void)
   pcre_free (re_scientific_linux);
   pcre_free (re_scientific_linux_no_minor);
   pcre_free (re_major_minor);
-  pcre_free (re_aug_seq);
   pcre_free (re_xdev);
   pcre_free (re_cciss);
   pcre_free (re_mdN);
@@ -725,47 +723,46 @@ check_fstab (guestfs_h *g, struct inspect_fs *fs)
   Hash_table *md_map;
   if (!map_md_devices (g, &md_map)) return -1;
 
-  char **lines = guestfs_aug_ls (g, "/files/etc/fstab");
-  if (lines == NULL) goto error;
+  char **entries = guestfs_aug_match (g, "/files/etc/fstab/*"
+                                         "[label() !=
'#comment']");
+  if (entries == NULL) goto error;
 
-  if (lines[0] == NULL) {
+  if (entries[0] == NULL) {
     error (g, _("could not parse /etc/fstab or empty file"));
     goto error;
   }
 
-  size_t i;
   char augpath[256];
-  for (i = 0; lines[i] != NULL; ++i) {
-    /* Ignore comments.  Only care about sequence lines which
-     * match m{/\d+$}.
-     */
-    if (match (g, lines[i], re_aug_seq)) {
-      snprintf (augpath, sizeof augpath, "%s/spec", lines[i]);
-      char *spec = guestfs_aug_get (g, augpath);
-      if (spec == NULL) goto error;
-
-      snprintf (augpath, sizeof augpath, "%s/file", lines[i]);
-      char *mp = guestfs_aug_get (g, augpath);
-      if (mp == NULL) {
-        free (spec);
-        goto error;
-      }
-
-      int r = add_fstab_entry (g, fs, spec, mp, md_map);
+  for (char **entry = entries; *entry != NULL; entry++) {
+    int len;
+    
+    len = snprintf(augpath, sizeof(augpath), "%s/spec", *entry);
+    if (len < 0 || (size_t)len >= sizeof(augpath)) g->abort_cb();
+    char *spec = guestfs_aug_get (g, augpath);
+    if (spec == NULL) goto error;
+
+    len = snprintf(augpath, sizeof(augpath), "%s/file", *entry);
+    if (len < 0 || (size_t)len >= sizeof(augpath)) g->abort_cb();
+    char *mp = guestfs_aug_get (g, augpath);
+    if (mp == NULL) {
       free (spec);
-      free (mp);
-
-      if (r == -1) goto error;
+      goto error;
     }
+
+    int r = add_fstab_entry (g, fs, spec, mp, md_map);
+    free (spec);
+    free (mp);
+
+    if (r == -1) goto error;
   }
 
   hash_free (md_map);
-  guestfs___free_string_list (lines);
+  guestfs___free_string_list (entries);
   return 0;
 
 error:
   hash_free (md_map);
-  if (lines) guestfs___free_string_list (lines);
+  if (entries) guestfs___free_string_list (entries);
   return -1;
 }
 
-- 
1.7.7.3