[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
On Mon, Aug 03, 2009 at 05:01:25PM +0100, Richard W.M. Jones wrote:
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 ...
This one maybe: http://www.gentoo.org/proj/en/hardened/gnu-stack.xml Gentoo reckon it's a security issue. 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
To see if a binary is marked with an executable stack, do: readelf -l binary | grep GNU_STACK and look for 'RWE'. RWE = executable stack (bad) RW = not executable stack (good) eg: $ readelf -l /bin/ls | grep GNU_STACK GNU_STACK 0x000000 0x00000000 0x00000000 0x00000 0x00000 RW 0x4 (You can only do this on the final binary, not on object files) Note that even a single object file in the entire binary having an executable stack means the whole binary will have an executable stack. So we have to care about 'purity' of every piece of code. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
I applied the patch and built guestfish and the test programs again. It _doesn't_ make the stack executable. So I'm OK with this patch, _except_ for the two problems that I noted previously which need to be fixed. 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
I've reworked this patch to: * Not use nested functions * Fix indentation * Use -1/0 instead of 0/1 for return values * Make the diff a little nicer by moving things around make check passes with the exception of test_upload and test_download. I've confirmed that these also fail without this patch (RHBZ#515764).
Also: * Un-duplicate device detection code by creating a common mapping function. * Add some more comments. --- daemon/devsparts.c | 168 ++++++++++++++++++++++++++++++---------------------- 1 files changed, 96 insertions(+), 72 deletions(-) diff --git a/daemon/devsparts.c b/daemon/devsparts.c index 1970e7d..b89682c 100644 --- a/daemon/devsparts.c +++ b/daemon/devsparts.c @@ -29,14 +29,19 @@ #include "daemon.h" #include "actions.h" -char ** -do_list_devices (void) +typedef int (*block_dev_func_t)(const char *dev, + char ***r, int *size, int *alloc); + +/* Execute a given function for each discovered block device */ +static char** +foreach_block_device (block_dev_func_t func) { char **r = NULL; int size = 0, alloc = 0; + DIR *dir; struct dirent *d; - char buf[256]; + int err = 0; dir = opendir ("/sys/block"); if (!dir) { @@ -44,121 +49,140 @@ do_list_devices (void) return NULL; } + 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) { - snprintf (buf, sizeof buf, "/dev/%s", d->d_name); + 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 (buf, O_RDONLY); + int fd = open (dev_path, O_RDONLY); if (fd == -1) { - perror (buf); + perror (dev_path); continue; } close (fd); - if (add_string (&r, &size, &alloc, buf) == -1) { - closedir (dir); - return NULL; + /* Call the map function for this device */ + if((*func)(d->d_name, &r, &size, &alloc) != 0) { + err = 1; + break; } } } - if (add_string (&r, &size, &alloc, NULL) == -1) { - closedir (dir); + /* Check readdir didn't fail */ + if(0 != errno) { + reply_with_perror ("readdir: /sys/block"); + free_stringslen(r, size); return NULL; } + /* Close the directory handle */ if (closedir (dir) == -1) { reply_with_perror ("closedir: /sys/block"); - free_strings (r); + free_stringslen(r, size); + return NULL; + } + + /* Free the result list on error */ + if(err) { + free_stringslen(r, size); + return NULL; + } + + /* 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; } -char ** -do_list_partitions (void) +/* Add a device to the list of devices */ +static int +add_device(const char *device, + char ***const r, int *const size, int *const alloc) { - char **r = NULL; - int size = 0, alloc = 0; - DIR *dir, *dir2; - struct dirent *d; - char buf[256], devname[256]; + char dev_path[256]; + snprintf (dev_path, sizeof dev_path, "/dev/%s", device); - dir = opendir ("/sys/block"); - if (!dir) { - reply_with_perror ("opendir: /sys/block"); - return NULL; + if (add_string (r, size, alloc, dev_path) == -1) { + return -1; } - 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); + return 0; +} - /* 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); +char ** +do_list_devices (void) +{ + return foreach_block_device(add_device); +} - strncpy (devname, d->d_name, sizeof devname); - devname[sizeof devname - 1] = '\0'; +static int +add_partitions(const char *device, + char ***const r, int *const size, int *const alloc) +{ + char devdir[256]; - snprintf (buf, sizeof buf, "/sys/block/%s", devname); + /* Open the device's directory under /sys/block */ + snprintf (devdir, sizeof devdir, "/sys/block/%s", device); - 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; - } - } - } + DIR *dir = opendir (devdir); + if (!dir) { + reply_with_perror ("opendir: %s", devdir); + free_stringslen (*r, *size); + return -1; + } - if (closedir (dir2) == -1) { - reply_with_perror ("closedir: /sys/block/%s", devname); - free_stringslen (r, size); - return NULL; + /* Look in /sys/block/<device>/ for entries starting with <device> + * e.g. /sys/block/sda/sda1 + */ + errno = 0; + struct dirent *d; + 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 -1; } } } - if (add_string (&r, &size, &alloc, NULL) == -1) { - closedir (dir); - return NULL; + /* Check if readdir failed */ + if(0 != errno) { + reply_with_perror ("readdir: %s", devdir); + free_stringslen(*r, *size); + return -1; } + /* Close the directory handle */ if (closedir (dir) == -1) { - reply_with_perror ("closedir: /sys/block"); - free_strings (r); - return NULL; + reply_with_perror ("closedir: /sys/block/%s", device); + free_stringslen (*r, *size); + return -1; } - sort_strings (r, size-1); - return r; + return 0; +} + +char ** +do_list_partitions (void) +{ + return foreach_block_device(add_partitions); } int -- 1.6.2.5
Matthew Booth wrote:
Also: * Un-duplicate device detection code by creating a common mapping function. * Add some more comments. --- daemon/devsparts.c | 168 ++++++++++++++++++++++++++++++---------------------- 1 files changed, 96 insertions(+), 72 deletions(-)
diff --git a/daemon/devsparts.c b/daemon/devsparts.c index 1970e7d..b89682c 100644 --- a/daemon/devsparts.c +++ b/daemon/devsparts.c @@ -29,14 +29,19 @@ #include "daemon.h" #include "actions.h"
-char ** -do_list_devices (void) +typedef int (*block_dev_func_t)(const char *dev, + char ***r, int *size, int *alloc); + +/* Execute a given function for each discovered block device */ +static char** +foreach_block_device (block_dev_func_t func) { char **r = NULL; int size = 0, alloc = 0; + DIR *dir; struct dirent *d; - char buf[256]; + int err = 0;
dir = opendir ("/sys/block"); if (!dir) { @@ -44,121 +49,140 @@ do_list_devices (void) return NULL; }
+ errno = 0; while ((d = readdir (dir)) != NULL) { ... }
- if (add_string (&r, &size, &alloc, NULL) == -1) { - closedir (dir); + /* Check readdir didn't fail */ + if(0 != errno) { + reply_with_perror ("readdir: /sys/block"); + free_stringslen(r, size); return NULL; }
It's nice to detect a failing readdir. However, that errno=0 assignment belongs inside the loop. A minimal change would produce this ugliness: while ((errno = 0, d = readdir (dir)) != NULL) { ... } This is more readable (also moving the decl of "d" into the loop): while (true) { errno = 0; struct dirent *d = readdir (dir); if (d != NULL) break; ... }
On Wed, Aug 05, 2009 at 05:49:39PM +0100, Matthew Booth wrote:
Also: * Un-duplicate device detection code by creating a common mapping function. * Add some more comments.
Yes, I checked this patch yesterday, so ACK. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
On Mon, Aug 03, 2009 at 05:01:25PM +0100, Richard W.M. Jones wrote:
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 ...
This has some details http://gcc.gnu.org/ml/gcc-help/2008-11/msg00049.html taking the address of a function causes your executable to get an executable stack, which is a 'bad thing(tm)'. If you merely call the function directly you'll seem to be safe Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Daniel P. Berrange -
Jim Meyering -
Matthew Booth -
Richard W.M. Jones