[PATCH 0/5] virth-host-validate: Couple of cleanups
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
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@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
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@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
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@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
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@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
[...]
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
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@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
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@redhat.com> Jano
On Tue, Jun 8, 2021 at 10:45 AM Michal Privoznik <mprivozn@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@fidencio.org> -- Fabiano Fidêncio
On Tue, Jun 8, 2021 at 9:17 PM Fabiano Fidêncio <fabiano@fidencio.org> wrote:
On Tue, Jun 8, 2021 at 10:45 AM Michal Privoznik <mprivozn@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@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
participants (3)
-
Fabiano Fidêncio -
Jano Tomko -
Michal Privoznik