[PATCH 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 | 23 +++++++++++++++++++---- 2 files changed, 21 insertions(+), 4 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 | 20 ++++++++++++++++---- 2 files changed, 18 insertions(+), 4 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..dccfa15bc 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -1213,13 +1213,18 @@ random_name (char *template) * fussed if it fails. */ void -udev_settle (void) +udev_settle_file (const char *file) { - char cmd[80]; + size_t cmd_size = strlen (str_udevadm) + + sizeof (" settle") + + sizeof (" --debug") + + (file ? sizeof (" --exit-if-exists=") + strlen (file) : 0); + char *cmd = malloc (cmd_size); int r; - snprintf (cmd, sizeof cmd, "%s%s settle", - str_udevadm, verbose ? " --debug" : ""); + snprintf(cmd, cmd_size, "%s%s settle%s%s", + str_udevadm, verbose ? " --debug" : "", + file ? " --exit-if-exists=" : "", file ? file : ""); if (verbose) printf ("%s\n", cmd); r = system (cmd); @@ -1227,6 +1232,13 @@ udev_settle (void) perror ("system"); else if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) fprintf (stderr, "warning: udevadm command failed\n"); + free (cmd); +} + +void +udev_settle (void) +{ + udev_settle_file (NULL); } char * -- 2.11.0
On Wed, Apr 12, 2017 at 05:01:10PM +0300, 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 | 20 ++++++++++++++++---- 2 files changed, 18 insertions(+), 4 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..dccfa15bc 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -1213,13 +1213,18 @@ random_name (char *template) * fussed if it fails. */ void -udev_settle (void) +udev_settle_file (const char *file) { - char cmd[80]; + size_t cmd_size = strlen (str_udevadm) + + sizeof (" settle") + + sizeof (" --debug") + + (file ? sizeof (" --exit-if-exists=") + strlen (file) : 0); + char *cmd = malloc (cmd_size); int r;
- snprintf (cmd, sizeof cmd, "%s%s settle", - str_udevadm, verbose ? " --debug" : ""); + snprintf(cmd, cmd_size, "%s%s settle%s%s", + str_udevadm, verbose ? " --debug" : "", + file ? " --exit-if-exists=" : "", file ? file : ""); if (verbose) printf ("%s\n", cmd); r = system (cmd); @@ -1227,6 +1232,13 @@ udev_settle (void) perror ("system"); else if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) fprintf (stderr, "warning: udevadm command failed\n"); + free (cmd);
There are some ugly quoting (and hence, possibly, security) problems with this patch. It's more secure and also much simpler to call ADD_ARG + commandv here. For an example of how to use it, see: https://github.com/libguestfs/libguestfs/blob/master/daemon/mkfs.c#L41-L247 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 13.04.2017 00:11, Richard W.M. Jones wrote:
On Wed, Apr 12, 2017 at 05:01:10PM +0300, 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 | 20 ++++++++++++++++---- 2 files changed, 18 insertions(+), 4 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..dccfa15bc 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -1213,13 +1213,18 @@ random_name (char *template) * fussed if it fails. */ void -udev_settle (void) +udev_settle_file (const char *file) { - char cmd[80]; + size_t cmd_size = strlen (str_udevadm) + + sizeof (" settle") + + sizeof (" --debug") + + (file ? sizeof (" --exit-if-exists=") + strlen (file) : 0); + char *cmd = malloc (cmd_size); int r;
- snprintf (cmd, sizeof cmd, "%s%s settle", - str_udevadm, verbose ? " --debug" : ""); + snprintf(cmd, cmd_size, "%s%s settle%s%s", + str_udevadm, verbose ? " --debug" : "", + file ? " --exit-if-exists=" : "", file ? file : ""); if (verbose) printf ("%s\n", cmd); r = system (cmd); @@ -1227,6 +1232,13 @@ udev_settle (void) perror ("system"); else if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) fprintf (stderr, "warning: udevadm command failed\n"); + free (cmd);
There are some ugly quoting (and hence, possibly, security) problems with this patch.
It's more secure and also much simpler to call ADD_ARG + commandv here. For an example of how to use it, see:
https://github.com/libguestfs/libguestfs/blob/master/daemon/mkfs.c#L41-L247
I agree, it would be better, thanks!
Rich.
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 dccfa15bc..9aa69301f 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -402,6 +402,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 Wed, Apr 12, 2017 at 05:01:11PM +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 dccfa15bc..9aa69301f 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -402,6 +402,9 @@ int is_root_device (const char *device) { struct stat statbuf; + + udev_settle_file(device);
Add a space after the function name, but with that change ACK.
if (stat (device, &statbuf) == -1) { perror (device); return 0;
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
participants (2)
-
Pavel Butsykin -
Richard W.M. Jones