[libvirt] [PATCH 0/4] virutil: Rework how 'udevadm settle' is called
Apparently, some build systems have a very minimalistic approach to their default build root. Not even udevadm is there. This renders our virWaitForDevices() to be no-op. But with this rework we can still run udevadm settle just fine with no extra build dependency added. Michal Prívozník (4): virWaitForDevices: Drop untrue part of comment lib: Drop UDEVSETTLE m4: Provide default value fore UDEVADM m4: Drop needless string checks m4/virt-external-programs.m4 | 26 +++++++------------------- src/util/virutil.c | 20 ++++++-------------- 2 files changed, 13 insertions(+), 33 deletions(-) -- 2.21.0
It's not true that there is a backup loop. There isn't. Drop this part of the comment to not confuse anybody. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virutil.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index e5917d33de..6123741756 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1495,8 +1495,6 @@ void virWaitForDevices(void) /* * 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. */ ignore_value(virRun(settleprog, &exitstatus)); } -- 2.21.0
On Fri, May 17, 2019 at 11:54:14AM +0200, Michal Privoznik wrote:
It's not true that there is a backup loop. There isn't. Drop this
There is one in nodeDeviceFindNewDevice. So: s/untrue/confusing/
part of the comment to not confuse anybody.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virutil.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index e5917d33de..6123741756 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1495,8 +1495,6 @@ void virWaitForDevices(void) /* * 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.
Documenting what the callers do here in this helper seems strange. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The udevsettle binary is no longer user anywhere as it was replaced by 'udevadm settle'. There's no reason for us to even check for it in configure. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-external-programs.m4 | 5 ----- src/util/virutil.c | 6 +----- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/m4/virt-external-programs.m4 b/m4/virt-external-programs.m4 index ab6149288f..3c915e1a65 100644 --- a/m4/virt-external-programs.m4 +++ b/m4/virt-external-programs.m4 @@ -46,7 +46,6 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [ AC_PATH_PROG([RADVD], [radvd], [radvd], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([TC], [tc], [tc], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([UDEVADM], [udevadm], [], [$LIBVIRT_SBIN_PATH]) - AC_PATH_PROG([UDEVSETTLE], [udevsettle], [], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([MODPROBE], [modprobe], [modprobe], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([RMMOD], [rmmod], [rmmod], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([MMCTL], [mm-ctl], [mm-ctl], [$LIBVIRT_SBIN_PATH]) @@ -71,10 +70,6 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [ 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 if test -n "$MODPROBE"; then AC_DEFINE_UNQUOTED([MODPROBE], ["$MODPROBE"], [Location or name of the modprobe program]) diff --git a/src/util/virutil.c b/src/util/virutil.c index 6123741756..ecef24d2f3 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1479,14 +1479,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, #endif -#if defined(UDEVADM) || defined(UDEVSETTLE) +#if defined(UDEVADM) void virWaitForDevices(void) { -# ifdef UDEVADM const char *const settleprog[] = { UDEVADM, "settle", NULL }; -# else - const char *const settleprog[] = { UDEVSETTLE, NULL }; -# endif int exitstatus; if (access(settleprog[0], X_OK) != 0) -- 2.21.0
On Fri, May 17, 2019 at 11:54:15AM +0200, Michal Privoznik wrote:
The udevsettle binary is no longer user anywhere as it was
*used
replaced by 'udevadm settle'. There's no reason for us to even check for it in configure.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-external-programs.m4 | 5 ----- src/util/virutil.c | 6 +----- 2 files changed, 1 insertion(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
https://bugzilla.redhat.com/show_bug.cgi?id=1710575 It may happen that the system where libvirt is built at doesn't have udevadm binary but the one where it runs does have it. If we change how udevadm is ran in virWaitForDevices() then we can safely pass a default value in m4 macro. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-external-programs.m4 | 9 +++------ src/util/virutil.c | 14 ++++++-------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/m4/virt-external-programs.m4 b/m4/virt-external-programs.m4 index 3c915e1a65..f1ae104b32 100644 --- a/m4/virt-external-programs.m4 +++ b/m4/virt-external-programs.m4 @@ -45,7 +45,7 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [ AC_PATH_PROG([DNSMASQ], [dnsmasq], [dnsmasq], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([RADVD], [radvd], [radvd], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([TC], [tc], [tc], [$LIBVIRT_SBIN_PATH]) - AC_PATH_PROG([UDEVADM], [udevadm], [], [$LIBVIRT_SBIN_PATH]) + AC_PATH_PROG([UDEVADM], [udevadm], [udevadm], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([MODPROBE], [modprobe], [modprobe], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([RMMOD], [rmmod], [rmmod], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([MMCTL], [mm-ctl], [mm-ctl], [$LIBVIRT_SBIN_PATH]) @@ -65,11 +65,8 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [ [Location or name of the mm-ctl program]) AC_DEFINE_UNQUOTED([OVSVSCTL], ["$OVSVSCTL"], [Location or name of the ovs-vsctl program]) - - if test -n "$UDEVADM"; then - AC_DEFINE_UNQUOTED([UDEVADM], ["$UDEVADM"], - [Location or name of the udevadm program]) - fi + AC_DEFINE_UNQUOTED([UDEVADM], ["$UDEVADM"], + [Location or name of the udevadm program]) if test -n "$MODPROBE"; then AC_DEFINE_UNQUOTED([MODPROBE], ["$MODPROBE"], [Location or name of the modprobe program]) diff --git a/src/util/virutil.c b/src/util/virutil.c index ecef24d2f3..2db0e9769c 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1479,25 +1479,23 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, #endif -#if defined(UDEVADM) void virWaitForDevices(void) { - const char *const settleprog[] = { UDEVADM, "settle", NULL }; + VIR_AUTOPTR(virCommand) cmd = NULL; + VIR_AUTOFREE(char *) udev = NULL; int exitstatus; - if (access(settleprog[0], X_OK) != 0) + if (!(udev = virFindFileInPath(UDEVADM)) || + !virFileIsExecutable(udev) || + !(cmd = virCommandNewArgList(udev, "settle", NULL))) 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. */ - ignore_value(virRun(settleprog, &exitstatus)); + ignore_value(virCommandRun(cmd, &exitstatus)); } -#else -void virWaitForDevices(void) -{} -#endif #if WITH_DEVMAPPER bool -- 2.21.0
On Fri, May 17, 2019 at 11:54:16AM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1710575
It may happen that the system where libvirt is built at doesn't have udevadm binary but the one where it runs does have it. If we change how udevadm is ran in virWaitForDevices() then we
s/ran/run/
can safely pass a default value in m4 macro.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-external-programs.m4 | 9 +++------ src/util/virutil.c | 14 ++++++-------- 2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/m4/virt-external-programs.m4 b/m4/virt-external-programs.m4 index 3c915e1a65..f1ae104b32 100644 --- a/m4/virt-external-programs.m4 +++ b/m4/virt-external-programs.m4 @@ -45,7 +45,7 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [ AC_PATH_PROG([DNSMASQ], [dnsmasq], [dnsmasq], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([RADVD], [radvd], [radvd], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([TC], [tc], [tc], [$LIBVIRT_SBIN_PATH]) - AC_PATH_PROG([UDEVADM], [udevadm], [], [$LIBVIRT_SBIN_PATH]) + AC_PATH_PROG([UDEVADM], [udevadm], [udevadm], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([MODPROBE], [modprobe], [modprobe], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([RMMOD], [rmmod], [rmmod], [$LIBVIRT_SBIN_PATH]) AC_PATH_PROG([MMCTL], [mm-ctl], [mm-ctl], [$LIBVIRT_SBIN_PATH]) @@ -65,11 +65,8 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [ [Location or name of the mm-ctl program]) AC_DEFINE_UNQUOTED([OVSVSCTL], ["$OVSVSCTL"], [Location or name of the ovs-vsctl program]) - - if test -n "$UDEVADM"; then - AC_DEFINE_UNQUOTED([UDEVADM], ["$UDEVADM"], - [Location or name of the udevadm program]) - fi + AC_DEFINE_UNQUOTED([UDEVADM], ["$UDEVADM"], + [Location or name of the udevadm program]) if test -n "$MODPROBE"; then AC_DEFINE_UNQUOTED([MODPROBE], ["$MODPROBE"], [Location or name of the modprobe program]) diff --git a/src/util/virutil.c b/src/util/virutil.c index ecef24d2f3..2db0e9769c 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1479,25 +1479,23 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, #endif
-#if defined(UDEVADM) void virWaitForDevices(void) { - const char *const settleprog[] = { UDEVADM, "settle", NULL }; + VIR_AUTOPTR(virCommand) cmd = NULL; + VIR_AUTOFREE(char *) udev = NULL; int exitstatus;
- if (access(settleprog[0], X_OK) != 0) + if (!(udev = virFindFileInPath(UDEVADM)) || + !virFileIsExecutable(udev) ||
virFileIsExecutable is not necessary here, virFindFileInPath has already done the check.
+ !(cmd = virCommandNewArgList(udev, "settle", NULL)))
Please give this condition a separate 'if'.
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. */
I'm not sure what kind of errors can happen here, but we should be able to expect that 'udevadm' is present on a Linux system. Reporting an error in that case might be better than silently dropping devices. But strictly speaking, this patch is an improvement, so: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
- ignore_value(virRun(settleprog, &exitstatus)); + ignore_value(virCommandRun(cmd, &exitstatus)); }
We provide default values for both MODPROBE and RMMOD and thus there is no way that their paths can be empty strings. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-external-programs.m4 | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/m4/virt-external-programs.m4 b/m4/virt-external-programs.m4 index f1ae104b32..0f995998c3 100644 --- a/m4/virt-external-programs.m4 +++ b/m4/virt-external-programs.m4 @@ -67,14 +67,10 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [ [Location or name of the ovs-vsctl program]) AC_DEFINE_UNQUOTED([UDEVADM], ["$UDEVADM"], [Location or name of the udevadm program]) - if test -n "$MODPROBE"; then - AC_DEFINE_UNQUOTED([MODPROBE], ["$MODPROBE"], - [Location or name of the modprobe program]) - fi - if test -n "$RMMOD"; then - AC_DEFINE_UNQUOTED([RMMOD], ["$RMMOD"], - [Location or name of the rmmod program]) - fi + AC_DEFINE_UNQUOTED([MODPROBE], ["$MODPROBE"], + [Location or name of the modprobe program]) + AC_DEFINE_UNQUOTED([RMMOD], ["$RMMOD"], + [Location or name of the rmmod program]) AC_DEFINE_UNQUOTED([SCRUB], ["$SCRUB"], [Location or name of the scrub program (for wiping algorithms)]) AC_DEFINE_UNQUOTED([ADDR2LINE], ["$ADDR2LINE"], -- 2.21.0
On Fri, May 17, 2019 at 11:54:17AM +0200, Michal Privoznik wrote:
We provide default values for both MODPROBE and RMMOD and thus there is no way that their paths can be empty strings.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-external-programs.m4 | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik