Currently when we build with nbdkit support, libvirt will always try to
use nbdkit to access remote disk sources when it is available. But
without an up-to-date selinux policy allowing this, it will fail.
because the required selinux policies are not yet widely available, we
have disabled nbdkit support on rpm builds for all distributions before
Fedora 40.

Unfortunately, this makes it more difficult to test nbdkit support.
After someone updates to the necessary selinux policies, they would also
need to rebuild libvirt to enable nbdkit support. By introducing a
configure option (nbdkit_config_default), we can build packages with
nbdkit support but have it disabled by default.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Suggested-by: Andrea Bolognani <abologna@redhat.com>
---
changes in v4
 - squashed in Andrea's suggested changes
   - updated error message
   - Changed one instance of WITH_NDBKIT to WITH_NBDKIT :)
   - tested

 libvirt.spec.in                    | 18 ++++++++++++++----
 meson.build                        | 10 ++++++++++
 meson_options.txt                  |  3 ++-
 src/qemu/libvirtd_qemu.aug         |  3 +++
 src/qemu/meson.build               |  2 ++
 src/qemu/qemu.conf.in              | 11 +++++++++++
 src/qemu/qemu_conf.c               | 24 ++++++++++++++++++++++++
 src/qemu/qemu_conf.h               |  2 ++
 src/qemu/qemu_domain.c             |  3 +++
 src/qemu/test_libvirtd_qemu.aug.in |  1 +
 tests/qemuxml2argvtest.c           | 15 +++++++++------
 11 files changed, 81 insertions(+), 11 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 1d0ec5073d..2f2d713732 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -96,6 +96,7 @@
 %define with_sanlock          0
 %define with_numad            0
 %define with_nbdkit           0
+%define with_nbdkit_config_default 0
 %define with_firewalld_zone   0
 %define with_netcf            0
 %define with_libssh2          0
@@ -174,15 +175,17 @@
     %endif
 %endif
 
-# We should only enable nbdkit support if the OS ships a SELinux policy that
-# allows libvirt to launch it. Right now that's not the case anywhere, but
-# things should be fine by the time Fedora 40 is released.
+# We want to build with nbdkit support, but should only enable nbdkit by
+# default if the OS ships a SELinux policy that allows libvirt to launch it.
+# Right now that's not the case anywhere, but things should be fine by the time
+# Fedora 40 is released.
 #
 # TODO: add RHEL 9 once a minor release that contains the necessary SELinux
 #       bits exists (we only support the most recent minor release)
 %if %{with_qemu}
+    %define with_nbdkit 0%{!?_without_nbdkit:1}
     %if 0%{?fedora} >= 40
-        %define with_nbdkit 0%{!?_without_nbdkit:1}
+        %define with_nbdkit_config_default 0%{!?_without_nbdkit_config_default:1}
     %endif
 %endif
 
@@ -1207,6 +1210,12 @@ exit 1
     %define arg_nbdkit -Dnbdkit=disabled
 %endif
 
+%if %{with_nbdkit_config_default}
+    %define arg_nbdkit_config_default -Dnbdkit_config_default=enabled
+%else
+    %define arg_nbdkit_config_default -Dnbdkit_config_default=disabled
+%endif
+
 %if %{with_fuse}
     %define arg_fuse -Dfuse=enabled
 %else
@@ -1322,6 +1331,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec)
            %{?arg_sanlock} \
            -Dlibpcap=enabled \
            %{?arg_nbdkit} \
+           %{?arg_nbdkit_config_default} \
            -Dlibnl=enabled \
            -Daudit=enabled \
            -Ddtrace=enabled \
diff --git a/meson.build b/meson.build
index 4d96b32e58..1e5e1d7954 100644
--- a/meson.build
+++ b/meson.build
@@ -1009,6 +1009,16 @@ if not conf.has('WITH_NBDKIT')
   libnbd_dep = dependency('', required: false)
 endif
 
+# default value for storage_use_nbdkit config option.
+# For now 'auto' just maps to disabled, but in the future it may depend on
+# which security drivers are enabled
+use_nbdkit_default = get_option('nbdkit_config_default').enabled()
+
+if use_nbdkit_default and not conf.has('WITH_NBDKIT')
+  error('nbdkit_config_default requires nbdkit to be enabled')
+endif
+conf.set10('USE_NBDKIT_DEFAULT', use_nbdkit_default)
+
 libnl_version = '3.0'
 if not get_option('libnl').disabled() and host_machine.system() == 'linux'
   libnl_dep = dependency('libnl-3.0', version: '>=' + libnl_version, required: get_option('libnl'))
diff --git a/meson_options.txt b/meson_options.txt
index a0928102bf..182e28b3d1 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -104,7 +104,8 @@ option('loader_nvram', type: 'string', value: '', description: 'Pass list of pai
 option('login_shell', type: 'feature', value: 'auto', description: 'build virt-login-shell')
 option('nss', type: 'feature', value: 'auto', description: 'enable Name Service Switch plugin for resolving guest IP addresses')
 option('numad', type: 'feature', value: 'auto', description: 'use numad to manage CPU placement dynamically')
-option('nbdkit', type: 'feature', value: 'auto', description: 'use nbdkit to access network disks')
+option('nbdkit', type: 'feature', value: 'auto', description: 'Build nbdkit storage backend')
+option('nbdkit_config_default', type: 'feature', value: 'auto', description: 'Whether to use nbdkit storage backend for network disks by default (configurable)')
 option('pm_utils', type: 'feature', value: 'auto', description: 'use pm-utils for power management')
 option('sysctl_config', type: 'feature', value: 'auto', description: 'Whether to install sysctl configs')
 option('tls_priority', type: 'string', value: 'NORMAL', description: 'set the default TLS session priority string')
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index ed097ea3d9..43485b43fb 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -147,6 +147,8 @@ module Libvirtd_qemu =
 
    let capability_filters_entry = str_array_entry "capability_filters"
 
+   let storage_entry = bool_entry "storage_use_nbdkit"
+
    (* Each entry in the config is one of the following ... *)
    let entry = default_tls_entry
              | vnc_entry
@@ -170,6 +172,7 @@ module Libvirtd_qemu =
              | nbd_entry
              | swtpm_entry
              | capability_filters_entry
+             | storage_entry
              | obsolete_entry
 
    let comment = [ label "#comment" . del /#[ \t]*/ "# " .  store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]
diff --git a/src/qemu/meson.build b/src/qemu/meson.build
index 2279fef2ca..4c3e1dee78 100644
--- a/src/qemu/meson.build
+++ b/src/qemu/meson.build
@@ -137,6 +137,7 @@ if conf.has('WITH_QEMU')
   qemu_user_group_conf = configuration_data({
     'QEMU_USER': qemu_user,
     'QEMU_GROUP': qemu_group,
+    'USE_NBDKIT_DEFAULT': use_nbdkit_default.to_int(),
   })
   qemu_conf = configure_file(
     input: 'qemu.conf.in',
@@ -147,6 +148,7 @@ if conf.has('WITH_QEMU')
   qemu_user_group_hack_conf = configuration_data({
     'QEMU_USER': qemu_user,
     'QEMU_GROUP': qemu_group,
+    'USE_NBDKIT_DEFAULT': use_nbdkit_default.to_int(),
     # This hack is necessary because the output file is going to be
     # used as input for another configure_file() call later, which
     # will take care of substituting @CONFIG@ with useful data
diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
index 6897e0f760..34025a02ef 100644
--- a/src/qemu/qemu.conf.in
+++ b/src/qemu/qemu.conf.in
@@ -974,3 +974,14 @@
 # "full"    -  both QEMU and its helper processes are placed into separate
 #              scheduling group
 #sched_core = "none"
+
+# Using nbdkit to access remote disk sources
+#
+# If this is set then libvirt will use nbdkit to access remote disk sources
+# when available. nbdkit will export an NBD share to QEMU rather than having
+# QEMU attempt to access the remote server directly.
+#
+# Possible values are 0 or 1. Default value is @USE_NBDKIT_DEFAULT@. Please
+# note that the default might change in future releases.
+#
+#storage_use_nbdkit = @USE_NBDKIT_DEFAULT@
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 513b5ebb1e..53eec9c43a 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -285,6 +285,7 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool privileged,
         return NULL;
 
     cfg->deprecationBehavior = g_strdup("none");
+    cfg->storageUseNbdkit = USE_NBDKIT_DEFAULT;
 
     return g_steal_pointer(&cfg);
 }
@@ -1065,6 +1066,26 @@ virQEMUDriverConfigLoadCapsFiltersEntry(virQEMUDriverConfig *cfg,
 }
 
 
+static int
+virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
+                                    virConf *conf)
+{
+    if (virConfGetValueBool(conf, "storage_use_nbdkit", &cfg->storageUseNbdkit) < 0)
+        return -1;
+
+#if !WITH_NBDKIT
+    if (cfg->storageUseNbdkit) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       "%s",
+                       _("configuration option 'storage_use_nbdkit' was specified, but nbdkit is not supported by this libvirt"));
+        return -1;
+    }
+#endif /* WITH_NBDKIT */
+
+    return 0;
+}
+
+
 int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
                                 const char *filename,
                                 bool privileged)
@@ -1136,6 +1157,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
     if (virQEMUDriverConfigLoadCapsFiltersEntry(cfg, conf) < 0)
         return -1;
 
+    if (virQEMUDriverConfigLoadStorageEntry(cfg, conf) < 0)
+        return -1;
+
     return 0;
 }
 
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 1a3ba3a0fb..36049b4bfa 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -230,6 +230,8 @@ struct _virQEMUDriverConfig {
 
     char *deprecationBehavior;
 
+    bool storageUseNbdkit;
+
     virQEMUSchedCore schedCore;
 };
 
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 734d63f8a4..e16ba1c225 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10296,6 +10296,9 @@ qemuDomainPrepareStorageSourceNbdkit(virStorageSource *src,
 {
     g_autoptr(qemuNbdkitCaps) nbdkit = NULL;
 
+    if (!cfg->storageUseNbdkit)
+        return false;
+
     if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK)
         return false;
 
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index c730df40b0..e4cfde6cc7 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -117,3 +117,4 @@ module Test_libvirtd_qemu =
 }
 { "deprecation_behavior" = "none" }
 { "sched_core" = "none" }
+{ "storage_use_nbdkit" = "@USE_NBDKIT_DEFAULT@" }
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index b2ea2191dc..74d20b9132 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1125,7 +1125,6 @@ mymain(void)
     DO_TEST_CAPS_LATEST("disk-cdrom-empty-network-invalid");
     DO_TEST_CAPS_LATEST("disk-cdrom-bus-other");
     DO_TEST_CAPS_LATEST("disk-cdrom-network");
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-cdrom-network-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
     DO_TEST_CAPS_LATEST("disk-cdrom-tray");
     DO_TEST_CAPS_LATEST("disk-floppy");
     DO_TEST_CAPS_LATEST("disk-floppy-q35");
@@ -1171,8 +1170,6 @@ mymain(void)
     DO_TEST_CAPS_VER("disk-network-sheepdog", "6.0.0");
     DO_TEST_CAPS_LATEST("disk-network-source-auth");
     DO_TEST_CAPS_LATEST("disk-network-source-curl");
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit-backing", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
     DO_TEST_CAPS_LATEST("disk-network-nfs");
     driver.config->vxhsTLS = 1;
     driver.config->nbdTLSx509secretUUID = g_strdup("6fd3f62d-9fe7-4a4e-a869-7acd6376d8ea");
@@ -1183,13 +1180,10 @@ mymain(void)
     DO_TEST_CAPS_LATEST("disk-network-tlsx509-nbd-hostname");
     DO_TEST_CAPS_VER("disk-network-tlsx509-vxhs", "5.0.0");
     DO_TEST_CAPS_LATEST("disk-network-http");
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-http-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
     VIR_FREE(driver.config->nbdTLSx509secretUUID);
     VIR_FREE(driver.config->vxhsTLSx509secretUUID);
     driver.config->vxhsTLS = 0;
     DO_TEST_CAPS_LATEST("disk-network-ssh");
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-password", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
     DO_TEST_CAPS_LATEST("disk-no-boot");
     DO_TEST_CAPS_LATEST("disk-nvme");
     DO_TEST_CAPS_VER("disk-vhostuser-numa", "4.2.0");
@@ -1259,6 +1253,15 @@ mymain(void)
     DO_TEST_CAPS_LATEST("disk-geometry");
     DO_TEST_CAPS_LATEST("disk-blockio");
 
+    driver.config->storageUseNbdkit = 1;
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-cdrom-network-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit-backing", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-http-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-password", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
+    driver.config->storageUseNbdkit = 0;
+
     DO_TEST_CAPS_VER("disk-virtio-scsi-reservations", "5.2.0");
     DO_TEST_CAPS_LATEST("disk-virtio-scsi-reservations");
 
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, Jan 04, 2024 at 12:24:38 -0600, Jonathon Jongsma wrote:
> Currently when we build with nbdkit support, libvirt will always try to
> use nbdkit to access remote disk sources when it is available. But
> without an up-to-date selinux policy allowing this, it will fail.
> because the required selinux policies are not yet widely available, we
> have disabled nbdkit support on rpm builds for all distributions before
> Fedora 40.
> 
> Unfortunately, this makes it more difficult to test nbdkit support.
> After someone updates to the necessary selinux policies, they would also
> need to rebuild libvirt to enable nbdkit support. By introducing a
> configure option (nbdkit_config_default), we can build packages with
> nbdkit support but have it disabled by default.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> Suggested-by: Andrea Bolognani <abologna@redhat.com>
> ---
> changes in v4
>  - squashed in Andrea's suggested changes
>    - updated error message
>    - Changed one instance of WITH_NDBKIT to WITH_NBDKIT :)
>    - tested

Looks like this broke the upstream CI on centos-stream-8 and mingw-fedoras:

https://gitlab.com/libvirt/libvirt/-/jobs/5864171071

Run-time dependency libnbd found: YES 1.6.0
meson.build:998:4: ERROR: Problem encountered: nbdkit support requires pidfd_open(2)
A full log can be found at /root/rpmbuild/BUILD/libvirt-10.0.0/x86_64-redhat-linux-gnu/meson-logs/meson-log.txt
error: Bad exit status from /var/tmp/rpm-tmp.HkQf7H (%build)
    Bad exit status from /var/tmp/rpm-tmp.HkQf7H (%build)
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, Jan 04, 2024 at 23:39:38 +0100, Peter Krempa wrote:
> On Thu, Jan 04, 2024 at 12:24:38 -0600, Jonathon Jongsma wrote:
> > Currently when we build with nbdkit support, libvirt will always try to
> > use nbdkit to access remote disk sources when it is available. But
> > without an up-to-date selinux policy allowing this, it will fail.
> > because the required selinux policies are not yet widely available, we
> > have disabled nbdkit support on rpm builds for all distributions before
> > Fedora 40.
> > 
> > Unfortunately, this makes it more difficult to test nbdkit support.
> > After someone updates to the necessary selinux policies, they would also
> > need to rebuild libvirt to enable nbdkit support. By introducing a
> > configure option (nbdkit_config_default), we can build packages with
> > nbdkit support but have it disabled by default.
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > Suggested-by: Andrea Bolognani <abologna@redhat.com>
> > ---
> > changes in v4
> >  - squashed in Andrea's suggested changes
> >    - updated error message
> >    - Changed one instance of WITH_NDBKIT to WITH_NBDKIT :)
> >    - tested
> 
> Looks like this broke the upstream CI on centos-stream-8 and mingw-fedoras:
> 
> https://gitlab.com/libvirt/libvirt/-/jobs/5864171071
> 
> Run-time dependency libnbd found: YES 1.6.0
> meson.build:998:4: ERROR: Problem encountered: nbdkit support requires pidfd_open(2)
> A full log can be found at /root/rpmbuild/BUILD/libvirt-10.0.0/x86_64-redhat-linux-gnu/meson-logs/meson-log.txt
> error: Bad exit status from /var/tmp/rpm-tmp.HkQf7H (%build)
>     Bad exit status from /var/tmp/rpm-tmp.HkQf7H (%build)

Okay there are two issues:

- rhel-8 lacks pidfd_open() thus nbdkit support won't work there, and
  thus can't be force-enabled

- mingw builds explicitly disable mingw, thus setting the new config
  option breaks as meson doesn't allow it if it's not used

Patch will be posted soon after CI build succeeds.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Jan 05, 2024 at 09:43:52AM +0100, Peter Krempa wrote:
> On Thu, Jan 04, 2024 at 23:39:38 +0100, Peter Krempa wrote:
> > On Thu, Jan 04, 2024 at 12:24:38 -0600, Jonathon Jongsma wrote:
> > > Currently when we build with nbdkit support, libvirt will always try to
> > > use nbdkit to access remote disk sources when it is available. But
> > > without an up-to-date selinux policy allowing this, it will fail.
> > > because the required selinux policies are not yet widely available, we
> > > have disabled nbdkit support on rpm builds for all distributions before
> > > Fedora 40.
> > >
> > > Unfortunately, this makes it more difficult to test nbdkit support.
> > > After someone updates to the necessary selinux policies, they would also
> > > need to rebuild libvirt to enable nbdkit support. By introducing a
> > > configure option (nbdkit_config_default), we can build packages with
> > > nbdkit support but have it disabled by default.
> > >
> > > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > > Suggested-by: Andrea Bolognani <abologna@redhat.com>
> > > ---
> > > changes in v4
> > >  - squashed in Andrea's suggested changes
> > >    - updated error message
> > >    - Changed one instance of WITH_NDBKIT to WITH_NBDKIT :)
> > >    - tested
> >
> > Looks like this broke the upstream CI on centos-stream-8 and mingw-fedoras:
> >
> > https://gitlab.com/libvirt/libvirt/-/jobs/5864171071
> >
> > Run-time dependency libnbd found: YES 1.6.0
> > meson.build:998:4: ERROR: Problem encountered: nbdkit support requires pidfd_open(2)
> > A full log can be found at /root/rpmbuild/BUILD/libvirt-10.0.0/x86_64-redhat-linux-gnu/meson-logs/meson-log.txt
> > error: Bad exit status from /var/tmp/rpm-tmp.HkQf7H (%build)
> >     Bad exit status from /var/tmp/rpm-tmp.HkQf7H (%build)
>
> Okay there are two issues:
>
> - rhel-8 lacks pidfd_open() thus nbdkit support won't work there, and
>   thus can't be force-enabled
>
> - mingw builds explicitly disable mingw, thus setting the new config
>   option breaks as meson doesn't allow it if it's not used
>
> Patch will be posted soon after CI build succeeds.

I'll wait for your patch instead of investigating myself then :)

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Jan 05, 2024 at 01:30:19 -0800, Andrea Bolognani wrote:
> On Fri, Jan 05, 2024 at 09:43:52AM +0100, Peter Krempa wrote:
> > On Thu, Jan 04, 2024 at 23:39:38 +0100, Peter Krempa wrote:
> > > On Thu, Jan 04, 2024 at 12:24:38 -0600, Jonathon Jongsma wrote:
> > > > Currently when we build with nbdkit support, libvirt will always try to
> > > > use nbdkit to access remote disk sources when it is available. But
> > > > without an up-to-date selinux policy allowing this, it will fail.
> > > > because the required selinux policies are not yet widely available, we
> > > > have disabled nbdkit support on rpm builds for all distributions before
> > > > Fedora 40.
> > > >
> > > > Unfortunately, this makes it more difficult to test nbdkit support.
> > > > After someone updates to the necessary selinux policies, they would also
> > > > need to rebuild libvirt to enable nbdkit support. By introducing a
> > > > configure option (nbdkit_config_default), we can build packages with
> > > > nbdkit support but have it disabled by default.
> > > >
> > > > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > > > Suggested-by: Andrea Bolognani <abologna@redhat.com>
> > > > ---
> > > > changes in v4
> > > >  - squashed in Andrea's suggested changes
> > > >    - updated error message
> > > >    - Changed one instance of WITH_NDBKIT to WITH_NBDKIT :)
> > > >    - tested
> > >
> > > Looks like this broke the upstream CI on centos-stream-8 and mingw-fedoras:
> > >
> > > https://gitlab.com/libvirt/libvirt/-/jobs/5864171071
> > >
> > > Run-time dependency libnbd found: YES 1.6.0
> > > meson.build:998:4: ERROR: Problem encountered: nbdkit support requires pidfd_open(2)
> > > A full log can be found at /root/rpmbuild/BUILD/libvirt-10.0.0/x86_64-redhat-linux-gnu/meson-logs/meson-log.txt
> > > error: Bad exit status from /var/tmp/rpm-tmp.HkQf7H (%build)
> > >     Bad exit status from /var/tmp/rpm-tmp.HkQf7H (%build)
> >
> > Okay there are two issues:
> >
> > - rhel-8 lacks pidfd_open() thus nbdkit support won't work there, and
> >   thus can't be force-enabled
> >
> > - mingw builds explicitly disable mingw, thus setting the new config
> >   option breaks as meson doesn't allow it if it's not used
> >
> > Patch will be posted soon after CI build succeeds.
> 
> I'll wait for your patch instead of investigating myself then :)

So, I've concluded the following:

 - rhel-8 build needs to be disabled, it was enabled explicitly by this
   patch, but the features are not there
 - mingw builds for some unkonwn-to-me reason use
   '--auto-features=enabled' which is weird
 - the logic handling errors in use of the 'nbdkit_config_default' meson
   option is a bit questionable:

    --auto-features=enabled -Dnbdkit=disabled -Dnbdkit_config_default=auto

  should NOT result in a failure even when the basic logic says so

My patch is strictly fixing the build failure by explicitly disabling
nbdkit_config_default on mingw and enabling 'nbdkit' only on rhel>=9.

You can investigate how to improve the questionable logic in
auto-selection of nbdkit_config_default.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Jan 05, 2024 at 10:53:10AM +0100, Peter Krempa wrote:
>  - mingw builds for some unkonwn-to-me reason use
>    '--auto-features=enabled' which is weird
>  - the logic handling errors in use of the 'nbdkit_config_default' meson
>    option is a bit questionable:
>
>     --auto-features=enabled -Dnbdkit=disabled -Dnbdkit_config_default=auto
>
>   should NOT result in a failure even when the basic logic says so
>
> My patch is strictly fixing the build failure by explicitly disabling
> nbdkit_config_default on mingw and enabling 'nbdkit' only on rhel>=9.
>
> You can investigate how to improve the questionable logic in
> auto-selection of nbdkit_config_default.

In the long run, the logic should probably just be "if
nbdkit_config_default is auto, then use the same value as nbdkit".
Right now of course that can't work because we want to keep the
former disabled by default.

The question is, when auto-features=enabled is used, can you really
implement something reasonable? In other words, can you tell

  --auto-features=enabled -Dnbdkit_config_default=auto

and

  -Dnbdkit_config_default=enabled

apart? If that's not the case, auto-features=enabled seems pretty
much completely broken.

Explicitly setting -Dnbdkit_config_default=disabled in the mingw part
of the spec file was the right thing to do anyway, it was just
unfortunately missed. Thankfully, now that we do mingw RPM builds as
part of the CI pipeline, it was caught reasonably early :)

Thank you again for your help!

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Jan 05, 2024 at 02:18:08 -0800, Andrea Bolognani wrote:
> On Fri, Jan 05, 2024 at 10:53:10AM +0100, Peter Krempa wrote:
> >  - mingw builds for some unkonwn-to-me reason use
> >    '--auto-features=enabled' which is weird
> >  - the logic handling errors in use of the 'nbdkit_config_default' meson
> >    option is a bit questionable:
> >
> >     --auto-features=enabled -Dnbdkit=disabled -Dnbdkit_config_default=auto
> >
> >   should NOT result in a failure even when the basic logic says so
> >
> > My patch is strictly fixing the build failure by explicitly disabling
> > nbdkit_config_default on mingw and enabling 'nbdkit' only on rhel>=9.
> >
> > You can investigate how to improve the questionable logic in
> > auto-selection of nbdkit_config_default.
> 
> In the long run, the logic should probably just be "if
> nbdkit_config_default is auto, then use the same value as nbdkit".
> Right now of course that can't work because we want to keep the
> former disabled by default.
> 
> The question is, when auto-features=enabled is used, can you really
> implement something reasonable? In other words, can you tell
> 
>   --auto-features=enabled -Dnbdkit_config_default=auto
> 
> and
> 
>   -Dnbdkit_config_default=enabled
> 
> apart? If that's not the case, auto-features=enabled seems pretty
> much completely broken.
> 
> Explicitly setting -Dnbdkit_config_default=disabled in the mingw part
> of the spec file was the right thing to do anyway, it was just
> unfortunately missed. Thankfully, now that we do mingw RPM builds as
> part of the CI pipeline, it was caught reasonably early :)

A brief look into meson docs and a few tries seem to confirm your
suspicion that auto-features=enabled is an anti-feature. If you declare
an option as a 'feature' in meson_options.txt (a config file with .txt
siuffix?!?!?) then internally it's a UserFeatureOption object and you
have the .enabled(), .disabled(). and .auto() methods at your disposal,
but they operate on the final value after 'auto-features' was applied to
any 'auto' setting. Even newer methods such as '.disable_auto_if()' seem
to apply only when the final setting is auto, so auto-features=enabled
breaks it.

IMO auto-features=enabled shouldn't be used and definitely not in our
spec file.

I can see value in auto-features=disabled though, which IMO should be
preferable also in spec files.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, Jan 04, 2024 at 12:24:38PM -0600, Jonathon Jongsma wrote:
> Currently when we build with nbdkit support, libvirt will always try to
> use nbdkit to access remote disk sources when it is available. But
> without an up-to-date selinux policy allowing this, it will fail.
> because the required selinux policies are not yet widely available, we
> have disabled nbdkit support on rpm builds for all distributions before
> Fedora 40.
>
> Unfortunately, this makes it more difficult to test nbdkit support.
> After someone updates to the necessary selinux policies, they would also
> need to rebuild libvirt to enable nbdkit support. By introducing a
> configure option (nbdkit_config_default), we can build packages with
> nbdkit support but have it disabled by default.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> Suggested-by: Andrea Bolognani <abologna@redhat.com>
> ---
> changes in v4
>  - squashed in Andrea's suggested changes
>    - updated error message
>    - Changed one instance of WITH_NDBKIT to WITH_NBDKIT :)

Nice catch O:-)

> +static int
> +virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
> +                                    virConf *conf)
> +{
> +    if (virConfGetValueBool(conf, "storage_use_nbdkit", &cfg->storageUseNbdkit) < 0)
> +        return -1;
> +
> +#if !WITH_NBDKIT
> +    if (cfg->storageUseNbdkit) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       "%s",
> +                       _("configuration option 'storage_use_nbdkit' was specified, but nbdkit is not supported by this libvirt"));
> +        return -1;
> +    }
> +#endif /* WITH_NBDKIT */

I was about to ACK this directly, but while performing some
last-minute due diligence I realized that unfortunately this approach
comes with its own drawbacks.

Because of socket activation, this failure is not as visible to the
admin as we would like it to be: in particular, if you set the option
and then restart virtqemud, you won't get any error message. The
issue will only really become visible once someone, who might not be
the admin, tries to connect to the driver, and they won't even get a
good error message out of it.

As a consequence, I suggest we adopt a middle of the road behavior:
ignore the option, but at least log a warning that will hopefully
clue in the admin once they realize that nbdkit is not being used
after all and wonder why.

Everything else looks good, so you can consider the patch

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

if you agree with this idea and squash in the diff below. Feel free
to tweak the error message.

Thank you for your patience :)


diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 53eec9c43a..4050a82341 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1075,10 +1075,8 @@
virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,

 #if !WITH_NBDKIT
     if (cfg->storageUseNbdkit) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                       "%s",
-                       _("configuration option 'storage_use_nbdkit'
was specified, but nbdkit is not supported by this libvirt"));
-        return -1;
+        VIR_WARN("Ignoring configuration option 'storage_use_nbdkit':
nbdkit is not supported by this libvirt");
+        cfg->storageUseNbdkit = false;
     }
 #endif /* WITH_NBDKIT */

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On 1/4/24 1:14 PM, Andrea Bolognani wrote:
> On Thu, Jan 04, 2024 at 12:24:38PM -0600, Jonathon Jongsma wrote:
>> Currently when we build with nbdkit support, libvirt will always try to
>> use nbdkit to access remote disk sources when it is available. But
>> without an up-to-date selinux policy allowing this, it will fail.
>> because the required selinux policies are not yet widely available, we
>> have disabled nbdkit support on rpm builds for all distributions before
>> Fedora 40.
>>
>> Unfortunately, this makes it more difficult to test nbdkit support.
>> After someone updates to the necessary selinux policies, they would also
>> need to rebuild libvirt to enable nbdkit support. By introducing a
>> configure option (nbdkit_config_default), we can build packages with
>> nbdkit support but have it disabled by default.
>>
>> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
>> Suggested-by: Andrea Bolognani <abologna@redhat.com>
>> ---
>> changes in v4
>>   - squashed in Andrea's suggested changes
>>     - updated error message
>>     - Changed one instance of WITH_NDBKIT to WITH_NBDKIT :)
> 
> Nice catch O:-)
> 
>> +static int
>> +virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
>> +                                    virConf *conf)
>> +{
>> +    if (virConfGetValueBool(conf, "storage_use_nbdkit", &cfg->storageUseNbdkit) < 0)
>> +        return -1;
>> +
>> +#if !WITH_NBDKIT
>> +    if (cfg->storageUseNbdkit) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                       "%s",
>> +                       _("configuration option 'storage_use_nbdkit' was specified, but nbdkit is not supported by this libvirt"));
>> +        return -1;
>> +    }
>> +#endif /* WITH_NBDKIT */
> 
> I was about to ACK this directly, but while performing some
> last-minute due diligence I realized that unfortunately this approach
> comes with its own drawbacks.
> 
> Because of socket activation, this failure is not as visible to the
> admin as we would like it to be: in particular, if you set the option
> and then restart virtqemud, you won't get any error message. The
> issue will only really become visible once someone, who might not be
> the admin, tries to connect to the driver, and they won't even get a
> good error message out of it.
> 
> As a consequence, I suggest we adopt a middle of the road behavior:
> ignore the option, but at least log a warning that will hopefully
> clue in the admin once they realize that nbdkit is not being used
> after all and wonder why.
> 
> Everything else looks good, so you can consider the patch
> 
>    Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> if you agree with this idea and squash in the diff below. Feel free
> to tweak the error message.

yeah, that's a good point. I've squashed your patch below and pushed it. 
I think the warning message was fine.

Jonathon


> 
> Thank you for your patience :)
> 
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 53eec9c43a..4050a82341 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1075,10 +1075,8 @@
> virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
> 
>   #if !WITH_NBDKIT
>       if (cfg->storageUseNbdkit) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       "%s",
> -                       _("configuration option 'storage_use_nbdkit'
> was specified, but nbdkit is not supported by this libvirt"));
> -        return -1;
> +        VIR_WARN("Ignoring configuration option 'storage_use_nbdkit':
> nbdkit is not supported by this libvirt");
> +        cfg->storageUseNbdkit = false;
>       }
>   #endif /* WITH_NBDKIT */
> 
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org