[libvirt] [PATCH 3/4] m4: Provide default value fore UDEVADM

[Ján Tomko]

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 at 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 at redhat.com>

Jano

>-    ignore_value(virRun(settleprog, &exitstatus));
>+    ignore_value(virCommandRun(cmd, &exitstatus));
> }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190517/b5d03d76/attachment-0001.sig>