[PATCH] storage: find vstorage-mount binary in runtime
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. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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]) 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"))) { + 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
On Tue, Jul 14, 2020 at 10:26:00AM +0300, 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.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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]) ])
Why was this changed to "no". It means we'll never enable the storage driver without an explicit --with-storage-vstorage arg passed
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])
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"))) { + 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
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 14.07.2020 11:50, Daniel P. Berrangé wrote:
On Tue, Jul 14, 2020 at 10:26:00AM +0300, 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.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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]) ])
Why was this changed to "no". It means we'll never enable the storage driver without an explicit --with-storage-vstorage arg passed
But with "check" we will have this driver built on any system with umount as I mentioned in commit message. I guess this is not desired given the driver actually needs some more binaries that are not usually installed. Nikolay
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])
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"))) { + 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
Regards, Daniel
On Tue, Jul 14, 2020 at 11:55:47AM +0300, Nikolay Shirokovskiy wrote:
On 14.07.2020 11:50, Daniel P. Berrangé wrote:
On Tue, Jul 14, 2020 at 10:26:00AM +0300, 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.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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]) ])
Why was this changed to "no". It means we'll never enable the storage driver without an explicit --with-storage-vstorage arg passed
But with "check" we will have this driver built on any system with umount as I mentioned in commit message. I guess this is not desired given the driver actually needs some more binaries that are not usually installed.
The core rule for configure checks is that users should never have to pass any --with args to enable use of features that their host OS has. So we need some mechanism to correctly enable the driver. I'd suggest that we in fact get rid of this entire check, and just make the storage driver be directly configured based on $with_vz, beucase if you have the hypervisor driver enabled, you want the storage driver to match. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 14.07.2020 12:02, Daniel P. Berrangé wrote:
On Tue, Jul 14, 2020 at 11:55:47AM +0300, Nikolay Shirokovskiy wrote:
On 14.07.2020 11:50, Daniel P. Berrangé wrote:
On Tue, Jul 14, 2020 at 10:26:00AM +0300, 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.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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]) ])
Why was this changed to "no". It means we'll never enable the storage driver without an explicit --with-storage-vstorage arg passed
But with "check" we will have this driver built on any system with umount as I mentioned in commit message. I guess this is not desired given the driver actually needs some more binaries that are not usually installed.
The core rule for configure checks is that users should never have to pass any --with args to enable use of features that their host OS has. So we need some mechanism to correctly enable the driver.
I'd suggest that we in fact get rid of this entire check, and just make the storage driver be directly configured based on $with_vz, beucase if you have the hypervisor driver enabled, you want the storage driver to match.
These drivers are not so strongly related. One can be used without another and vz driver soon going to be outdated. May be we can leave "check" really check for vstorage-mount presence and leave default to "check" also while make "yes" be forceful and don't fail if there is no binary? This new "yes" behavior is supposed to be used in CI. Nikolay
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@virtuozzo.com> --- 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
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Nikolay Shirokovskiy