[PATCH] tools: only fail validations if VIR_HOST_VALIDATE_FAIL is set
Currently `virt-host-validate` will fail whenever one of its calls fail, regardless of virHostValidateLevel set. This behaviour is not optimal and makes it not exactly reliable as a command line tool as other tools or scripts using it would have to check its output to figure out whether something really failed or if a warning was mistakenly treated as failure. With this change, the behaviour of whether to fail or not, is defined by the caller of those functions, based on the virHostValidateLevel passed to them. Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- tools/virt-host-validate-common.c | 129 ++++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 35 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 6dd851f07d..2bf97bad75 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -138,15 +138,21 @@ int virHostValidateDeviceExists(const char *hvname, virHostValidateLevel level, const char *hint) { + int ret = 0; + virHostMsgCheck(hvname, "if device %s exists", dev_name); if (access(dev_name, F_OK) < 0) { virHostMsgFail(level, "%s", hint); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; } virHostMsgPass(); - return 0; + + out: + return ret; } @@ -155,15 +161,21 @@ int virHostValidateDeviceAccessible(const char *hvname, virHostValidateLevel level, const char *hint) { + int ret = 0; + virHostMsgCheck(hvname, "if device %s is accessible", dev_name); if (access(dev_name, R_OK|W_OK) < 0) { virHostMsgFail(level, "%s", hint); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; } virHostMsgPass(); - return 0; + + out: + return ret; } @@ -173,6 +185,7 @@ int virHostValidateNamespace(const char *hvname, const char *hint) { char nspath[100]; + int ret = 0; virHostMsgCheck(hvname, "for namespace %s", ns_name); @@ -180,11 +193,15 @@ int virHostValidateNamespace(const char *hvname, if (access(nspath, F_OK) < 0) { virHostMsgFail(level, "%s", hint); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; } virHostMsgPass(); - return 0; + + out: + return ret; } @@ -254,6 +271,7 @@ int virHostValidateLinuxKernel(const char *hvname, { struct utsname uts; unsigned long thisversion; + int ret = 0; uname(&uts); @@ -264,21 +282,29 @@ int virHostValidateLinuxKernel(const char *hvname, if (STRNEQ(uts.sysname, "Linux")) { virHostMsgFail(level, "%s", hint); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; } if (virParseVersionString(uts.release, &thisversion, true) < 0) { virHostMsgFail(level, "%s", hint); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; } if (thisversion < version) { virHostMsgFail(level, "%s", hint); - return -1; - } else { - virHostMsgPass(); - return 0; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; } + + virHostMsgPass(); + + out: + return ret; } #ifdef __linux__ @@ -290,8 +316,11 @@ int virHostValidateCGroupControllers(const char *hvname, int ret = 0; size_t i; - if (virCgroupNew("/", -1, &group) < 0) - return -1; + if (virCgroupNew("/", -1, &group) < 0) { + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; + } for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { int flag = 1 << i; @@ -303,7 +332,8 @@ int virHostValidateCGroupControllers(const char *hvname, virHostMsgCheck(hvname, "for cgroup '%s' controller support", cg_name); if (!virCgroupHasController(group, i)) { - ret = -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; virHostMsgFail(level, "Enable '%s' in kernel Kconfig file or " "mount/enable cgroup controller in your system", cg_name); @@ -312,6 +342,7 @@ int virHostValidateCGroupControllers(const char *hvname, } } + out: return ret; } #else /* !__linux__ */ @@ -319,8 +350,13 @@ int virHostValidateCGroupControllers(const char *hvname G_GNUC_UNUSED, int controllers G_GNUC_UNUSED, virHostValidateLevel level) { + int ret = 0; + virHostMsgFail(level, "%s", "This platform does not support cgroups"); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + + return ret; } #endif /* !__linux__ */ @@ -334,6 +370,7 @@ int virHostValidateIOMMU(const char *hvname, virArch arch = virArchFromHost(); struct dirent *dent; int rc; + int ret = 0; flags = virHostValidateGetCPUFlags(); @@ -354,7 +391,9 @@ int virHostValidateIOMMU(const char *hvname, "No ACPI DMAR table found, IOMMU either " "disabled in BIOS or not supported by this " "hardware platform"); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; } } else if (isAMD) { virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU support")); @@ -366,7 +405,9 @@ int virHostValidateIOMMU(const char *hvname, "No ACPI IVRS table found, IOMMU either " "disabled in BIOS or not supported by this " "hardware platform"); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; } } else if (ARCH_IS_PPC64(arch)) { /* Empty Block */ @@ -378,23 +419,25 @@ int virHostValidateIOMMU(const char *hvname, * no PCI devices the directory is still there but is * empty. */ if (!virDirOpen(&dir, "/sys/bus/pci/devices")) - return 0; + goto out; rc = virDirRead(dir, &dent, NULL); if (rc <= 0) - return 0; + goto out; } else { virHostMsgFail(level, "Unknown if this platform has IOMMU support"); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; } /* We can only check on newer kernels with iommu groups & vfio */ if (stat("/sys/kernel/iommu_groups", &sb) < 0) - return 0; + goto out; if (!S_ISDIR(sb.st_mode)) - return 0; + goto out; virHostMsgCheck(hvname, "%s", _("if IOMMU is enabled by kernel")); if (sb.st_nlink <= 2) { @@ -404,10 +447,16 @@ int virHostValidateIOMMU(const char *hvname, "Add %s to kernel cmdline arguments", bootarg); else virHostMsgFail(level, "IOMMU capability not compiled into kernel."); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; + } + virHostMsgPass(); - return 0; + + out: + return ret; } @@ -448,6 +497,7 @@ int virHostValidateSecureGuests(const char *hvname, g_autofree char *cmdline = NULL; static const char *kIBMValues[] = {"y", "Y", "on", "ON", "oN", "On", "1"}; g_autofree char *mod_value = NULL; + int ret = 0; flags = virHostValidateGetCPUFlags(); @@ -464,12 +514,15 @@ int virHostValidateSecureGuests(const char *hvname, if (!virFileIsDir("/sys/firmware/uv")) { virHostMsgFail(level, "IBM Secure Execution not supported by " "the currently used kernel"); - return 0; + goto out; } - if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) - return -1; - + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) { + if (level == VIR_HOST_VALIDATE_FAIL) { + ret = -1; + goto out; + } + } /* we're prefix matching rather than equality matching here, because * kernel would treat even something like prot_virt='yFOO' as * enabled @@ -479,7 +532,8 @@ int virHostValidateSecureGuests(const char *hvname, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_FIRST | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) { virHostMsgPass(); - return 1; + ret = 1; + goto out; } else { virHostMsgFail(level, "IBM Secure Execution appears to be disabled " @@ -494,7 +548,7 @@ int virHostValidateSecureGuests(const char *hvname, 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; + goto out; } if (mod_value[0] != '1') { @@ -502,12 +556,13 @@ 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; + goto out; } if (virFileExists("/dev/sev")) { virHostMsgPass(); - return 1; + ret = 1; + goto out; } else { virHostMsgFail(level, "AMD Secure Encrypted Virtualization appears to be " @@ -516,8 +571,12 @@ int virHostValidateSecureGuests(const char *hvname, } else { virHostMsgFail(level, "Unknown if this platform has Secure Guest support"); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; + } - return 0; + out: + return ret; } -- 2.31.1
[...]
@@ -464,12 +514,15 @@ int virHostValidateSecureGuests(const char *hvname, if (!virFileIsDir("/sys/firmware/uv")) { virHostMsgFail(level, "IBM Secure Execution not supported by " "the currently used kernel"); - return 0; + goto out; }
- if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) - return -1; - + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) { + if (level == VIR_HOST_VALIDATE_FAIL) { + ret = -1; + goto out;
Oops, this `goto out;` should be out of the if scope, sorry. I'll fix this and re-submit a v2 after I get some reviews on this version. [...] Best Regards, -- Fabiano Fidêncio
On 6/6/21 12:15 PM, Fabiano Fidêncio wrote:
Currently `virt-host-validate` will fail whenever one of its calls fail, regardless of virHostValidateLevel set.
This behaviour is not optimal and makes it not exactly reliable as a command line tool as other tools or scripts using it would have to check its output to figure out whether something really failed or if a warning was mistakenly treated as failure.
With this change, the behaviour of whether to fail or not, is defined by the caller of those functions, based on the virHostValidateLevel passed to them.
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- tools/virt-host-validate-common.c | 129 ++++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 35 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 6dd851f07d..2bf97bad75 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -138,15 +138,21 @@ int virHostValidateDeviceExists(const char *hvname, virHostValidateLevel level, const char *hint) { + int ret = 0; + virHostMsgCheck(hvname, "if device %s exists", dev_name);
if (access(dev_name, F_OK) < 0) { virHostMsgFail(level, "%s", hint); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; }
virHostMsgPass(); - return 0; + + out: + return ret; }
The patch, or idea it implements is correct. However, I think a lot of these 'out' labels can be dropped and 'goto out' can be replaced with 'return -1' or 'return 0'. Does that work for you? Michal
On Mon, Jun 7, 2021 at 10:45 AM Michal Prívozník <mprivozn@redhat.com> wrote:
On 6/6/21 12:15 PM, Fabiano Fidêncio wrote:
Currently `virt-host-validate` will fail whenever one of its calls fail, regardless of virHostValidateLevel set.
This behaviour is not optimal and makes it not exactly reliable as a command line tool as other tools or scripts using it would have to check its output to figure out whether something really failed or if a warning was mistakenly treated as failure.
With this change, the behaviour of whether to fail or not, is defined by the caller of those functions, based on the virHostValidateLevel passed to them.
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- tools/virt-host-validate-common.c | 129 ++++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 35 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 6dd851f07d..2bf97bad75 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -138,15 +138,21 @@ int virHostValidateDeviceExists(const char *hvname, virHostValidateLevel level, const char *hint) { + int ret = 0; + virHostMsgCheck(hvname, "if device %s exists", dev_name);
if (access(dev_name, F_OK) < 0) { virHostMsgFail(level, "%s", hint); - return -1; + if (level == VIR_HOST_VALIDATE_FAIL) + ret = -1; + goto out; }
virHostMsgPass(); - return 0; + + out: + return ret; }
The patch, or idea it implements is correct. However, I think a lot of these 'out' labels can be dropped and 'goto out' can be replaced with 'return -1' or 'return 0'. Does that work for you?
Sure, I'll submit a v2 later Today addressing those. Thanks a whole lot for the timely review! Best Regards, -- Fabiano Fidêncio
participants (2)
-
Fabiano Fidêncio -
Michal Prívozník