Re: [PATCH v4] qemu: add runtime config option for nbdkit
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(a)redhat.com>
> Suggested-by: Andrea Bolognani <abologna(a)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(a)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 */