3:45 a.m.
I've noticed couple of bugs/problems while reviewing Fabiano's patch.
Here are fixes.
Michal Prívozník (5):
virt-host-validate: Initialize the error object
virt-host-validate: Report an error if failed to detect CGroups
virt-host-validate: Turn failure to read /proc/cmdline into an error
virt-host-validate: Call VIR_HOST_VALIDATE_FAILURE() more frequently
virHostValidateSecureGuests: Drop useless 'return 0' at the end
tools/virt-host-validate-common.c | 28 +++++++++++++++-------------
tools/virt-host-validate.c | 6 +++++-
2 files changed, 20 insertions(+), 14 deletions(-)
--
2.31.1
3:45 a.m.
New subject: [PATCH 1/5] virt-host-validate: Initialize the error object
Several libvirt functions are called from virt-host-validate.
Some of these functions do report an error on failure. But
reporting an error is coupled with freeing previous error (by
calling virResetError()). But we've never called
virErrorInitialize() and thus resetting error object frees some
random pointer.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virt-host-validate.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/virt-host-validate.c b/tools/virt-host-validate.c
index c119d649ce..806d61bc8e 100644
--- a/tools/virt-host-validate.c
+++ b/tools/virt-host-validate.c
@@ -27,6 +27,7 @@
#include <getopt.h>
#include "internal.h"
+#include "virerror.h"
#include "virgettext.h"
#include "virt-host-validate-common.h"
@@ -83,8 +84,11 @@ main(int argc, char **argv)
bool quiet = false;
bool usedHvname = false;
- if (virGettextInitialize() < 0)
+ if (virGettextInitialize() < 0 ||
+ virErrorInitialize() < 0) {
+ fprintf(stderr, _("%s: initialization failed\n"), argv[0]);
return EXIT_FAILURE;
+ }
while ((c = getopt_long(argc, argv, "hvq", argOptions, NULL)) != -1) {
switch (c) {
--
2.31.1
3:45 a.m.
New subject: [PATCH 2/5] virt-host-validate: Report an error if failed to detect CGroups
As a part of its checks, virt-host-validate calls virCgroupNew()
to detect CGroup controllers which are then printed out. However,
virCgroupNew() can fail (with appropriate error message set).
Let's print an error onto stderr if that happens.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virt-host-validate-common.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
index 9412bb7514..c0cee43409 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -290,8 +290,11 @@ int virHostValidateCGroupControllers(const char *hvname,
int ret = 0;
size_t i;
- if (virCgroupNew("/", -1, &group) < 0)
+ if (virCgroupNew("/", -1, &group) < 0) {
+ fprintf(stderr, "Unable to initialize cgroups: %s\n",
+ virGetLastErrorMessage());
return VIR_HOST_VALIDATE_FAILURE(level);
+ }
for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
int flag = 1 << i;
--
2.31.1
3:45 a.m.
New subject: [PATCH 3/5] virt-host-validate: Turn failure to read /proc/cmdline into an error
When validating secure guests support on s390(x) we may read
/proc/cmdline and look for "prot_virt" argument. Reading the
kernel command line is done via virFileReadValueString() which
may fail. In such case caller won't see any error message. But we
can produce the same warning/error as if "prot_virt" argument
wasn't found. Not only this lets users know about the problem,
it also terminates the "Checking for ...." line correctly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virt-host-validate-common.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
index c0cee43409..4482690b4b 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -470,14 +470,12 @@ int virHostValidateSecureGuests(const char *hvname,
return 0;
}
- if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0)
- return VIR_HOST_VALIDATE_FAILURE(level);
-
/* we're prefix matching rather than equality matching here, because
* kernel would treat even something like prot_virt='yFOO' as
* enabled
*/
- if (virKernelCmdlineMatchParam(cmdline, "prot_virt", kIBMValues,
+ if (virFileReadValueString(&cmdline, "/proc/cmdline") >= 0
&&
+ virKernelCmdlineMatchParam(cmdline, "prot_virt", kIBMValues,
G_N_ELEMENTS(kIBMValues),
VIR_KERNEL_CMDLINE_FLAGS_SEARCH_FIRST |
VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) {
--
2.31.1
3:45 a.m.
New subject: [PATCH 4/5] virt-host-validate: Call VIR_HOST_VALIDATE_FAILURE() more frequently
Ideally, every virHostMsgFail() would be coupled with
VIR_HOST_VALIDATE_FAILURE() so that the failure is correctly
propagated to the caller. However, in
virHostValidateSecureGuests() we are either ignoring @level and
returning 0 directly (no error), or not returning at all, relying
on 'return 0' at the end of the function. Neither of these help
propagate failure correctly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virt-host-validate-common.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
index 4482690b4b..9ec4e6f00b 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -467,7 +467,7 @@ int virHostValidateSecureGuests(const char *hvname,
if (!virFileIsDir("/sys/firmware/uv")) {
virHostMsgFail(level, "IBM Secure Execution not supported by "
"the currently used kernel");
- return 0;
+ return VIR_HOST_VALIDATE_FAILURE(level);
}
/* we're prefix matching rather than equality matching here, because
@@ -486,16 +486,18 @@ int virHostValidateSecureGuests(const char *hvname,
"IBM Secure Execution appears to be disabled "
"in kernel. Add prot_virt=1 to kernel cmdline "
"arguments");
+ return VIR_HOST_VALIDATE_FAILURE(level);
}
} else {
virHostMsgFail(level, "Hardware or firmware does not provide "
"support for IBM Secure Execution");
+ return VIR_HOST_VALIDATE_FAILURE(level);
}
} else if (hasAMDSev) {
if (virFileReadValueString(&mod_value,
"/sys/module/kvm_amd/parameters/sev") < 0) {
virHostMsgFail(level, "AMD Secure Encrypted Virtualization not "
"supported by the currently used kernel");
- return 0;
+ return VIR_HOST_VALIDATE_FAILURE(level);
}
if (mod_value[0] != '1') {
@@ -503,7 +505,7 @@ int virHostValidateSecureGuests(const char *hvname,
"AMD Secure Encrypted Virtualization appears to be
"
"disabled in kernel. Add kvm_amd.sev=1 "
"to the kernel cmdline arguments");
- return 0;
+ return VIR_HOST_VALIDATE_FAILURE(level);
}
if (virFileExists("/dev/sev")) {
@@ -513,6 +515,7 @@ int virHostValidateSecureGuests(const char *hvname,
virHostMsgFail(level,
"AMD Secure Encrypted Virtualization appears to be
"
"disabled in firemare.");
+ return VIR_HOST_VALIDATE_FAILURE(level);
}
} else {
virHostMsgFail(level,
--
2.31.1
2:17 p.m.
New subject: [PATCH 4/5] virt-host-validate: Call VIR_HOST_VALIDATE_FAILURE() more frequently
[...]
if (virFileExists("/dev/sev")) {
@@ -513,6 +515,7 @@ int virHostValidateSecureGuests(const char *hvname,
virHostMsgFail(level,
"AMD Secure Encrypted Virtualization appears to be
"
"disabled in firemare.");
Not related to this series, but: firemare -> firmware.
+ return VIR_HOST_VALIDATE_FAILURE(level);
}
} else {
virHostMsgFail(level,
--
2.31.1
Best Regards,
--
Fabiano Fidêncio
3:45 a.m.
New subject: [PATCH 5/5] virHostValidateSecureGuests: Drop useless 'return 0' at the end
Previous patches rendered 'return 0' at the end of the function a
dead code. Therefore, the code can be rearranged a bit and the
line can be dropped.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virt-host-validate-common.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
index 9ec4e6f00b..8ce45609e7 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -517,11 +517,9 @@ int virHostValidateSecureGuests(const char *hvname,
"disabled in firemare.");
return VIR_HOST_VALIDATE_FAILURE(level);
}
- } else {
- virHostMsgFail(level,
- "Unknown if this platform has Secure Guest support");
- return VIR_HOST_VALIDATE_FAILURE(level);
}
- return 0;
+ virHostMsgFail(level,
+ "Unknown if this platform has Secure Guest support");
+ return VIR_HOST_VALIDATE_FAILURE(level);
}
--
2.31.1
9:55 a.m.
On 6/8/21 10:45 AM, Michal Privoznik wrote:
I've noticed couple of bugs/problems while reviewing
Fabiano's patch.
Here are fixes.
Michal Prívozník (5):
virt-host-validate: Initialize the error object
virt-host-validate: Report an error if failed to detect CGroups
virt-host-validate: Turn failure to read /proc/cmdline into an error
virt-host-validate: Call VIR_HOST_VALIDATE_FAILURE() more frequently
virHostValidateSecureGuests: Drop useless 'return 0' at the end
tools/virt-host-validate-common.c | 28 +++++++++++++++-------------
tools/virt-host-validate.c | 6 +++++-
2 files changed, 20 insertions(+), 14 deletions(-)
Series:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
2:17 p.m.
On Tue, Jun 8, 2021 at 10:45 AM Michal Privoznik <mprivozn(a)redhat.com> wrote:
I've noticed couple of bugs/problems while reviewing Fabiano's patch.
Here are fixes.
Michal Prívozník (5):
virt-host-validate: Initialize the error object
virt-host-validate: Report an error if failed to detect CGroups
virt-host-validate: Turn failure to read /proc/cmdline into an error
virt-host-validate: Call VIR_HOST_VALIDATE_FAILURE() more frequently
virHostValidateSecureGuests: Drop useless 'return 0' at the end
tools/virt-host-validate-common.c | 28 +++++++++++++++-------------
tools/virt-host-validate.c | 6 +++++-
2 files changed, 20 insertions(+), 14 deletions(-)
--
2.31.1
Series:
Reviewed-by: Fabiano Fidêncio <fabiano(a)fidencio.org>
--
Fabiano Fidêncio
3:18 p.m.
On Tue, Jun 8, 2021 at 9:17 PM Fabiano Fidêncio <fabiano(a)fidencio.org> wrote:
On Tue, Jun 8, 2021 at 10:45 AM Michal Privoznik <mprivozn(a)redhat.com> wrote:
>
> I've noticed couple of bugs/problems while reviewing Fabiano's patch.
> Here are fixes.
>
> Michal Prívozník (5):
> virt-host-validate: Initialize the error object
> virt-host-validate: Report an error if failed to detect CGroups
> virt-host-validate: Turn failure to read /proc/cmdline into an error
> virt-host-validate: Call VIR_HOST_VALIDATE_FAILURE() more frequently
> virHostValidateSecureGuests: Drop useless 'return 0' at the end
>
> tools/virt-host-validate-common.c | 28 +++++++++++++++-------------
> tools/virt-host-validate.c | 6 +++++-
> 2 files changed, 20 insertions(+), 14 deletions(-)
>
> --
> 2.31.1
>
Series:
Reviewed-by: Fabiano Fidêncio <fabiano(a)fidencio.org>
--
Fabiano Fidêncio
And while I have your attention, please, also consider:
https://listman.redhat.com/archives/libvir-list/2021-June/msg00214.html
I wrote that one as a follow-up to your series as it was just easier, sorry.
Best Regards,
--
Fabiano Fidêncio
1266
days inactive
1266
days old
9 comments
3 participants
participants (3)
-
Fabiano Fidêncio -
Jano Tomko -
Michal Privoznik