Re: [PATCH] storage: find vstorage-mount binary in runtime

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Tuesday in 2020, Nikolay Shirokovskiy wrote:
> This allows us to use CI for vstorage driver without installing Virtuozzo
> Storage packages. This way we can leave aside license considerations.
>
> By the way we need to change configure defaults from 'check' to 'no' otherwise
> vstorage driver will be build on any system with umount binary which is not
> expected I guess.

If it does not have any compile-time dependencies, compiling it by
default would help developers like me - I would not even have to wait
for the CI to finish to see I've broken the compilation of this driver.

> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
> ---
> m4/virt-storage-vstorage.m4            | 17 +----------------
> src/storage/storage_backend_vstorage.c |  9 ++++++++-
> 2 files changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/m4/virt-storage-vstorage.m4 b/m4/virt-storage-vstorage.m4
> index e3b3bb4..cf0a543 100644
> --- a/m4/virt-storage-vstorage.m4
> +++ b/m4/virt-storage-vstorage.m4
> @@ -21,30 +21,19 @@ dnl
> AC_DEFUN([LIBVIRT_STORAGE_ARG_VSTORAGE], [
>   LIBVIRT_ARG_WITH_FEATURE([STORAGE_VSTORAGE],
>                            [Virtuozzo Storage backend for the storage driver],
> -                           [check])
> +                           [no])
> ])
>
> AC_DEFUN([LIBVIRT_STORAGE_CHECK_VSTORAGE], [
>   if test "$with_storage_vstorage" = "yes" ||
>      test "$with_storage_vstorage" = "check"; then
> -    AC_PATH_PROG([VSTORAGE], [vstorage], [], [$LIBVIRT_SBIN_PATH])
> -    AC_PATH_PROG([VSTORAGE_MOUNT], [vstorage-mount], [], [$LIBVIRT_SBIN_PATH])
>     AC_PATH_PROG([UMOUNT], [umount], [], [$LIBVIRT_SBIN_PATH])

I'm not sure why we (in general):
a) look for the paths of the helper binaries upfront
b) make decisions based on that

It seems reasonable that someone would build libvirt first and then
install e.g. lvm2 and expected the logical storage driver to work.

I doubt the runtime speedup would be significant, is this just to verify
the sanity of the build environment?

Would it make sense to enforce their presence with = "yes", but build
the driver regardless of whether the tools are present with = "check"?

>     if test "$with_storage_vstorage" = "yes"; then
> -      if test -z "$VSTORAGE" || test -z "$VSTORAGE_MOUNT"; then
> -        AC_MSG_ERROR([We need vstorage and vstorage-mount tool for Vstorage storage driver]);
> -      fi
>       if test -z "$UMOUNT" ; then
>         AC_MSG_ERROR([We need umount for Vstorage storage driver]);
>       fi
>     else
> -      if test -z "$VSTORAGE" ; then
> -        with_storage_vstorage=no
> -      fi
> -      if test -z "$VSTORAGE_MOUNT" ; then
> -        with_storage_vstorage=no
> -      fi
>       if test -z "$UMOUNT" ; then
>         with_storage_vstorage=no
>       fi
> @@ -57,10 +46,6 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_VSTORAGE], [
>     if test "$with_storage_vstorage" = "yes" ; then
>       AC_DEFINE_UNQUOTED([WITH_STORAGE_VSTORAGE], 1,
>                          [whether Vstorage backend for storage driver is enabled])
> -      AC_DEFINE_UNQUOTED([VSTORAGE], ["$VSTORAGE"],
> -                         [Location or name of the vstorage client tool])
> -      AC_DEFINE_UNQUOTED([VSTORAGE_MOUNT], ["$VSTORAGE_MOUNT"],
> -                         [Location or name of the vstorage mount tool])
>       AC_DEFINE_UNQUOTED([UMOUNT], ["$UMOUNT"],
>                          [Location or name of the umount programm])
>     fi
> diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c
> index 6cff9f1..ea3bfaa 100644
> --- a/src/storage/storage_backend_vstorage.c
> +++ b/src/storage/storage_backend_vstorage.c
> @@ -44,9 +44,16 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool)
>     g_autofree char *grp_name = NULL;
>     g_autofree char *usr_name = NULL;
>     g_autofree char *mode = NULL;
> +    g_autofree char *mount_bin = NULL;
>     g_autoptr(virCommand) cmd = NULL;
>     int ret;
>
> +    if (!(mount_bin = virFindFileInPath("vstorage-mount"))) {

virCommand already does this for you if the binary path does not start
with a slash, so it would be safe to AC_DEFINE VSTORAGE_MOUNT to
"vstorage-mount" if it wasn't found.

Jano

> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("unable to find vstorage-mount"));
> +        return -1;
> +    }
> +
>     /* Check the permissions */
>     if (def->target.perms.mode == (mode_t)-1)
>         def->target.perms.mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE;
> @@ -65,7 +72,7 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool)
>
>     mode = g_strdup_printf("%o", def->target.perms.mode);
>
> -    cmd = virCommandNewArgList(VSTORAGE_MOUNT,
> +    cmd = virCommandNewArgList(mount_bin,
>                                "-c", def->source.name,
>                                def->target.path,
>                                "-m", mode,
> --
> 1.8.3.1

Attachment: signature.asc
Description: PGP signature