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>
---
Sorry for the delay in getting out a new version. Thanksgiving break in
the USA, etc.

Changes in v3:
 - nbdkit_config_default changed from a meson 'boolean' to a meson
   'feature' configure option
 - print error message for senseless configurations where nbdkit is
   disabled but nbdkit_config_default is enabled
 - added a comment to example config file about possibility of the
   default value changing in the future
 - don't include the storage_use_nbdkit in the config file if nbdkit
   support is not compiled in. I used the same strategy that is used in
   the remote driver where certain config options are disabled based on
   whether WITH_IP is defined or not. When WITH_NBDKIT is not defined,
   the value is not included in the config file and we don't attempt to
   parse it from the config file either.

 libvirt.spec.in                               | 18 +++++++--
 meson.build                                   | 10 +++++
 meson_options.txt                             |  3 +-
 ...libvirtd_qemu.aug => libvirtd_qemu.aug.in} |  7 ++++
 src/qemu/meson.build                          | 37 ++++++++++++++++++-
 src/qemu/qemu.conf.in                         | 13 +++++++
 src/qemu/qemu_conf.c                          | 19 ++++++++++
 src/qemu/qemu_conf.h                          |  2 +
 src/qemu/qemu_domain.c                        |  3 ++
 tests/qemuxml2argvtest.c                      | 15 +++++---
 10 files changed, 114 insertions(+), 13 deletions(-)
 rename src/qemu/{libvirtd_qemu.aug => libvirtd_qemu.aug.in} (98%)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index f3fdedfde4..7aa497a565 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
 
@@ -1214,6 +1217,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
@@ -1329,6 +1338,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 62481a008d..e63b807f21 100644
--- a/meson.build
+++ b/meson.build
@@ -1012,6 +1012,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..05fd63dd41 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: 'disabled', 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.in
similarity index 98%
rename from src/qemu/libvirtd_qemu.aug
rename to src/qemu/libvirtd_qemu.aug.in
index ed097ea3d9..bcc0aa8938 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug.in
@@ -147,6 +147,10 @@ module Libvirtd_qemu =
 
    let capability_filters_entry = str_array_entry "capability_filters"
 
+@CUT_WITH_NBDKIT@
+   let storage_entry = bool_entry "storage_use_nbdkit"
+
+@END@
    (* Each entry in the config is one of the following ... *)
    let entry = default_tls_entry
              | vnc_entry
@@ -170,6 +174,9 @@ module Libvirtd_qemu =
              | nbd_entry
              | swtpm_entry
              | capability_filters_entry
+@CUT_WITH_NBDKIT@
+             | storage_entry
+@END@
              | 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..43519e2834 100644
--- a/src/qemu/meson.build
+++ b/src/qemu/meson.build
@@ -137,9 +137,41 @@ 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(),
   })
+
+  # preprocess config files to remove items related to features that are not
+  # compiled in
+  preprocess_config_files =  [
+    {
+      'inpath': 'qemu.conf.in',
+      'outpath': 'qemu.conf.tmp',
+      'outvar': 'qemu_conf_tmp',
+    },
+    {
+      'inpath': 'libvirtd_qemu.aug.in',
+      'outpath': 'libvirtd_qemu.aug',
+      'outvar': 'libvirtd_qemu_aug',
+    }
+  ]
+  foreach item : preprocess_config_files
+    if conf.has('WITH_NBDKIT')
+      preprocess_cmd = [ 'sed', '-e', '/[@]CUT_WITH_NBDKIT[@]/d', '-e', '/[@]END[@]/d', '@INPUT@' ]
+    else
+      preprocess_cmd = [ 'sed', '-e', '/[@]CUT_WITH_NBDKIT[@]/,/[@]END[@]/d', '@INPUT@' ]
+    endif
+
+    tmp = configure_file(
+      input: item['inpath'],
+      output: item['outpath'],
+      command: preprocess_cmd,
+      capture: true,
+    )
+    set_variable(item['outvar'], tmp)
+  endforeach
+
   qemu_conf = configure_file(
-    input: 'qemu.conf.in',
+    input: qemu_conf_tmp,
     output: 'qemu.conf',
     configuration: qemu_user_group_conf,
   )
@@ -147,6 +179,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
@@ -159,7 +192,7 @@ if conf.has('WITH_QEMU')
   )
 
   virt_conf_files += qemu_conf
-  virt_aug_files += files('libvirtd_qemu.aug')
+  virt_aug_files += libvirtd_qemu_aug
   virt_test_aug_files += {
     'name': 'test_libvirtd_qemu.aug',
     'aug': test_libvirtd_qemu_aug_tmp,
diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
index 6897e0f760..76633c6a2f 100644
--- a/src/qemu/qemu.conf.in
+++ b/src/qemu/qemu.conf.in
@@ -974,3 +974,16 @@
 # "full"    -  both QEMU and its helper processes are placed into separate
 #              scheduling group
 #sched_core = "none"
+
+@CUT_WITH_NBDKIT@
+# 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@
+@END@
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 513b5ebb1e..e1a1bc3655 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,19 @@ virQEMUDriverConfigLoadCapsFiltersEntry(virQEMUDriverConfig *cfg,
 }
 
 
+#if WITH_NBDKIT
+static int
+virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
+                                    virConf *conf)
+{
+    if (virConfGetValueBool(conf, "storage_use_nbdkit", &cfg->storageUseNbdkit) < 0)
+        return -1;
+
+    return 0;
+}
+#endif /* WITH_NBDKIT */
+
+
 int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
                                 const char *filename,
                                 bool privileged)
@@ -1136,6 +1150,11 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
     if (virQEMUDriverConfigLoadCapsFiltersEntry(cfg, conf) < 0)
         return -1;
 
+#if WITH_NBDKIT
+    if (virQEMUDriverConfigLoadStorageEntry(cfg, conf) < 0)
+        return -1;
+#endif /* WITH_NBDKIT */
+
     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 953808fcfe..0891ddabe9 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/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.41.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, Nov 30, 2023 at 05:04:11PM -0600, Jonathon Jongsma wrote:
> Sorry for the delay in getting out a new version. Thanksgiving break in
> the USA, etc.

... followed by holidays in Europe. We still have a few days to get
this into 10.0.0 though :)

> +++ 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: 'disabled', description: 'Whether to use nbdkit storage backend for network disks by default (configurable)')

You can have 'auto' as the default value here, it's still going to
behave the same as 'disabled' due to the way you've implemented the
meson.build part but it'll appear more consistent with existing
options.

> +++ b/src/qemu/qemu.conf.in
> @@ -974,3 +974,16 @@
>  # "full"    -  both QEMU and its helper processes are placed into separate
>  #              scheduling group
>  #sched_core = "none"
> +
> +@CUT_WITH_NBDKIT@
> +# 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@
> +@END@

I think this is unnecessary. It's okay to leave the comment in there
even if ndbkit support is compiled out. More on this below.

Either way, the line for the option itself needs to look like

  #storage_use_nbdkit = @USE_NBDKIT_DEFAULT@

Note the lack of whitespace between the comment marker and the option
name. That's not just for looks: the augeas test requires this
specific arrangement.

To make the augeas test I've just mentioned work, you also need to
squash in the following:

  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@" }

This demonstrates that augeas is really picking up the value for the
option and interpreting it correctly.

> +++ b/src/qemu/meson.build
> @@ -137,9 +137,41 @@ if conf.has('WITH_QEMU')
> +  # preprocess config files to remove items related to features that are not
> +  # compiled in
> +  preprocess_config_files =  [
> +    {
> +      'inpath': 'qemu.conf.in',
> +      'outpath': 'qemu.conf.tmp',
> +      'outvar': 'qemu_conf_tmp',
> +    },
> +    {
> +      'inpath': 'libvirtd_qemu.aug.in',
> +      'outpath': 'libvirtd_qemu.aug',
> +      'outvar': 'libvirtd_qemu_aug',
> +    }
> +  ]
> +  foreach item : preprocess_config_files
> +    if conf.has('WITH_NBDKIT')
> +      preprocess_cmd = [ 'sed', '-e', '/[@]CUT_WITH_NBDKIT[@]/d', '-e', '/[@]END[@]/d', '@INPUT@' ]
> +    else
> +      preprocess_cmd = [ 'sed', '-e', '/[@]CUT_WITH_NBDKIT[@]/,/[@]END[@]/d', '@INPUT@' ]
> +    endif
> +
> +    tmp = configure_file(
> +      input: item['inpath'],
> +      output: item['outpath'],
> +      command: preprocess_cmd,
> +      capture: true,
> +    )
> +    set_variable(item['outvar'], tmp)
> +  endforeach

Once you stop cutting out the bits of the config file related to
nbdkit when nbdkit support is compiled out, you no longer need any of
this additional complexity.

> +++ b/src/qemu/qemu_conf.c
> @@ -1065,6 +1066,19 @@ virQEMUDriverConfigLoadCapsFiltersEntry(virQEMUDriverConfig *cfg,
>  }
>
>
> +#if WITH_NBDKIT
> +static int
> +virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
> +                                    virConf *conf)
> +{
> +    if (virConfGetValueBool(conf, "storage_use_nbdkit", &cfg->storageUseNbdkit) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +#endif /* WITH_NBDKIT */

When parsing the configuration file, we shouldn't make this part
conditional. If the user tries to enable nbdkit usage with a build of
libvirt that doesn't include support, it's a *good thing* to report
an error rather than silently ignoring that request. Hence, we want
something like

  #if !WITH_NDBKIT
      if (cfg->storageUseNbdkit) {
          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                         "%s",
                         _("ndbkit support is not compiled in"));
          return -1;
      }
  #endif /* WITH_NBDKIT */

here.


Since I'm not 100% sure everything I've explained will come through
successfully (feeling a bit tired right now) I've just pushed a
commit with all the changes I'm suggesting here:

  https://gitlab.com/abologna/libvirt/-/commit/abae4deaa68b9123a59d58b5a0ba2b1b028b784d

-- 
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/3/24 12:37 PM, Andrea Bolognani wrote:
> On Thu, Nov 30, 2023 at 05:04:11PM -0600, Jonathon Jongsma wrote:
>> Sorry for the delay in getting out a new version. Thanksgiving break in
>> the USA, etc.
> 
> ... followed by holidays in Europe. We still have a few days to get
> this into 10.0.0 though :)
> 
>> +++ 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: 'disabled', description: 'Whether to use nbdkit storage backend for network disks by default (configurable)')
> 
> You can have 'auto' as the default value here, it's still going to
> behave the same as 'disabled' due to the way you've implemented the
> meson.build part but it'll appear more consistent with existing
> options.
> 
>> +++ b/src/qemu/qemu.conf.in
>> @@ -974,3 +974,16 @@
>>   # "full"    -  both QEMU and its helper processes are placed into separate
>>   #              scheduling group
>>   #sched_core = "none"
>> +
>> +@CUT_WITH_NBDKIT@
>> +# 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@
>> +@END@
> 
> I think this is unnecessary. It's okay to leave the comment in there
> even if ndbkit support is compiled out. More on this below.

I'm happy to drop all of this stuff. But just by way of explanation, but 
after your comments on v2, I decided to try to mimic the only other 
configuration option in libvirt that could be compiled out: the WITH_IP 
build configuration in the remote driver.

> 
> Either way, the line for the option itself needs to look like
> 
>    #storage_use_nbdkit = @USE_NBDKIT_DEFAULT@
> 
> Note the lack of whitespace between the comment marker and the option
> name. That's not just for looks: the augeas test requires this
> specific arrangement.
> 
> To make the augeas test I've just mentioned work, you also need to
> squash in the following:
> 
>    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@" }
> 
> This demonstrates that augeas is really picking up the value for the
> option and interpreting it correctly.

Oops, thanks for catching that.

> 
>> +++ b/src/qemu/meson.build
>> @@ -137,9 +137,41 @@ if conf.has('WITH_QEMU')
>> +  # preprocess config files to remove items related to features that are not
>> +  # compiled in
>> +  preprocess_config_files =  [
>> +    {
>> +      'inpath': 'qemu.conf.in',
>> +      'outpath': 'qemu.conf.tmp',
>> +      'outvar': 'qemu_conf_tmp',
>> +    },
>> +    {
>> +      'inpath': 'libvirtd_qemu.aug.in',
>> +      'outpath': 'libvirtd_qemu.aug',
>> +      'outvar': 'libvirtd_qemu_aug',
>> +    }
>> +  ]
>> +  foreach item : preprocess_config_files
>> +    if conf.has('WITH_NBDKIT')
>> +      preprocess_cmd = [ 'sed', '-e', '/[@]CUT_WITH_NBDKIT[@]/d', '-e', '/[@]END[@]/d', '@INPUT@' ]
>> +    else
>> +      preprocess_cmd = [ 'sed', '-e', '/[@]CUT_WITH_NBDKIT[@]/,/[@]END[@]/d', '@INPUT@' ]
>> +    endif
>> +
>> +    tmp = configure_file(
>> +      input: item['inpath'],
>> +      output: item['outpath'],
>> +      command: preprocess_cmd,
>> +      capture: true,
>> +    )
>> +    set_variable(item['outvar'], tmp)
>> +  endforeach
> 
> Once you stop cutting out the bits of the config file related to
> nbdkit when nbdkit support is compiled out, you no longer need any of
> this additional complexity.
> 
>> +++ b/src/qemu/qemu_conf.c
>> @@ -1065,6 +1066,19 @@ virQEMUDriverConfigLoadCapsFiltersEntry(virQEMUDriverConfig *cfg,
>>   }
>>
>>
>> +#if WITH_NBDKIT
>> +static int
>> +virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
>> +                                    virConf *conf)
>> +{
>> +    if (virConfGetValueBool(conf, "storage_use_nbdkit", &cfg->storageUseNbdkit) < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +#endif /* WITH_NBDKIT */
> 
> When parsing the configuration file, we shouldn't make this part
> conditional. If the user tries to enable nbdkit usage with a build of
> libvirt that doesn't include support, it's a *good thing* to report
> an error rather than silently ignoring that request. Hence, we want
> something like
> 
>    #if !WITH_NDBKIT
>        if (cfg->storageUseNbdkit) {
>            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                           "%s",
>                           _("ndbkit support is not compiled in"));
>            return -1;
>        }
>    #endif /* WITH_NBDKIT */
> 
> here.

Once again, i modeled it on the WITH_IP configure options. In that case, 
we do not return an error if the WITH_IP configuration options are 
specified when the functionality is compiled out. But I'm happy to do it 
this way as well.

> 
> 
> Since I'm not 100% sure everything I've explained will come through
> successfully (feeling a bit tired right now) I've just pushed a
> commit with all the changes I'm suggesting here:
> 
>    https://gitlab.com/abologna/libvirt/-/commit/abae4deaa68b9123a59d58b5a0ba2b1b028b784d
> 

That commit looks find to me. Do you want to squash it? or would you 
like me to?

Jonathon
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Wed, Jan 03, 2024 at 04:52:00PM -0600, Jonathon Jongsma wrote:
> On 1/3/24 12:37 PM, Andrea Bolognani wrote:
> > When parsing the configuration file, we shouldn't make this part
> > conditional. If the user tries to enable nbdkit usage with a build of
> > libvirt that doesn't include support, it's a *good thing* to report
> > an error rather than silently ignoring that request. Hence, we want
> > something like
> >
> >    #if !WITH_NDBKIT
> >        if (cfg->storageUseNbdkit) {
> >            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> >                           "%s",
> >                           _("ndbkit support is not compiled in"));
> >            return -1;
> >        }
> >    #endif /* WITH_NBDKIT */
> >
> > here.
>
> Once again, i modeled it on the WITH_IP configure options. In that case, we
> do not return an error if the WITH_IP configuration options are specified
> when the functionality is compiled out. But I'm happy to do it this way as
> well.

In that case, however, whether the IP-related options are accepted
does not depend on some choice the user/packager made at build time:
the monolithic daemon will always accept them, the modular ones
won't. There's much less opportunity for confusion. So yeah, it's
similar enough that it makes sense to consider implementing the same
behavior, but IMO different enough that ultimately a separate
handling is preferrable.

> > Since I'm not 100% sure everything I've explained will come through
> > successfully (feeling a bit tired right now) I've just pushed a
> > commit with all the changes I'm suggesting here:
> >
> >    https://gitlab.com/abologna/libvirt/-/commit/abae4deaa68b9123a59d58b5a0ba2b1b028b784d
> >
>
> That commit looks find to me. Do you want to squash it? or would you like me
> to?

Please squash it in, consider maybe writing a better error message
for the "nbdkit not compiled in" case, and post v4 to the list.

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