Libguestfs - Apr 2017 - Re: [PATCH v2 1/2] daemon: run 'udevadm settle' with --exit-if-exists option

On Thursday, 13 April 2017 16:55:26 CEST Pavel Butsykin
wrote:
> Add udev_settle_file() to run 'udevadm settle' with
--exit-if-exists option. It
> will slightly reduce the waiting-time for pending events if we need to wait
> for events related to a particular device/file.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  daemon/daemon.h   |  2 ++
>  daemon/guestfsd.c | 39 ++++++++++++++++++++++++++++-----------
>  2 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index 79a5288f6..90ebaafbe 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -141,6 +141,8 @@ extern int parse_btrfsvol (const char *desc,
mountable_t *mountable);
>  
>  extern int prog_exists (const char *prog);
>  
> +extern void udev_settle_file (const char *file);
> +
>  extern void udev_settle (void);
>  
>  extern int random_name (char *template);
> diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> index 85ce5d2ad..b82c98acd 100644
> --- a/daemon/guestfsd.c
> +++ b/daemon/guestfsd.c
> @@ -63,6 +63,8 @@
>  
>  #include "daemon.h"
>  
> +#define MAX_ARGS 64
> +
>  GUESTFSD_EXT_CMD(str_udevadm, udevadm);
>  GUESTFSD_EXT_CMD(str_uuidgen, uuidgen);
>  
> @@ -1213,20 +1215,35 @@ random_name (char *template)
>   * fussed if it fails.
>   */
>  void
> -udev_settle (void)
> +udev_settle_file (const char *file)
>  {
> -  char cmd[80];
> +  const char *argv[MAX_ARGS];
> +  CLEANUP_FREE char *err = NULL;
> +  size_t i = 0;
>    int r;
>  
> -  snprintf (cmd, sizeof cmd, "%s%s settle",
> -            str_udevadm, verbose ? " --debug" : "");
> -  if (verbose)
> -    printf ("%s\n", cmd);
> -  r = system (cmd);
> -  if (r == -1)
> -    perror ("system");
> -  else if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
> -    fprintf (stderr, "warning: udevadm command failed\n");
> +  ADD_ARG (argv, i, str_udevadm);
> +  if (verbose) {
> +    ADD_ARG (argv, i, "--debug");
> +  }
> +
> +  ADD_ARG (argv, i, "settle");
> +  if (file) {
> +    ADD_ARG (argv, i, "-E");
> +    ADD_ARG (argv, i, file);
> +  }
> +  ADD_ARG (argv, i, NULL);
> +
> +  r = commandv (NULL, &err, argv);
> +  if (r == -1) {
> +    reply_with_error ("udevadm command failed: %s", err);
IMHO reply_with_error is not correct here -- so far a failure to run
udevadm is not fatal at all, so it should be kept that way.
Also most probably commandrv, so we can still
- show perror when udevadm could not be run (return code = -1)
- show the warning when udevadm failed to run (exit code != 0)

The rest of the changes looks good.

Thanks,
-- 
Pino Toscano