[PATCH v2 0/2] error accessing harddrive partitions
virt-v2v fails when traverses each partition: inspect_sources.ml: ... and is_uefi_bootable_part part = let dev = g#part_to_dev part in parttype_is_gpt dev && is_uefi_ESP dev part in let partitions = Array.to_list (g#list_partitions ()) in let partitions = List.filter is_uefi_bootable_part partitions in log: ... libguestfs: trace: v2v: list_partitions guestfsd: main_loop: new request, len 0x28 guestfsd: main_loop: proc 8 (list_partitions) took 0.00 seconds libguestfs: trace: v2v: list_partitions = ["/dev/sda1", "/dev/sda2", "/dev/sda3", "/dev/sdb1", "/dev/sdb2", "/dev/sdb5", "/dev/sdc1", "/dev/sdc2", "/dev/sdc5", "/dev/sdd1", "/dev/sdd2", "/dev/sdd5", "/dev/sde1", "/dev/sde2", "/dev/sde5", "/dev/sdf1", "/dev/sdf2", "/dev/sdf5", "/dev/sdg1", "/dev/sdg2", "/dev/sdg5"] ... libguestfs: trace: v2v: part_get_parttype "/dev/sdb" guestfsd: main_loop: new request, len 0x34 commandrvf: stdout=y stderr=y flags=0x0 commandrvf: parted -m -s -- /dev/sdb unit b print libguestfs: trace: v2v: part_get_parttype = "msdos" libguestfs: trace: v2v: part_get_parttype "/dev/sdb" guestfsd: main_loop: proc 214 (part_get_parttype) took 0.00 seconds guestfsd: main_loop: new request, len 0x34 commandrvf: stdout=y stderr=y flags=0x0 commandrvf: parted -m -s -- /dev/sdb unit b print libguestfs: trace: v2v: part_get_parttype = "msdos" libguestfs: trace: v2v: part_to_dev "/dev/sdb5" guestfsd: main_loop: proc 214 (part_get_parttype) took 0.00 seconds [ 9.164666] sdb: sdb1 sdb2 < sdb5 > guestfsd: main_loop: new request, len 0x38 /dev/sdb5: No such file or directory call part_to_dev "/dev/sdb5" failed in is_root_device(): part_to_dev_stub() RESOLVE_DEVICE() is_root_device (): { struct stat statbuf; if (stat (device, &statbuf) == -1) { perror (device); return 0; ... let's look at the udev event log after the error: KERNEL[10.527855] change /devices/pci0000:00/0000:00:03.0/virtio1/host2/target2:0:1/2:0:1:0/block/sdb (block) UDEV [10.528693] change /devices/pci0000:00/0000:00:03.0/virtio1/host2/target2:0:0/2:0:0:0/block/sda/sda1 (block) KERNEL[10.529156] add /devices/pci0000:00/0000:00:03.0/virtio1/host2/target2:0:1/2:0:1:0/block/sdb/sdb1 (block) KERNEL[10.529618] add /devices/pci0000:00/0000:00:03.0/virtio1/host2/target2:0:1/2:0:1:0/block/sdb/sdb2 (block) KERNEL[10.530096] add /devices/pci0000:00/0000:00:03.0/virtio1/host2/target2:0:1/2:0:1:0/block/sdb/sdb5 (block) ... Apparently is_root_device("/dev/sdb5") was called before "/dev/sdb5" was added. To solve issue we need to wait adding a partition before we can access it. Another solution if we wait for adding all partitions somewhere in one place. But there is at least one drawback, not for each task need to touch the disk partitions and waiting for all partitions will slow such tasks. Pavel Butsykin (2): daemon: run 'udevadm settle' with --exit-if-exists option daemon: add udev_settle_file to is_root_device daemon/daemon.h | 2 ++ daemon/guestfsd.c | 42 +++++++++++++++++++++++++++++++----------- 2 files changed, 33 insertions(+), 11 deletions(-) -- 2.11.0
Add udev_settle_file() to run 'udevadm settle' with --exit-if-exists option. It will slightly reduce the waiting-time for pending events if we need to wait for events related to a particular device/file. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- daemon/daemon.h | 2 ++ daemon/guestfsd.c | 39 ++++++++++++++++++++++++++++----------- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/daemon/daemon.h b/daemon/daemon.h index 79a5288f6..90ebaafbe 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -141,6 +141,8 @@ extern int parse_btrfsvol (const char *desc, mountable_t *mountable); extern int prog_exists (const char *prog); +extern void udev_settle_file (const char *file); + extern void udev_settle (void); extern int random_name (char *template); diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index 85ce5d2ad..b82c98acd 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -63,6 +63,8 @@ #include "daemon.h" +#define MAX_ARGS 64 + GUESTFSD_EXT_CMD(str_udevadm, udevadm); GUESTFSD_EXT_CMD(str_uuidgen, uuidgen); @@ -1213,20 +1215,35 @@ random_name (char *template) * fussed if it fails. */ void -udev_settle (void) +udev_settle_file (const char *file) { - char cmd[80]; + const char *argv[MAX_ARGS]; + CLEANUP_FREE char *err = NULL; + size_t i = 0; int r; - snprintf (cmd, sizeof cmd, "%s%s settle", - str_udevadm, verbose ? " --debug" : ""); - if (verbose) - printf ("%s\n", cmd); - r = system (cmd); - if (r == -1) - perror ("system"); - else if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) - fprintf (stderr, "warning: udevadm command failed\n"); + ADD_ARG (argv, i, str_udevadm); + if (verbose) { + ADD_ARG (argv, i, "--debug"); + } + + ADD_ARG (argv, i, "settle"); + if (file) { + ADD_ARG (argv, i, "-E"); + ADD_ARG (argv, i, file); + } + ADD_ARG (argv, i, NULL); + + r = commandv (NULL, &err, argv); + if (r == -1) { + reply_with_error ("udevadm command failed: %s", err); + } +} + +void +udev_settle (void) +{ + udev_settle_file (NULL); } char * -- 2.11.0
On Thursday, 13 April 2017 16:55:26 CEST Pavel Butsykin wrote:
Add udev_settle_file() to run 'udevadm settle' with --exit-if-exists option. It will slightly reduce the waiting-time for pending events if we need to wait for events related to a particular device/file.
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- daemon/daemon.h | 2 ++ daemon/guestfsd.c | 39 ++++++++++++++++++++++++++++----------- 2 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/daemon/daemon.h b/daemon/daemon.h index 79a5288f6..90ebaafbe 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -141,6 +141,8 @@ extern int parse_btrfsvol (const char *desc, mountable_t *mountable);
extern int prog_exists (const char *prog);
+extern void udev_settle_file (const char *file); + extern void udev_settle (void);
extern int random_name (char *template); diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index 85ce5d2ad..b82c98acd 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -63,6 +63,8 @@
#include "daemon.h"
+#define MAX_ARGS 64 + GUESTFSD_EXT_CMD(str_udevadm, udevadm); GUESTFSD_EXT_CMD(str_uuidgen, uuidgen);
@@ -1213,20 +1215,35 @@ random_name (char *template) * fussed if it fails. */ void -udev_settle (void) +udev_settle_file (const char *file) { - char cmd[80]; + const char *argv[MAX_ARGS]; + CLEANUP_FREE char *err = NULL; + size_t i = 0; int r;
- snprintf (cmd, sizeof cmd, "%s%s settle", - str_udevadm, verbose ? " --debug" : ""); - if (verbose) - printf ("%s\n", cmd); - r = system (cmd); - if (r == -1) - perror ("system"); - else if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) - fprintf (stderr, "warning: udevadm command failed\n"); + ADD_ARG (argv, i, str_udevadm); + if (verbose) { + ADD_ARG (argv, i, "--debug"); + } + + ADD_ARG (argv, i, "settle"); + if (file) { + ADD_ARG (argv, i, "-E"); + ADD_ARG (argv, i, file); + } + ADD_ARG (argv, i, NULL); + + r = commandv (NULL, &err, argv); + if (r == -1) { + reply_with_error ("udevadm command failed: %s", err);
IMHO reply_with_error is not correct here -- so far a failure to run udevadm is not fatal at all, so it should be kept that way. Also most probably commandrv, so we can still - show perror when udevadm could not be run (return code = -1) - show the warning when udevadm failed to run (exit code != 0) The rest of the changes looks good. Thanks, -- Pino Toscano
On 13.04.2017 18:10, Pino Toscano wrote:
On Thursday, 13 April 2017 16:55:26 CEST Pavel Butsykin wrote:
Add udev_settle_file() to run 'udevadm settle' with --exit-if-exists option. It will slightly reduce the waiting-time for pending events if we need to wait for events related to a particular device/file.
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- daemon/daemon.h | 2 ++ daemon/guestfsd.c | 39 ++++++++++++++++++++++++++++----------- 2 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/daemon/daemon.h b/daemon/daemon.h index 79a5288f6..90ebaafbe 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -141,6 +141,8 @@ extern int parse_btrfsvol (const char *desc, mountable_t *mountable);
extern int prog_exists (const char *prog);
+extern void udev_settle_file (const char *file); + extern void udev_settle (void);
extern int random_name (char *template); diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index 85ce5d2ad..b82c98acd 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -63,6 +63,8 @@
#include "daemon.h"
+#define MAX_ARGS 64 + GUESTFSD_EXT_CMD(str_udevadm, udevadm); GUESTFSD_EXT_CMD(str_uuidgen, uuidgen);
@@ -1213,20 +1215,35 @@ random_name (char *template) * fussed if it fails. */ void -udev_settle (void) +udev_settle_file (const char *file) { - char cmd[80]; + const char *argv[MAX_ARGS]; + CLEANUP_FREE char *err = NULL; + size_t i = 0; int r;
- snprintf (cmd, sizeof cmd, "%s%s settle", - str_udevadm, verbose ? " --debug" : ""); - if (verbose) - printf ("%s\n", cmd); - r = system (cmd); - if (r == -1) - perror ("system"); - else if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) - fprintf (stderr, "warning: udevadm command failed\n"); + ADD_ARG (argv, i, str_udevadm); + if (verbose) { + ADD_ARG (argv, i, "--debug"); + } + + ADD_ARG (argv, i, "settle"); + if (file) { + ADD_ARG (argv, i, "-E"); + ADD_ARG (argv, i, file); + } + ADD_ARG (argv, i, NULL); + + r = commandv (NULL, &err, argv); + if (r == -1) { + reply_with_error ("udevadm command failed: %s", err);
IMHO reply_with_error is not correct here -- so far a failure to run udevadm is not fatal at all, so it should be kept that way.
I missed that that reply_with_error is not just error output, but may lead to termination of the client. Agree.
Also most probably commandrv, so we can still - show perror when udevadm could not be run (return code = -1) - show the warning when udevadm failed to run (exit code != 0)
will fix.
The rest of the changes looks good.
Thanks! Can you add me in CC when you reply to my messages?
Thanks,
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
On Thu, Apr 13, 2017 at 07:01:09PM +0300, Pavel Butsykin wrote:
I missed that that reply_with_error is not just error output, but may lead to termination of the client. Agree.
That's right -- reply_with_error sends a protocol message back to the client, so it's important to only call it when the client (ie. the library) is expecting it. In this case, just ignore the error or call perror or fprintf (stderr, ...). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
It fixes a fail with ENOENT when device is not yet present after system boot. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- daemon/guestfsd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index b82c98acd..e6a4c65c4 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -404,6 +404,9 @@ int is_root_device (const char *device) { struct stat statbuf; + + udev_settle_file (device); + if (stat (device, &statbuf) == -1) { perror (device); return 0; -- 2.11.0
On Thu, Apr 13, 2017 at 05:55:27PM +0300, Pavel Butsykin wrote:
It fixes a fail with ENOENT when device is not yet present after system boot.
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- daemon/guestfsd.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index b82c98acd..e6a4c65c4 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -404,6 +404,9 @@ int is_root_device (const char *device) { struct stat statbuf; + + udev_settle_file (device); + if (stat (device, &statbuf) == -1) { perror (device); return 0;
This one is fine now, so ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
participants (3)
-
Pavel Butsykin -
Pino Toscano -
Richard W.M. Jones