Re: Re: [libvirt PATCH v3] qemu: add runtime config option for nbdkit
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-le...@lists.libvirt.org
Re: [libvirt PATCH v3] qemu: add runtime config option for nbdkit
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", >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
Re: [libvirt PATCH v3] qemu: add runtime config option for nbdkit
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",
> >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) {
