[libvirt] [PATCH 2/2]: Call udevsettle in the appropriate places
Instead of relying solely on polling for /dev devices to appear in libvirt, we really should be synchronizing against udev. This is generally done by a call to udevsettle, which is exactly what this patch implements for the storage backends that are likely to create new /dev nodes. I believe I've read that even after udevsettle, you are not guaranteed that devices are all the way created, so we still need the polling in the rest of the sources, but this should give us a much better chance of things existing as we expect. Signed-off-by: Chris Lalancette <clalance@redhat.com> 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 26 Nov 2008 12:40:19 -0000 @@ -270,6 +270,20 @@ return 0; } +void virStorageBackendWaitForDevices(virConnectPtr conn) +{ + const char *const settleprog[] = { UDEVSETTLE, NULL }; + int exitstatus; + + /* + * 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 (udevsettle exits badly, or doesn't exist + * at all), then we still have a fallback mechanism anyway. + */ + virRun(conn, settleprog, &exitstatus); +} + /* * 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.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 26 Nov 2008 12:40:19 -0000 @@ -69,6 +69,8 @@ int fd, int withCapacity); +void virStorageBackendWaitForDevices(virConnectPtr conn); + char *virStorageBackendStablePath(virConnectPtr conn, virStoragePoolObjPtr pool, const char *devpath); 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 26 Nov 2008 12:40:19 -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_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 26 Nov 2008 12:40:19 -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_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 26 Nov 2008 12:40:19 -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: 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 26 Nov 2008 12:40:19 -0000 @@ -115,11 +115,15 @@ [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([BRCTL], [brctl], [brctl], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_PATH_PROG([UDEVSETTLE], [udevsettle], [udevsettle], + [/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)]) +AC_DEFINE_UNQUOTED([UDEVSETTLE],["$UDEVSETTLE"], + [Location or name of the udevsettle program (see udev)]) dnl Specific dir for HTML output ? AC_ARG_WITH([html-dir], [AC_HELP_STRING([--with-html-dir=path],
On Wed, Nov 26, 2008 at 05:08:49PM +0100, Chris Lalancette wrote:
+AC_PATH_PROG([UDEVSETTLE], [udevsettle], [udevsettle], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) As far as I understand things as of udev 117 udevsettle is deprecated in favour of "udevadm settle". So it's probably better to prefer this over udevsettle if present? http://git.kernel.org/?p=linux/hotplug/udev.git;a=blob;f=ChangeLog;h=d317b97...
Recent udev versions even dropped the compat symlink. Cheers, -- Guido
Guido Günther wrote:
On Wed, Nov 26, 2008 at 05:08:49PM +0100, Chris Lalancette wrote:
+AC_PATH_PROG([UDEVSETTLE], [udevsettle], [udevsettle], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) As far as I understand things as of udev 117 udevsettle is deprecated in favour of "udevadm settle". So it's probably better to prefer this over udevsettle if present? http://git.kernel.org/?p=linux/hotplug/udev.git;a=blob;f=ChangeLog;h=d317b97...
Recent udev versions even dropped the compat symlink.
Ah, thanks for the heads up. I'll definitely change this to be "udevadm settle" then. I'll just wait to see if there are any other review comments first. Thanks, -- Chris Lalancette
On Wed, Nov 26, 2008 at 05:08:49PM +0100, Chris Lalancette wrote:
Instead of relying solely on polling for /dev devices to appear in libvirt, we really should be synchronizing against udev. This is generally done by a call to udevsettle, which is exactly what this patch implements for the storage backends that are likely to create new /dev nodes. I believe I've read that even after udevsettle, you are not guaranteed that devices are all the way created, so we still need the polling in the rest of the sources, but this should give us a much better chance of things existing as we expect.
Signed-off-by: Chris Lalancette <clalance@redhat.com>
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 26 Nov 2008 12:40:19 -0000 @@ -270,6 +270,20 @@ return 0; }
+void virStorageBackendWaitForDevices(virConnectPtr conn) +{ + const char *const settleprog[] = { UDEVSETTLE, NULL }; + int exitstatus; + + /* + * 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 (udevsettle exits badly, or doesn't exist + * at all), then we still have a fallback mechanism anyway. + */ + virRun(conn, settleprog, &exitstatus); +}
If we know it doesn't exist, we should not try to run it.
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 26 Nov 2008 12:40:19 -0000 @@ -115,11 +115,15 @@ [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([BRCTL], [brctl], [brctl], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_PATH_PROG([UDEVSETTLE], [udevsettle], [udevsettle], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
This will set UDEVSETTLE=udevsettle even if it doesn't exist. This will result in us running it on every OS we build on. We should not set this variable if its not found. 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:
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 26 Nov 2008 12:40:19 -0000 @@ -270,6 +270,20 @@ return 0; }
+void virStorageBackendWaitForDevices(virConnectPtr conn) +{ + const char *const settleprog[] = { UDEVSETTLE, NULL }; + int exitstatus; + + /* + * 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 (udevsettle exits badly, or doesn't exist + * at all), then we still have a fallback mechanism anyway. + */ + virRun(conn, settleprog, &exitstatus); +}
If we know it doesn't exist, we should not try to run it.
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 26 Nov 2008 12:40:19 -0000 @@ -115,11 +115,15 @@ [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([BRCTL], [brctl], [brctl], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_PATH_PROG([UDEVSETTLE], [udevsettle], [udevsettle], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
This will set UDEVSETTLE=udevsettle even if it doesn't exist. This will result in us running it on every OS we build on. We should not set this variable if its not found.
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@redhat.com> 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:
This will set UDEVSETTLE=udevsettle even if it doesn't exist. This will result in us running it on every OS we build on. We should not set this variable if its not found.
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".
This patch looks fine to me, +1 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Nov 27, 2008 at 08:53:49AM +0100, Chris Lalancette wrote:
Daniel P. Berrange wrote:
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
ACK.
+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 +}
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 :|
Daniel P. Berrange wrote:
+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 +}
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
Committed with this change in place. -- Chris Lalancette
On Thu, Nov 27, 2008 at 8:53 AM, Chris Lalancette <clalance@redhat.com>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".
Please make it try both: RHEL5 has an ancient udev095 and doesn't have udevadm but it does have udevsettle
Alan Pevec wrote:
On Thu, Nov 27, 2008 at 8:53 AM, Chris Lalancette <clalance@redhat.com <mailto:clalance@redhat.com>> 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".
Please make it try both: RHEL5 has an ancient udev095 and doesn't have udevadm but it does have udevsettle
Sigh. Silly older distros :). Thanks for the heads up, though, good to know it now rather than later. Unfortunately, it's not so easy to make it try both; we would get into a twisty passage of #ifdef's, I think. Dan, what do you think of the attached patch for older udev compatibility? -- Chris Lalancette Index: configure.in =================================================================== RCS file: /data/cvs/libvirt/configure.in,v retrieving revision 1.188 diff -u -r1.188 configure.in --- configure.in 28 Nov 2008 07:50:20 -0000 1.188 +++ configure.in 28 Nov 2008 14:50:11 -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], 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 --- src/storage_backend.c 28 Nov 2008 07:50:20 -0000 1.31 +++ src/storage_backend.c 28 Nov 2008 14:50:11 -0000 @@ -287,6 +287,23 @@ */ virRun(conn, settleprog, &exitstatus); } +#elif defined(UDEVSETTLE) +void virStorageBackendWaitForDevices(virConnectPtr conn) +{ + const char *const settleprog[] = { UDEVSETTLE, NULL }; + int exitstatus; + + if (access(UDEVSETTLE, 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); +} #else void virStorageBackendWaitForDevices(virConnectPtr conn ATTRIBUTE_UNUSED) {} #endif
On Fri, Nov 28, 2008 at 03:50:11PM +0100, Chris Lalancette wrote:
Alan Pevec wrote:
On Thu, Nov 27, 2008 at 8:53 AM, Chris Lalancette <clalance@redhat.com <mailto:clalance@redhat.com>> 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".
Please make it try both: RHEL5 has an ancient udev095 and doesn't have udevadm but it does have udevsettle
Sigh. Silly older distros :). Thanks for the heads up, though, good to know it now rather than later. Unfortunately, it's not so easy to make it try both; we would get into a twisty passage of #ifdef's, I think. Dan, what do you think of the attached patch for older udev compatibility?
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.31 diff -u -r1.31 storage_backend.c --- src/storage_backend.c 28 Nov 2008 07:50:20 -0000 1.31 +++ src/storage_backend.c 28 Nov 2008 14:50:11 -0000 @@ -287,6 +287,23 @@ */ virRun(conn, settleprog, &exitstatus); } +#elif defined(UDEVSETTLE) +void virStorageBackendWaitForDevices(virConnectPtr conn) +{ + const char *const settleprog[] = { UDEVSETTLE, NULL }; + int exitstatus; + + if (access(UDEVSETTLE, 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); +}
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:
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;
Well, I didn't do that originally because of the access() check, which would also have needed to use #ifdef. But I just realized I should be able to do: if (access(settleprog[0], X_OK) != 0) return; And be good. So I'll respin the patch like you say above and with this fix in place. -- Chris Lalancette
Daniel P. Berrange 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;
OK, updated patch attached that basically does the above, and adds the configure.in test. Signed-off-by: Chris Lalancette <clalance@redhat.com> 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],
Chris Lalancette wrote:
Daniel P. Berrange 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;
OK, updated patch attached that basically does the above, and adds the configure.in test.
Committed this after talking to DanB about it on IRC. -- Chris Lalancette
participants (5)
-
Alan Pevec -
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Guido Günther