Libguestfs - Aug 2009 - [PATCH] Recognise cd-rom devices in devsparts.c

Also:
* Un-duplicate device detection code by creating a common mapping function.
* Add some more comments.
---
 daemon/devsparts.c |  209 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 116 insertions(+), 93 deletions(-)

diff --git a/daemon/devsparts.c b/daemon/devsparts.c
index 33579ba..0e056a1 100644
--- a/daemon/devsparts.c
+++ b/daemon/devsparts.c
@@ -29,57 +29,38 @@
 #include "daemon.h"
 #include "actions.h"
 
+typedef int (*block_dev_func_t)(const char *dev);
+
+static int foreach_block_device (block_dev_func_t func);
+
 char **
 do_list_devices (void)
 {
   char **r = NULL;
   int size = 0, alloc = 0;
-  DIR *dir;
-  struct dirent *d;
-  char buf[256];
-
-  dir = opendir ("/sys/block");
-  if (!dir) {
-    reply_with_perror ("opendir: /sys/block");
-    return NULL;
-  }
 
-  while ((d = readdir (dir)) != NULL) {
-    if (strncmp (d->d_name, "sd", 2) == 0 ||
-	strncmp (d->d_name, "hd", 2) == 0 ||
-	strncmp (d->d_name, "vd", 2) == 0) {
-      snprintf (buf, sizeof buf, "/dev/%s", d->d_name);
+  /* Add a device to the list of devices */
+  int add_to_device_list(const char *device) {
+      char dev_path[256];
+      snprintf (dev_path, sizeof dev_path, "/dev/%s", device);
 
-      /* RHBZ#514505: Some versions of qemu <= 0.10 add a
-       * CD-ROM device even though we didn't request it.  Try to
-       * detect this by seeing if the device contains media.
-       */
-      int fd = open (buf, O_RDONLY);
-      if (fd == -1) {
-	perror (buf);
-	continue;
+      if (add_string (&r, &size, &alloc, dev_path) == -1) {
+        return 0;
       }
-      close (fd);
 
-      if (add_string (&r, &size, &alloc, buf) == -1) {
-	closedir (dir);
-	return NULL;
-      }
-    }
+      return 1;
   }
 
-  if (add_string (&r, &size, &alloc, NULL) == -1) {
-    closedir (dir);
-    return NULL;
-  }
+  if(!foreach_block_device(add_to_device_list)) return NULL;
 
-  if (closedir (dir) == -1) {
-    reply_with_perror ("closedir: /sys/block");
-    free_strings (r);
+  /* Sort the devices */
+  sort_strings (r, size);
+
+  /* NULL terminate the list */
+  if (add_string (&r, &size, &alloc, NULL) == -1) {
     return NULL;
   }
 
-  sort_strings (r, size-1);
   return r;
 }
 
@@ -88,76 +69,63 @@ do_list_partitions (void)
 {
   char **r = NULL;
   int size = 0, alloc = 0;
-  DIR *dir, *dir2;
-  struct dirent *d;
-  char buf[256], devname[256];
 
-  dir = opendir ("/sys/block");
-  if (!dir) {
-    reply_with_perror ("opendir: /sys/block");
-    return NULL;
-  }
+  int find_partitions(const char *device) {
+    struct dirent *d;
+    char devdir[256];
 
-  while ((d = readdir (dir)) != NULL) {
-    if (strncmp (d->d_name, "sd", 2) == 0 ||
-	strncmp (d->d_name, "hd", 2) == 0 ||
-	strncmp (d->d_name, "vd", 2) == 0) {
-      snprintf (buf, sizeof buf, "/dev/%s", d->d_name);
+    /* Open the device's directory under /sys/block */
+    snprintf (devdir, sizeof devdir, "/sys/block/%s", device);
 
-      /* RHBZ#514505: Some versions of qemu <= 0.10 add a
-       * CD-ROM device even though we didn't request it.  Try to
-       * detect this by seeing if the device contains media.
-       */
-      int fd = open (buf, O_RDONLY);
-      if (fd == -1) {
-	perror (buf);
-	continue;
-      }
-      close (fd);
-
-      strncpy (devname, d->d_name, sizeof devname);
-      devname[sizeof devname - 1] = '\0';
-
-      snprintf (buf, sizeof buf, "/sys/block/%s", devname);
+    DIR *dir = opendir (devdir);
+    if (!dir) {
+      reply_with_perror ("opendir: %s", devdir);
+      free_stringslen (r, size);
+      return 0;
+    }
 
-      dir2 = opendir (buf);
-      if (!dir2) {
-	reply_with_perror ("opendir: %s", buf);
-	free_stringslen (r, size);
-	return NULL;
-      }
-      while ((d = readdir (dir2)) != NULL) {
-	if (strncmp (d->d_name, devname, strlen (devname)) == 0) {
-	  snprintf (buf, sizeof buf, "/dev/%s", d->d_name);
-
-	  if (add_string (&r, &size, &alloc, buf) == -1) {
-	    closedir (dir2);
-	    closedir (dir);
-	    return NULL;
-	  }
-	}
+    /* Look in /sys/block/<device>/ for entries starting with
<device>
+     * e.g. /sys/block/sda/sda1
+     */
+    errno = 0;
+    while ((d = readdir (dir)) != NULL) {
+      if (strncmp (d->d_name, device, strlen (device)) == 0) {
+        char part[256];
+        snprintf (part, sizeof part, "/dev/%s", d->d_name);
+
+        if (add_string (&r, &size, &alloc, part) == -1) {
+          closedir (dir);
+          return 0;
+        }
       }
+    }
 
-      if (closedir (dir2) == -1) {
-	reply_with_perror ("closedir: /sys/block/%s", devname);
-	free_stringslen (r, size);
-	return NULL;
-      }
+    /* Check if readdir failed */
+    if(0 != errno) {
+        reply_with_perror ("readdir: %s", devdir);
+        free_stringslen(r, size);
+        return 0;
     }
-  }
 
-  if (add_string (&r, &size, &alloc, NULL) == -1) {
-    closedir (dir);
-    return NULL;
+    if (closedir (dir) == -1) {
+      reply_with_perror ("closedir: /sys/block/%s", device);
+      free_stringslen (r, size);
+      return 0;
+    }
+
+    return 1;
   }
 
-  if (closedir (dir) == -1) {
-    reply_with_perror ("closedir: /sys/block");
-    free_strings (r);
+  if(!foreach_block_device(find_partitions)) return NULL;
+
+  /* Sort the partition list */
+  sort_strings (r, size-1);
+
+  /* NULL-terminate the partition list */
+  if (add_string (&r, &size, &alloc, NULL) == -1) {
     return NULL;
   }
 
-  sort_strings (r, size-1);
   return r;
 }
 
@@ -179,3 +147,58 @@ do_mkfs (char *fstype, char *device)
   free (err);
   return 0;
 }
+
+static int
+foreach_block_device (block_dev_func_t func)
+{
+  DIR *dir;
+  struct dirent *d;
+  int success = 1;
+
+  dir = opendir ("/sys/block");
+  if (!dir) {
+    reply_with_perror ("opendir: /sys/block");
+    return 0;
+  }
+
+  errno = 0;
+  while ((d = readdir (dir)) != NULL) {
+    if (strncmp (d->d_name, "sd", 2) == 0 ||
+        strncmp (d->d_name, "hd", 2) == 0 ||
+        strncmp (d->d_name, "vd", 2) == 0 ||
+        strncmp (d->d_name, "sr", 2) == 0) {
+      char dev_path[256];
+      snprintf (dev_path, sizeof dev_path, "/dev/%s", d->d_name);
+
+      /* RHBZ#514505: Some versions of qemu <= 0.10 add a
+       * CD-ROM device even though we didn't request it.  Try to
+       * detect this by seeing if the device contains media.
+       */
+      int fd = open (dev_path, O_RDONLY);
+      if (fd == -1) {
+        perror (dev_path);
+        continue;
+      }
+      close (fd);
+
+      /* Call the map function for this device */
+      if(!(*func)(d->d_name)) {
+        success = 0;
+        break;
+      }
+    }
+  }
+
+  /* Check readdir didn't fail */
+  if(0 != errno) {
+    reply_with_perror ("readdir: /sys/block");
+    return 0;
+  }
+
+  if (closedir (dir) == -1) {
+    reply_with_perror ("closedir: /sys/block");
+    return 0;
+  }
+
+  return success;
+}
-- 
1.6.2.5

On Mon, Aug 03, 2009 at 04:25:55PM +0100, Matthew Booth
wrote:
> +  /* Add a device to the list of devices */
> +  int add_to_device_list(const char *device) {
> +      char dev_path[256];
> +      snprintf (dev_path, sizeof dev_path, "/dev/%s", device);
So we really are going for nested functions then.

There is something about nested functions which is bad (in the context
of GCC).  I don't recall exactly what it is now.  It's something like
it uses stack trampolines, or breaks something ...

Anyway, in this case:

(1) Indenting is wrong.

(2) The nested function should return 0 / -1 (instead of true/false)
to be consistent with everything else.

Otherwise it looks good.

Please also ensure it doesn't break a complete run of 'make check',
since list-devices/list-partitions are *very* widely used functions,
probably the most important functions after 'add-device' and
'launch'.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 75 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora