Daniel P. Berrange wrote: Right, both points make sense. I think the following patch should address it; I only conditionally set the UDEVADM variable if I find it. So, for machines without it, the meat of virStorageBackendWaitForDevices is compiled out. In places where I've found it on the build machine, I then do "access" for executable at runtime, and only if that succeeds do I run it. Does that seem correct? In addition, based on the comment from Guido, I changed it over to use "udevadm settle" instead of "udevsettle". Signed-off-by: Chris Lalancette <clalance(a)redhat.com&gt; Index: configure.in =================================================================== RCS file: /data/cvs/libvirt/configure.in,v retrieving revision 1.187 diff -u -r1.187 configure.in --- a/configure.in 25 Nov 2008 15:48:11 -0000 1.187 +++ b/configure.in 27 Nov 2008 07:44:51 -0000 @@ -115,11 +115,17 @@ [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([BRCTL], [brctl], [brctl], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_PATH_PROG([UDEVADM], [udevadm], [], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"], [Location or name of the dnsmasq program]) AC_DEFINE_UNQUOTED([BRCTL],["$BRCTL"], [Location or name of the brctl program (see bridge-utils)]) +if test -n "$UDEVADM"; then + AC_DEFINE_UNQUOTED([UDEVADM],["$UDEVADM"], + [Location or name of the udevadm program]) +fi dnl Specific dir for HTML output ? AC_ARG_WITH([html-dir], [AC_HELP_STRING([--with-html-dir=path], Index: src/storage_backend.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend.c,v retrieving revision 1.29 diff -u -r1.29 storage_backend.c --- a/src/storage_backend.c 17 Nov 2008 11:19:33 -0000 1.29 +++ b/src/storage_backend.c 27 Nov 2008 07:44:51 -0000 @@ -270,6 +270,25 @@ return 0; } +void virStorageBackendWaitForDevices(virConnectPtr conn) +{ +#ifdef UDEVADM + const char *const settleprog[] = { UDEVADM, "settle", NULL }; + int exitstatus; + + if (access(UDEVADM, X_OK) != 0) + return; + + /* + * NOTE: we ignore errors here; this is just to make sure that any device + * nodes that are being created finish before we try to scan them. + * If this fails for any reason, we still have the backup of polling for + * 5 seconds for device nodes. + */ + virRun(conn, settleprog, &exitstatus); +#endif +} + /* * Given a volume path directly in /dev/XXX, iterate over the * entries in the directory pool->def->target.path and find the Index: src/storage_backend_iscsi.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend_iscsi.c,v retrieving revision 1.18 diff -u -r1.18 storage_backend_iscsi.c --- a/src/storage_backend_iscsi.c 17 Nov 2008 11:19:33 -0000 1.18 +++ b/src/storage_backend_iscsi.c 27 Nov 2008 07:44:51 -0000 @@ -603,6 +603,8 @@ pool->def->allocation = pool->def->capacity = pool->def->available = 0; + virStorageBackendWaitForDevices(conn); + if ((session = virStorageBackendISCSISession(conn, pool)) == NULL) goto cleanup; if (virStorageBackendISCSIRescanLUNs(conn, pool, session) < 0) Index: src/storage_backend_disk.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend_disk.c,v retrieving revision 1.20 diff -u -r1.20 storage_backend_disk.c --- a/src/storage_backend_disk.c 17 Nov 2008 11:19:33 -0000 1.20 +++ b/src/storage_backend_disk.c 27 Nov 2008 07:44:51 -0000 @@ -262,6 +262,8 @@ VIR_FREE(pool->def->source.devices[0].freeExtents); pool->def->source.devices[0].nfreeExtent = 0; + virStorageBackendWaitForDevices(conn); + return virStorageBackendDiskReadPartitions(conn, pool, NULL); } Index: src/storage_backend_logical.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend_logical.c,v retrieving revision 1.26 diff -u -r1.26 storage_backend_logical.c --- a/src/storage_backend_logical.c 17 Nov 2008 11:19:33 -0000 1.26 +++ b/src/storage_backend_logical.c 27 Nov 2008 07:44:51 -0000 @@ -470,6 +470,8 @@ }; int exitstatus; + virStorageBackendWaitForDevices(conn); + /* Get list of all logical volumes */ if (virStorageBackendLogicalFindLVs(conn, pool, NULL) < 0) { virStoragePoolObjClearVols(pool); Index: src/storage_backend.h =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend.h,v retrieving revision 1.13 diff -u -r1.13 storage_backend.h --- a/src/storage_backend.h 17 Nov 2008 11:19:33 -0000 1.13 +++ b/src/storage_backend.h 27 Nov 2008 07:44:51 -0000 @@ -69,6 +69,8 @@ int fd, int withCapacity); +void virStorageBackendWaitForDevices(virConnectPtr conn); + char *virStorageBackendStablePath(virConnectPtr conn, virStoragePoolObjPtr pool, const char *devpath);

On Thu, Nov 27, 2008 at 08:53:49AM +0100, Chris Lalancette wrote: ACK. This will generate a compiler warning when UDEVADM is not defined. Better to have the #ifdef around the entire function, and then do a no-op decl with ATTRIBUTE_UNUSED #else void virStorageBackendWaitForDevices(virConnectPtr conn ATTRIBUTE_UNUSED) {} #endif 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 :|

On Fri, Nov 28, 2008 at 03:50:11PM +0100, Chris Lalancette wrote: This seems rather overkill when you could just do #if defined(UDEVADM) || defined(UDEVSETTLE) void virStorageBackendWaitForDevices(virConnectPtr conn) { #ifdef UDEVADM const char *const settleprog[] = { UDEVADM, "settle", NULL }; #else const char *const settleprog[] = { UDEVSETTLE, NULL }; #endif int exitstatus; 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 :|

Daniel P. Berrange wrote: OK, updated patch attached that basically does the above, and adds the configure.in test. Signed-off-by: Chris Lalancette <clalance(a)redhat.com&gt; Index: src/storage_backend.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend.c,v retrieving revision 1.31 diff -u -r1.31 storage_backend.c --- a/src/storage_backend.c 28 Nov 2008 07:50:20 -0000 1.31 +++ b/src/storage_backend.c 1 Dec 2008 11:47:16 -0000 @@ -270,13 +270,17 @@ return 0; } -#ifdef UDEVADM +#if defined(UDEVADM) || defined(UDEVSETTLE) void virStorageBackendWaitForDevices(virConnectPtr conn) { +#ifdef UDEVADM const char *const settleprog[] = { UDEVADM, "settle", NULL }; +#else + const char *const settleprog[] = { UDEVSETTLE, NULL }; +#endif int exitstatus; - if (access(UDEVADM, X_OK) != 0) + if (access(settleprog[0], X_OK) != 0) return; /* Index: configure.in =================================================================== RCS file: /data/cvs/libvirt/configure.in,v retrieving revision 1.188 diff -u -r1.188 configure.in --- a/configure.in 28 Nov 2008 07:50:20 -0000 1.188 +++ b/configure.in 1 Dec 2008 11:47:16 -0000 @@ -117,6 +117,8 @@ [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([UDEVADM], [udevadm], [], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_PATH_PROG([UDEVSETTLE], [udevsettle], [], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"], [Location or name of the dnsmasq program]) @@ -126,6 +128,10 @@ AC_DEFINE_UNQUOTED([UDEVADM],["$UDEVADM"], [Location or name of the udevadm program]) fi +if test -n "$UDEVSETTLE"; then + AC_DEFINE_UNQUOTED([UDEVSETTLE],["$UDEVSETTLE"], + [Location or name of the udevsettle program]) +fi dnl Specific dir for HTML output ? AC_ARG_WITH([html-dir], [AC_HELP_STRING([--with-html-dir=path],