10:08 a.m.
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(a)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],
11:11 a.m.
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=d317...
Recent udev versions even dropped the compat symlink.
Cheers,
-- Guido
2:39 p.m.
New subject: [libvirt] [PATCH 2/2]: Call udevsettle in the appropriate places
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=d317...
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
2:44 p.m.
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(a)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 :|
1:53 a.m.
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(a)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);
10:17 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:21 a.m.
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 :|
1:50 a.m.
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
7:15 a.m.
On Thu, Nov 27, 2008 at 8:53 AM, Chris Lalancette <clalance(a)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
8:50 a.m.
Alan Pevec wrote:
On Thu, Nov 27, 2008 at 8:53 AM, Chris Lalancette
<clalance(a)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
9:08 a.m.
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(a)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 :|
9:13 a.m.
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
6:02 a.m.
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(a)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],
6:42 a.m.
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
5833
days inactive
5839
days old
13 comments
5 participants
participants (5)
-
Alan Pevec -
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Guido Günther