[PATCH] vstorage: remove build time checks for runtime binaries

Nikolay Shirokovskiy posted 1 patch 3 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1611038071-897439-1-git-send-email-nshirokovskiy@virtuozzo.com
meson.build                            | 22 ++--------------------
src/storage/storage_backend_vstorage.c |  4 ++--
2 files changed, 4 insertions(+), 22 deletions(-)
[PATCH] vstorage: remove build time checks for runtime binaries
Posted by Nikolay Shirokovskiy 3 years, 3 months ago
Accoring to current agreement mentioned in list recently [1]. Now
vstorage driver will be build in default devs environment and also can
be included into CI. This also closes quite old abandoned thread on
alternative checks for binaries in case of this same driver [2].

[1] https://www.redhat.com/archives/libvir-list/2021-January/msg00750.html
[2] https://www.redhat.com/archives/libvir-list/2020-July/msg00697.html

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 meson.build                            | 22 ++--------------------
 src/storage/storage_backend_vstorage.c |  4 ++--
 2 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/meson.build b/meson.build
index b5277b4..e3e7ff7 100644
--- a/meson.build
+++ b/meson.build
@@ -1957,26 +1957,8 @@ if conf.has('WITH_LIBVIRTD')
   endif
 
   if not get_option('storage_vstorage').disabled()
-    vstorage_enable = true
-
-    foreach name : ['vstorage', 'vstorage-mount', 'umount']
-      set_variable(
-        '@0@_prog'.format(name.underscorify()),
-        find_program(name, required: get_option('storage_vstorage'), dirs: libvirt_sbin_path)
-      )
-      if not get_variable('@0@_prog'.format(name.underscorify())).found()
-        vstorage_enable = false
-      endif
-    endforeach
-
-    if vstorage_enable
-      use_storage = true
-      conf.set('WITH_STORAGE_VSTORAGE', 1)
-      foreach name : ['vstorage', 'vstorage-mount', 'umount']
-        path = get_variable('@0@_prog'.format(name.underscorify())).path()
-        conf.set_quoted(name.to_upper(), path)
-      endforeach
-    endif
+    use_storage = true
+    conf.set('WITH_STORAGE_VSTORAGE', 1)
   endif
 
   if not get_option('storage_zfs').disabled()
diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c
index 6cff9f1..7c67407 100644
--- a/src/storage/storage_backend_vstorage.c
+++ b/src/storage/storage_backend_vstorage.c
@@ -65,7 +65,7 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool)
 
     mode = g_strdup_printf("%o", def->target.perms.mode);
 
-    cmd = virCommandNewArgList(VSTORAGE_MOUNT,
+    cmd = virCommandNewArgList("vstorage-mount",
                                "-c", def->source.name,
                                def->target.path,
                                "-m", mode,
@@ -129,7 +129,7 @@ virStorageBackendVzPoolStop(virStoragePoolObjPtr pool)
     if ((rc = virStorageBackendVzIsMounted(pool)) != 1)
         return rc;
 
-    cmd = virCommandNewArgList(UMOUNT, def->target.path, NULL);
+    cmd = virCommandNewArgList("umount", def->target.path, NULL);
     return virCommandRun(cmd, NULL);
 }
 
-- 
1.8.3.1

Re: [PATCH] vstorage: remove build time checks for runtime binaries
Posted by Michal Privoznik 3 years, 3 months ago
On 1/19/21 7:34 AM, Nikolay Shirokovskiy wrote:
> Accoring to current agreement mentioned in list recently [1]. Now
> vstorage driver will be build in default devs environment and also can
> be included into CI. This also closes quite old abandoned thread on
> alternative checks for binaries in case of this same driver [2].
> 
> [1] https://www.redhat.com/archives/libvir-list/2021-January/msg00750.html
> [2] https://www.redhat.com/archives/libvir-list/2020-July/msg00697.html
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>   meson.build                            | 22 ++--------------------
>   src/storage/storage_backend_vstorage.c |  4 ++--
>   2 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index b5277b4..e3e7ff7 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1957,26 +1957,8 @@ if conf.has('WITH_LIBVIRTD')
>     endif
>   
>     if not get_option('storage_vstorage').disabled()
> -    vstorage_enable = true
> -
> -    foreach name : ['vstorage', 'vstorage-mount', 'umount']
> -      set_variable(
> -        '@0@_prog'.format(name.underscorify()),
> -        find_program(name, required: get_option('storage_vstorage'), dirs: libvirt_sbin_path)
> -      )
> -      if not get_variable('@0@_prog'.format(name.underscorify())).found()
> -        vstorage_enable = false
> -      endif
> -    endforeach
> -
> -    if vstorage_enable
> -      use_storage = true
> -      conf.set('WITH_STORAGE_VSTORAGE', 1)
> -      foreach name : ['vstorage', 'vstorage-mount', 'umount']
> -        path = get_variable('@0@_prog'.format(name.underscorify())).path()
> -        conf.set_quoted(name.to_upper(), path)
> -      endforeach
> -    endif
> +    use_storage = true
> +    conf.set('WITH_STORAGE_VSTORAGE', 1)
>     endif
>   
>     if not get_option('storage_zfs').disabled()
> diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c
> index 6cff9f1..7c67407 100644
> --- a/src/storage/storage_backend_vstorage.c
> +++ b/src/storage/storage_backend_vstorage.c
> @@ -65,7 +65,7 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool)
>   
>       mode = g_strdup_printf("%o", def->target.perms.mode);
>   
> -    cmd = virCommandNewArgList(VSTORAGE_MOUNT,
> +    cmd = virCommandNewArgList("vstorage-mount",
>                                  "-c", def->source.name,
>                                  def->target.path,
>                                  "-m", mode,


These two hunks look good, although I have slight preference to keep 
VSTORAGE_MOUNT and maybe just:

   #define VSTORAGE_MOUNT "vstorage-mount"

somewhere in storage_backend_vstorage.c; but that is just cosmetics and 
we have existing examples of your approach too:

   libvirt.git $ git grep "virCommandNew.*(\"" | wc -l
   27


> @@ -129,7 +129,7 @@ virStorageBackendVzPoolStop(virStoragePoolObjPtr pool)
>       if ((rc = virStorageBackendVzIsMounted(pool)) != 1)
>           return rc;
>   
> -    cmd = virCommandNewArgList(UMOUNT, def->target.path, NULL);
> +    cmd = virCommandNewArgList("umount", def->target.path, NULL);
>       return virCommandRun(cmd, NULL);
>   }
>   
> 

A-ha! So if FS driver is disabled, then UMOUNT is undefined? Well, an 
empty string, whatever. If it is so then:

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal