m4/virt-loader-nvram.m4 | 10 +++++++++- src/qemu/qemu.conf | 3 +++ src/qemu/qemu_conf.c | 19 +++++++++++++++++-- 3 files changed, 29 insertions(+), 3 deletions(-)
There are two ways for specifying loader:nvram pairs:
1) --with-loader-nvram configure option
2) nvram variable in qemu.conf
Since we have FW descriptors, using this old style is
discouraged, but not as strong as one would expect. Produce more
warnings:
1) produce a warning if somebody tries the configure option
2) produce a warning if somebody sets nvram variable and at
least on FW descriptor was found
The reason for producing warning in case 1) is that package
maintainers, who set the configure option in the first place
should start moving towards FW descriptors and abandon the
configure option. After all, the warning is printed into config
output only in this case.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1763477
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
m4/virt-loader-nvram.m4 | 10 +++++++++-
src/qemu/qemu.conf | 3 +++
src/qemu/qemu_conf.c | 19 +++++++++++++++++--
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/m4/virt-loader-nvram.m4 b/m4/virt-loader-nvram.m4
index d7e0c8ca18..ed2ae0cf27 100644
--- a/m4/virt-loader-nvram.m4
+++ b/m4/virt-loader-nvram.m4
@@ -30,6 +30,8 @@ AC_DEFUN([LIBVIRT_CHECK_LOADER_NVRAM], [
l=$(echo $with_loader_nvram | tr ':' '\n' | wc -l)
if test $(expr $l % 2) -ne 0 ; then
AC_MSG_ERROR([Malformed --with-loader-nvram argument])
+ elif test $l -gt 0 ; then
+ AC_MSG_WARN([Note that --with-loader-nvram is obsolete and will be removed soon])
fi
AC_DEFINE_UNQUOTED([DEFAULT_LOADER_NVRAM], ["$with_loader_nvram"],
[List of loader:nvram pairs])
@@ -37,5 +39,11 @@ AC_DEFUN([LIBVIRT_CHECK_LOADER_NVRAM], [
])
AC_DEFUN([LIBVIRT_RESULT_LOADER_NVRAM], [
- LIBVIRT_RESULT([Loader/NVRAM], [$with_loader_nvram])
+ if test "x$with_loader_nvram" != "xno" && \
+ test "x$with_loader_nvram" != "x" ; then
+ LIBVIRT_RESULT([Loader/NVRAM], [$with_loader_nvram],
+ [!!! Using this configure option is strongly discouraged !!!])
+ else
+ LIBVIRT_RESULT([Loader/NVRAM], [$with_loader_nvram])
+ fi
])
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index b3a3428e4c..7a056b037e 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -761,6 +761,9 @@
# source tree. These metadata files are distributed alongside any
# firmware images intended for use with QEMU.
#
+# NOTE: if ANY firmware metadata files are detected, this setting
+# will be COMPLETELY IGNORED.
+#
# ------------------------------------------
#
# When a domain is configured to use UEFI instead of standard
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index fae697a2ef..293f2635cc 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -32,6 +32,7 @@
#include "qemu_conf.h"
#include "qemu_capabilities.h"
#include "qemu_domain.h"
+#include "qemu_firmware.h"
#include "qemu_security.h"
#include "viruuid.h"
#include "virbuffer.h"
@@ -799,7 +800,8 @@ virQEMUDriverConfigLoadLogEntry(virQEMUDriverConfigPtr cfg,
static int
virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg,
- virConfPtr conf)
+ virConfPtr conf,
+ bool privileged)
{
VIR_AUTOSTRINGLIST nvram = NULL;
size_t i;
@@ -807,8 +809,21 @@ virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg,
if (virConfGetValueStringList(conf, "nvram", false, &nvram) < 0)
return -1;
if (nvram) {
+ VIR_AUTOSTRINGLIST fwList = NULL;
+
virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares);
+ if (qemuFirmwareFetchConfigs(&fwList, privileged) < 0)
+ return -1;
+
+ if (fwList) {
+ VIR_WARN("Obsolete nvram variable is set while firmware metadata "
+ "files found. Note that the nvram config file variable is "
+ "going to be ignored.");
+ cfg->nfirmwares = 0;
+ return 0;
+ }
+
cfg->nfirmwares = virStringListLength((const char *const *)nvram);
if (nvram[0] && VIR_ALLOC_N(cfg->firmwares, cfg->nfirmwares) < 0)
return -1;
@@ -1041,7 +1056,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (virQEMUDriverConfigLoadLogEntry(cfg, conf) < 0)
return -1;
- if (virQEMUDriverConfigLoadNVRAMEntry(cfg, conf) < 0)
+ if (virQEMUDriverConfigLoadNVRAMEntry(cfg, conf, privileged) < 0)
return -1;
if (virQEMUDriverConfigLoadGlusterDebugEntry(cfg, conf) < 0)
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 11, 2019 at 05:42:17PM +0100, Michal Privoznik wrote: > There are two ways for specifying loader:nvram pairs: > > 1) --with-loader-nvram configure option > 2) nvram variable in qemu.conf > > Since we have FW descriptors, using this old style is > discouraged, but not as strong as one would expect. Produce more > warnings: > > 1) produce a warning if somebody tries the configure option > 2) produce a warning if somebody sets nvram variable and at > least on FW descriptor was found > > The reason for producing warning in case 1) is that package > maintainers, who set the configure option in the first place > should start moving towards FW descriptors and abandon the > configure option. After all, the warning is printed into config > output only in this case. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1763477 > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > m4/virt-loader-nvram.m4 | 10 +++++++++- > src/qemu/qemu.conf | 3 +++ > src/qemu/qemu_conf.c | 19 +++++++++++++++++-- > 3 files changed, 29 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/11/19 9:42 AM, Michal Privoznik wrote:
> There are two ways for specifying loader:nvram pairs:
>
> 1) --with-loader-nvram configure option
> 2) nvram variable in qemu.conf
>
> Since we have FW descriptors, using this old style is
> discouraged, but not as strong as one would expect. Produce more
> warnings:
Oh, I didn't know the old style was discouraged when using FW descriptors.
Thanks for mentioning it!
>
> 1) produce a warning if somebody tries the configure option
> 2) produce a warning if somebody sets nvram variable and at
> least on FW descriptor was found
>
> The reason for producing warning in case 1) is that package
> maintainers, who set the configure option in the first place
> should start moving towards FW descriptors and abandon the
> configure option. After all, the warning is printed into config
> output only in this case.
Should the configure option be removed from the upstream spec file?
Regards,
Jim
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1763477
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> m4/virt-loader-nvram.m4 | 10 +++++++++-
> src/qemu/qemu.conf | 3 +++
> src/qemu/qemu_conf.c | 19 +++++++++++++++++--
> 3 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/m4/virt-loader-nvram.m4 b/m4/virt-loader-nvram.m4
> index d7e0c8ca18..ed2ae0cf27 100644
> --- a/m4/virt-loader-nvram.m4
> +++ b/m4/virt-loader-nvram.m4
> @@ -30,6 +30,8 @@ AC_DEFUN([LIBVIRT_CHECK_LOADER_NVRAM], [
> l=$(echo $with_loader_nvram | tr ':' '\n' | wc -l)
> if test $(expr $l % 2) -ne 0 ; then
> AC_MSG_ERROR([Malformed --with-loader-nvram argument])
> + elif test $l -gt 0 ; then
> + AC_MSG_WARN([Note that --with-loader-nvram is obsolete and will be removed soon])
> fi
> AC_DEFINE_UNQUOTED([DEFAULT_LOADER_NVRAM], ["$with_loader_nvram"],
> [List of loader:nvram pairs])
> @@ -37,5 +39,11 @@ AC_DEFUN([LIBVIRT_CHECK_LOADER_NVRAM], [
> ])
>
> AC_DEFUN([LIBVIRT_RESULT_LOADER_NVRAM], [
> - LIBVIRT_RESULT([Loader/NVRAM], [$with_loader_nvram])
> + if test "x$with_loader_nvram" != "xno" && \
> + test "x$with_loader_nvram" != "x" ; then
> + LIBVIRT_RESULT([Loader/NVRAM], [$with_loader_nvram],
> + [!!! Using this configure option is strongly discouraged !!!])
> + else
> + LIBVIRT_RESULT([Loader/NVRAM], [$with_loader_nvram])
> + fi
> ])
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index b3a3428e4c..7a056b037e 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -761,6 +761,9 @@
> # source tree. These metadata files are distributed alongside any
> # firmware images intended for use with QEMU.
> #
> +# NOTE: if ANY firmware metadata files are detected, this setting
> +# will be COMPLETELY IGNORED.
> +#
> # ------------------------------------------
> #
> # When a domain is configured to use UEFI instead of standard
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index fae697a2ef..293f2635cc 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -32,6 +32,7 @@
> #include "qemu_conf.h"
> #include "qemu_capabilities.h"
> #include "qemu_domain.h"
> +#include "qemu_firmware.h"
> #include "qemu_security.h"
> #include "viruuid.h"
> #include "virbuffer.h"
> @@ -799,7 +800,8 @@ virQEMUDriverConfigLoadLogEntry(virQEMUDriverConfigPtr cfg,
>
> static int
> virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg,
> - virConfPtr conf)
> + virConfPtr conf,
> + bool privileged)
> {
> VIR_AUTOSTRINGLIST nvram = NULL;
> size_t i;
> @@ -807,8 +809,21 @@ virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg,
> if (virConfGetValueStringList(conf, "nvram", false, &nvram) < 0)
> return -1;
> if (nvram) {
> + VIR_AUTOSTRINGLIST fwList = NULL;
> +
> virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares);
>
> + if (qemuFirmwareFetchConfigs(&fwList, privileged) < 0)
> + return -1;
> +
> + if (fwList) {
> + VIR_WARN("Obsolete nvram variable is set while firmware metadata "
> + "files found. Note that the nvram config file variable is "
> + "going to be ignored.");
> + cfg->nfirmwares = 0;
> + return 0;
> + }
> +
> cfg->nfirmwares = virStringListLength((const char *const *)nvram);
> if (nvram[0] && VIR_ALLOC_N(cfg->firmwares, cfg->nfirmwares) < 0)
> return -1;
> @@ -1041,7 +1056,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
> if (virQEMUDriverConfigLoadLogEntry(cfg, conf) < 0)
> return -1;
>
> - if (virQEMUDriverConfigLoadNVRAMEntry(cfg, conf) < 0)
> + if (virQEMUDriverConfigLoadNVRAMEntry(cfg, conf, privileged) < 0)
> return -1;
>
> if (virQEMUDriverConfigLoadGlusterDebugEntry(cfg, conf) < 0)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/12/19 11:17 PM, Jim Fehlig wrote: > On 11/11/19 9:42 AM, Michal Privoznik wrote: >> There are two ways for specifying loader:nvram pairs: >> >> 1) --with-loader-nvram configure option >> 2) nvram variable in qemu.conf >> >> Since we have FW descriptors, using this old style is >> discouraged, but not as strong as one would expect. Produce more >> warnings: > > Oh, I didn't know the old style was discouraged when using FW descriptors. > Thanks for mentioning it! > >> >> 1) produce a warning if somebody tries the configure option >> 2) produce a warning if somebody sets nvram variable and at >> least on FW descriptor was found >> >> The reason for producing warning in case 1) is that package >> maintainers, who set the configure option in the first place >> should start moving towards FW descriptors and abandon the >> configure option. After all, the warning is printed into config >> output only in this case. > > Should the configure option be removed from the upstream spec file? Definitely, Fedora ships FW descriptors and so does RHEL. What's the state in OpenSUSE? But I bet you have your own spec file anyway, don't you? Just like we have for Fedora and RHEL. Will post a patch tomorrow (unless you beat me to it). Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/13/19 9:20 AM, Michal Privoznik wrote: > On 11/12/19 11:17 PM, Jim Fehlig wrote: >> On 11/11/19 9:42 AM, Michal Privoznik wrote: >>> There are two ways for specifying loader:nvram pairs: >>> >>> 1) --with-loader-nvram configure option >>> 2) nvram variable in qemu.conf >>> >>> Since we have FW descriptors, using this old style is >>> discouraged, but not as strong as one would expect. Produce more >>> warnings: >> >> Oh, I didn't know the old style was discouraged when using FW descriptors. >> Thanks for mentioning it! >> >>> >>> 1) produce a warning if somebody tries the configure option >>> 2) produce a warning if somebody sets nvram variable and at >>> least on FW descriptor was found >>> >>> The reason for producing warning in case 1) is that package >>> maintainers, who set the configure option in the first place >>> should start moving towards FW descriptors and abandon the >>> configure option. After all, the warning is printed into config >>> output only in this case. >> >> Should the configure option be removed from the upstream spec file? > > Definitely, Fedora ships FW descriptors and so does RHEL. What's the state in > OpenSUSE? But I bet you have your own spec file anyway, don't you? Just like we > have for Fedora and RHEL. Will post a patch tomorrow (unless you beat me to it). I was attempting to beat you to it, but removing $LOADERS from the spec file means removing use of --with-loader-nvram configure option. Does that mean we should remove associated code from configure.ac, nuke m4/virt-loader-nvram.m4, remove DEFAULT_LOADER_NVRAM from qemu and libxl drivers, ...? It seems a bit early for that since you are just now adding the deprecation warning to virt-loader-nvram.m4 :-). Should we wait until completely removing support for the build-time FW list before nuking from the spec file? Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/13/19 7:59 PM, Jim Fehlig wrote: > On 11/13/19 9:20 AM, Michal Privoznik wrote: >> On 11/12/19 11:17 PM, Jim Fehlig wrote: >>> On 11/11/19 9:42 AM, Michal Privoznik wrote: >>>> There are two ways for specifying loader:nvram pairs: >>>> >>>> 1) --with-loader-nvram configure option >>>> 2) nvram variable in qemu.conf >>>> >>>> Since we have FW descriptors, using this old style is >>>> discouraged, but not as strong as one would expect. Produce more >>>> warnings: >>> >>> Oh, I didn't know the old style was discouraged when using FW descriptors. >>> Thanks for mentioning it! >>> >>>> >>>> 1) produce a warning if somebody tries the configure option >>>> 2) produce a warning if somebody sets nvram variable and at >>>> least on FW descriptor was found >>>> >>>> The reason for producing warning in case 1) is that package >>>> maintainers, who set the configure option in the first place >>>> should start moving towards FW descriptors and abandon the >>>> configure option. After all, the warning is printed into config >>>> output only in this case. >>> >>> Should the configure option be removed from the upstream spec file? >> >> Definitely, Fedora ships FW descriptors and so does RHEL. What's the state in >> OpenSUSE? But I bet you have your own spec file anyway, don't you? Just like we >> have for Fedora and RHEL. Will post a patch tomorrow (unless you beat me to it). > > I was attempting to beat you to it, but removing $LOADERS from the spec file > means removing use of --with-loader-nvram configure option. Does that mean we > should remove associated code from configure.ac, nuke m4/virt-loader-nvram.m4, > remove DEFAULT_LOADER_NVRAM from qemu and libxl drivers, ...? It seems a bit > early for that since you are just now adding the deprecation warning to > virt-loader-nvram.m4 :-). Should we wait until completely removing support for > the build-time FW list before nuking from the spec file? I think it's safe to remove the configure argument from the spec file, but at this point I'd rather wait and give distros a while to adapt to FW descriptors before removing the argument even from configure script. I mean, I can see us removing the argument and nvram=[] from qemu.conf at the same time, but not right now. Anyway, I'll let you write the patch since it looks like you already have it. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 14, 2019 at 03:26:33PM +0100, Michal Privoznik wrote: > On 11/13/19 7:59 PM, Jim Fehlig wrote: > > On 11/13/19 9:20 AM, Michal Privoznik wrote: > > > On 11/12/19 11:17 PM, Jim Fehlig wrote: > > > > On 11/11/19 9:42 AM, Michal Privoznik wrote: > > > > > There are two ways for specifying loader:nvram pairs: > > > > > > > > > > 1) --with-loader-nvram configure option > > > > > 2) nvram variable in qemu.conf > > > > > > > > > > Since we have FW descriptors, using this old style is > > > > > discouraged, but not as strong as one would expect. Produce more > > > > > warnings: > > > > > > > > Oh, I didn't know the old style was discouraged when using FW descriptors. > > > > Thanks for mentioning it! > > > > > > > > > > > > > > 1) produce a warning if somebody tries the configure option > > > > > 2) produce a warning if somebody sets nvram variable and at > > > > > least on FW descriptor was found > > > > > > > > > > The reason for producing warning in case 1) is that package > > > > > maintainers, who set the configure option in the first place > > > > > should start moving towards FW descriptors and abandon the > > > > > configure option. After all, the warning is printed into config > > > > > output only in this case. > > > > > > > > Should the configure option be removed from the upstream spec file? > > > > > > Definitely, Fedora ships FW descriptors and so does RHEL. What's the state in > > > OpenSUSE? But I bet you have your own spec file anyway, don't you? Just like we > > > have for Fedora and RHEL. Will post a patch tomorrow (unless you beat me to it). > > > > I was attempting to beat you to it, but removing $LOADERS from the spec file > > means removing use of --with-loader-nvram configure option. Does that mean we > > should remove associated code from configure.ac, nuke m4/virt-loader-nvram.m4, > > remove DEFAULT_LOADER_NVRAM from qemu and libxl drivers, ...? It seems a bit > > early for that since you are just now adding the deprecation warning to > > virt-loader-nvram.m4 :-). Should we wait until completely removing support for > > the build-time FW list before nuking from the spec file? > > I think it's safe to remove the configure argument from the spec file, but > at this point I'd rather wait and give distros a while to adapt to FW > descriptors before removing the argument even from configure script. I mean, > I can see us removing the argument and nvram=[] from qemu.conf at the same > time, but not right now. As long as we aim to support QEMU versions (aka our QEMU min version in caps probing) that pre-date the FW descriptor spec, we shouldn't remove support for this from our configure script. Removing it from the RPM spec is fine since thd spec assumes recent Fedora or RHEL only which both have FW descriptor support. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/13/19 9:20 AM, Michal Privoznik wrote: > On 11/12/19 11:17 PM, Jim Fehlig wrote: >> On 11/11/19 9:42 AM, Michal Privoznik wrote: >>> There are two ways for specifying loader:nvram pairs: >>> >>> 1) --with-loader-nvram configure option >>> 2) nvram variable in qemu.conf >>> >>> Since we have FW descriptors, using this old style is >>> discouraged, but not as strong as one would expect. Produce more >>> warnings: >> >> Oh, I didn't know the old style was discouraged when using FW descriptors. >> Thanks for mentioning it! >> >>> >>> 1) produce a warning if somebody tries the configure option >>> 2) produce a warning if somebody sets nvram variable and at >>> least on FW descriptor was found >>> >>> The reason for producing warning in case 1) is that package >>> maintainers, who set the configure option in the first place >>> should start moving towards FW descriptors and abandon the >>> configure option. After all, the warning is printed into config >>> output only in this case. >> >> Should the configure option be removed from the upstream spec file? > > Definitely, Fedora ships FW descriptors and so does RHEL. What's the state in > OpenSUSE? Yes, openSUSE supports FW descriptors, as does the latest version of SLES under development (SLES15 SP2). > But I bet you have your own spec file anyway, don't you? Just like we > have for Fedora and RHEL. Will post a patch tomorrow (unless you beat me to it). Yes, I have my own spec file, heavily influenced by the upstream one :-). Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.