[libvirt] [PATCH v2 0/6] rewrite virt-host-validate to be data driven, using Go & YAML

This is a followup to a previous PoC patch I submitted a month ago: https://www.redhat.com/archives/libvir-list/2019-September/msg00036.html The commit messages in the individual patches given quite a bit of detail, so I'll keep this cover letter brief. In my previous posting I was unhappy with the implications for the RPM packaging, and was considering having this as a separate source repo & RPM. On further investigation such an approach would not in fact solve the RPM packaging problem, because we would still not be using a pure go build toolchain, as we have data files that need installing in the right place. This forced me to actually address the RPM packaging problems that Fedora had with Go when used from a build tool like make or meson. After alot of debugging I finally got a viable solution merged into the Fedora go-rpm-macros package: https://pagure.io/go-rpm-macros/c/67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2 commit 67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Wed Sep 18 16:49:58 2019 +0100 macros: define a %gobuildflags macro Using the %gobuild macro is fine for a project where the go code is the only thing being built, and can be built directly by invoking the Go toolchain from RPM. In more complex cases though, the Go code is just a small part of the project and the Go toolchain is invoked by a build system such as make (possibly automake), or meson. In such a case we need to be able to tell this build system what flags to pass to the compiler. The %gobuildflags macros services this purpose allowing a RPM spec todo GOBUILDFLAGS="%gobuildflags" %configure or %make GOBUILDFLAGS="%gobuildflags" Ideally the %gobuild macro would in turn reference the %gobuildflags macro, but that does not appear possible given the semantics around quote expansion and escaping across RPM and shell. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> As a result in this series, we're now fully integrated into the RPM build, on Fedora at least. I've not checked what approach RHEL takes for Go, whether it requires separate RPM for each 3rd party dep, or prefers bundling. Either way though, we can deal with the problem now. The other obvious change is that this is now a patch series, to make it easier to review the code in managable chunks. The really big difference though is that I replaced the use of XML data files with YAML data files. This was done with the aim of making the data more human friendly. XML is really optimized for machines, not humans, so writing the data files was not pretty. YAML is optimized for human readability, and is actually even easier to consume in Go than the XML was, so its a double win. Finally, we also add new checks at the end for the various CPU hardware side channel mitigations, and report whether SMT/HT is unsafe or not (any Intel host is basically unsafe before Icelake). Daniel P. Berrangé (6): build: introduce logic for using golang in libvirt tools: introduce a data driven impl of virt-host-validate tools: define YAML rules for virt-host-validate checks tools: switch to build the new virt-host-validate impl tools: delete the old virt-host-validate impl tools: make virt-host-validate check CPU vulnerabilities configure.ac | 1 + libvirt.spec.in | 35 +- m4/virt-golang.m4 | 46 ++ m4/virt-host-validate.m4 | 8 +- po/POTFILES | 5 - tools/Makefile.am | 76 +-- tools/host-validate/go.mod | 10 + tools/host-validate/go.sum | 9 + tools/host-validate/main.go | 98 +++ tools/host-validate/pkg/engine.go | 481 ++++++++++++++ tools/host-validate/pkg/facts.go | 585 ++++++++++++++++++ .../pkg/facts_test.go} | 36 +- tools/host-validate/rules/builtin.yaml | 20 + tools/host-validate/rules/cpu.yaml | 50 ++ tools/host-validate/rules/freebsd-kernel.yaml | 77 +++ tools/host-validate/rules/linux-acpi.yaml | 39 ++ tools/host-validate/rules/linux-cgroups.yaml | 470 ++++++++++++++ .../rules/linux-cpu-hardware-flaws.yaml | 165 +++++ tools/host-validate/rules/linux-cpu.yaml | 134 ++++ tools/host-validate/rules/linux-devices.yaml | 71 +++ tools/host-validate/rules/linux-iommu.yaml | 113 ++++ .../host-validate/rules/linux-namespaces.yaml | 119 ++++ tools/host-validate/rules/linux-pci.yaml | 10 + tools/virt-host-validate-bhyve.c | 77 --- tools/virt-host-validate-common.c | 419 ------------- tools/virt-host-validate-common.h | 85 --- tools/virt-host-validate-lxc.c | 87 --- tools/virt-host-validate-lxc.h | 24 - tools/virt-host-validate-qemu.c | 116 ---- tools/virt-host-validate-qemu.h | 24 - tools/virt-host-validate.c | 152 ----- tools/virt-host-validate.pod | 12 +- 32 files changed, 2609 insertions(+), 1045 deletions(-) create mode 100644 m4/virt-golang.m4 create mode 100644 tools/host-validate/go.mod create mode 100644 tools/host-validate/go.sum create mode 100644 tools/host-validate/main.go create mode 100644 tools/host-validate/pkg/engine.go create mode 100644 tools/host-validate/pkg/facts.go rename tools/{virt-host-validate-bhyve.h => host-validate/pkg/facts_test.go} (52%) create mode 100644 tools/host-validate/rules/builtin.yaml create mode 100644 tools/host-validate/rules/cpu.yaml create mode 100644 tools/host-validate/rules/freebsd-kernel.yaml create mode 100644 tools/host-validate/rules/linux-acpi.yaml create mode 100644 tools/host-validate/rules/linux-cgroups.yaml create mode 100644 tools/host-validate/rules/linux-cpu-hardware-flaws.yaml create mode 100644 tools/host-validate/rules/linux-cpu.yaml create mode 100644 tools/host-validate/rules/linux-devices.yaml create mode 100644 tools/host-validate/rules/linux-iommu.yaml create mode 100644 tools/host-validate/rules/linux-namespaces.yaml create mode 100644 tools/host-validate/rules/linux-pci.yaml delete mode 100644 tools/virt-host-validate-bhyve.c delete mode 100644 tools/virt-host-validate-common.c delete mode 100644 tools/virt-host-validate-common.h delete mode 100644 tools/virt-host-validate-lxc.c delete mode 100644 tools/virt-host-validate-lxc.h delete mode 100644 tools/virt-host-validate-qemu.c delete mode 100644 tools/virt-host-validate-qemu.h delete mode 100644 tools/virt-host-validate.c -- 2.21.0

This decides on requiring Golang >= 1.11, since that introduces the new standard concept of "Go modules": https://blog.golang.org/using-go-modules Previously such a concept was dealt with by any number of external 3rd party. These didn't seemlessly integrate into the go toolchain in the way modules do. The particularly attractive benefits of Go modules are - You are /not/ forced to use a particular directory hierarchy that matches your package import name. - Independent of any global $GOPATH which may or may not be set in your user sesion. - External dependencies do not need vendoring (copying) into the libvirt source tree. The toolchain downloads (with local caching) as needed, or can be pointed to a local OS distro provided copy Right now Fedora's Go packaging does not support Go modules, and obviously doesn't dynamically download packages during build. Thus in the RPM we need to turn off the normal Go module support and point it at the local copies of deps, provided by RPMs. This is still static linking, but without the deps being bundled in libvirt's src.rpm. Fedora also does not yet have the %gobuildflags macro defined, so in the short term we must define the desired Go build flags to ensure debuginfo RPMs work. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- configure.ac | 1 + libvirt.spec.in | 19 ++++++++++++++++++- m4/virt-golang.m4 | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 m4/virt-golang.m4 diff --git a/configure.ac b/configure.ac index af8cbcdfd8..b835a10698 100644 --- a/configure.ac +++ b/configure.ac @@ -511,6 +511,7 @@ LIBVIRT_ARG_TLS_PRIORITY LIBVIRT_ARG_SYSCTL_CONFIG +LIBVIRT_CHECK_GOLANG LIBVIRT_CHECK_DEBUG LIBVIRT_CHECK_DTRACE LIBVIRT_CHECK_NUMAD diff --git a/libvirt.spec.in b/libvirt.spec.in index 7f5183f341..1c74dbb252 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -401,6 +401,8 @@ BuildRequires: libtirpc-devel BuildRequires: firewalld-filesystem %endif +BuildRequires: golang >= 1.11 + Provides: bundled(gnulib) %description @@ -1161,6 +1163,21 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/%{name}.spec) autoreconf -if %endif +# We're building without go modules enabled, so +# must make a local Go root with the old style +# dir naming scheme/hierarchy +mkdir -p gocode/src/libvirt.org + +export GO111MODULE=off +export GOPATH=/usr/share/gocode:`pwd`/gocode + +%if 0%{?fedora} >= 32 +GOBUILDFLAGS="%gobuildflags" +%else +GOBUILDFLAGS="-buildmode pie -compiler gc -tags=\"rpm_crashtraceback ${BUILDTAGS:-}\" -ldflags \"${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -extldflags '-Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld '\" -a -v -x" +%endif +export GOBUILDFLAGS + rm -f po/stamp-po %configure --with-runstatedir=%{_rundir} \ %{?arg_qemu} \ @@ -1224,7 +1241,7 @@ rm -f po/stamp-po --enable-expensive-tests \ --with-init-script=systemd \ %{?arg_login_shell} -make %{?_smp_mflags} V=1 +make %{?_smp_mflags} V=1 GOBUILDFLAGS="$GOBUILDFLAGS" %install rm -fr %{buildroot} diff --git a/m4/virt-golang.m4 b/m4/virt-golang.m4 new file mode 100644 index 0000000000..2e39bdac7d --- /dev/null +++ b/m4/virt-golang.m4 @@ -0,0 +1,46 @@ +dnl Golang checks +dnl +dnl Copyright (C) 2019 Red Hat, Inc. +dnl +dnl This library is free software; you can redistribute it and/or +dnl modify it under the terms of the GNU Lesser General Public +dnl License as published by the Free Software Foundation; either +dnl version 2.1 of the License, or (at your option) any later version. +dnl +dnl This library is distributed in the hope that it will be useful, +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +dnl Lesser General Public License for more details. +dnl +dnl You should have received a copy of the GNU Lesser General Public +dnl License along with this library. If not, see +dnl <http://www.gnu.org/licenses/>. +dnl + +AC_DEFUN([LIBVIRT_CHECK_GOLANG], [ + AC_PATH_PROGS([GO], [go], [no]) + if test "x$ac_cv_path_GO" != "xno" + then + with_go=yes + else + with_go=no + fi + AM_CONDITIONAL([HAVE_GOLANG], [test "$with_go" != "no"]) + + if test "$with_go" != "no" + then + GOVERSION=`$ac_cv_path_GO version | awk '{print \$ 3}' | sed -e 's/go//' -e 's/rc.*//'` + GOMAJOR=`echo $GOVERSION | awk -F . '{print \$ 1}'` + GOMINOR=`echo $GOVERSION | awk -F . '{print \$ 2}'` + + dnl We want to use go modules which first arrived in 1.11 + AC_MSG_CHECKING([for Go version >= 1.11]) + if test "$GOMAJOR" != "1" || test "$GOMINOR" -lt "11" + then + with_go=no + AC_MSG_RESULT([no]) + else + AC_MSG_RESULT([yes]) + fi + fi +]) -- 2.21.0

The current virt-host-validate command has a bunch of checks defined in the source code which are thus only extensible by the upstream project, or downstream code modification. The checks are implemented by a fairly simple set of rules, mostly matching the contents of files, or output from commands, against some expected state or regex. This lends itself very well to having the checks defined in data, in the form of rules which can be processed by a generic engine. This patch introduces an implementation of virt-host-validate which does exactly that, using rules defined by a set of YAML files. Parsing XML/JSON/YAML/etc from C is incredibly tedious, which has long discouraged this conversion. The size & fagility of the parsing code in C would be too high to justify the benefits of a rewrite. This new impl is thus written in Go to take advantage of its YAML encoding feature that lets you simply annotate struct fields to define an YAML parser. YAML was chosen instead of XML because the file format is more user readable, than XML, and the Go parsing code is even simpler than with XML. JSON would have been another valid option instead of YAML. JSON is said to be optimized for the simplest machine parsing at the cost of human readability. YAML is said to be optimized for human readability at the cost of more complex parsers. Interestingly YAML is a true superset of JSON so although we used a YAML parser, our code can in fact load JSON files anyway. Comparing the JSON & YAML files for some example rules, the YAML did indeed appear more readable. The new impl also has a few new features in its CLI interface. It is possible display a list of all facts that are set, instead of just the subset which have reports associated with them. For example, by default $ virt-host-validate Checking cgroup memory controller present...PASS Checking cgroup memory controller mounted...PASS Checking cgroup cpu controller present...PASS Checking cgroup cpu controller mounted...PASS Checking cgroup cpuacct controller present...PASS Checking cgroup cpuacct controller mounted...PASS ...snip... But it can be run with $ virt-host-validate -q -f Set fact 'libvirt.driver.qemu' = 'true' Set fact 'libvirt.driver.lxc' = 'true' Set fact 'libvirt.driver.parallels' = 'true' Set fact 'cpu.arch' = 'x86_64' Set fact 'os.kernel' = 'Linux' Set fact 'os.release' = '5.1.16-300.fc30.x86_64' Set fact 'os.version' = '#1 SMP Wed Jul 3 15:06:51 UTC 2019' Set fact 'os.cgroup.controller.cpuset' = 'true' Set fact 'os.cgroup.controller.cpu' = 'true' Set fact 'os.cgroup.controller.cpuacct' = 'true' ...snip... This is quite useful for generating reports to include on bug reports / support requests, as it provides much more detailed information than is included in the summary messages. The main thing lost with this new impl is support for translation, which covers two sources: - Reports associated with the facts defined in the YAML. It is unclear how we would best deal with this. Merge translations from the .po file into the .yaml files, and then ensure we pick the appropriate one to display. - Error messages when things go wrong in the code itself. This would need to use some gettext like translation system for golang. I have not investigated the options here yet This impl uses three 3rd party dependancies - github.com/spf13/pflag - replacement command line arg parsing. Go's standard CLI arg parsing uses single dash for long options and does not support short options as a distinct concept. This is a widely used drop-in replacement that does the traditional long/short opt processing. - golang.org/x/sys - this is actually distributed with Golang but is considered an optional part of the runtime, since its APIs are not platform portable. This is needed to check file access permissions and to extract uname data. - github.com/ghodss/yaml - this provides the YAML parsing API. The observant reviewer will notice the facts.go file contains 'json:' attribute annotations. This is because the YAML parser actually leverages the JSON parser internally. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/host-validate/go.mod | 10 + tools/host-validate/go.sum | 9 + tools/host-validate/main.go | 98 +++++ tools/host-validate/pkg/engine.go | 481 +++++++++++++++++++++ tools/host-validate/pkg/facts.go | 585 ++++++++++++++++++++++++++ tools/host-validate/pkg/facts_test.go | 50 +++ 6 files changed, 1233 insertions(+) create mode 100644 tools/host-validate/go.mod create mode 100644 tools/host-validate/go.sum create mode 100644 tools/host-validate/main.go create mode 100644 tools/host-validate/pkg/engine.go create mode 100644 tools/host-validate/pkg/facts.go create mode 100644 tools/host-validate/pkg/facts_test.go diff --git a/tools/host-validate/go.mod b/tools/host-validate/go.mod new file mode 100644 index 0000000000..fe4d56441d --- /dev/null +++ b/tools/host-validate/go.mod @@ -0,0 +1,10 @@ +module libvirt.org/host-validate + +go 1.11 + +require ( + github.com/ghodss/yaml v1.0.0 + github.com/spf13/pflag v1.0.3 + golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a + gopkg.in/yaml.v2 v2.2.2 // indirect +) diff --git a/tools/host-validate/go.sum b/tools/host-validate/go.sum new file mode 100644 index 0000000000..8f74f6a45f --- /dev/null +++ b/tools/host-validate/go.sum @@ -0,0 +1,9 @@ +github.com/ghodss/yaml v1.0.0 h1:wQHKEahhL6wmXdzwWG11gIVCkOv05bNOh+Rxn0yngAk= +github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04= +github.com/spf13/pflag v1.0.3 h1:zPAT6CGy6wXeQ7NtTnaTerfKOsV6V6F8agHXFiazDkg= +github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= +golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a h1:aYOabOQFp6Vj6W1F80affTUvO9UxmJRx8K0gsfABByQ= +golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw= +gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= diff --git a/tools/host-validate/main.go b/tools/host-validate/main.go new file mode 100644 index 0000000000..f6dae86558 --- /dev/null +++ b/tools/host-validate/main.go @@ -0,0 +1,98 @@ +/* + * This file is part of the libvirt project + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Copyright (C) 2019 Red Hat, Inc. + * + */ + +package main + +import ( + "flag" + "fmt" + "github.com/spf13/pflag" + "io/ioutil" + vl "libvirt.org/host-validate/pkg" + "os" + "path/filepath" + "strings" +) + +func main() { + var showfacts bool + var quiet bool + var rulesdir string + + pflag.BoolVarP(&showfacts, "show-facts", "f", false, "Show raw fact names and values") + pflag.BoolVarP(&quiet, "quiet", "q", false, "Don't report on fact checks") + pflag.StringVarP(&rulesdir, "rules-dir", "r", "/usr/share/libvirt/host-validate", "Directory to load validation rules from") + + pflag.CommandLine.AddGoFlagSet(flag.CommandLine) + pflag.Parse() + // Convince glog that we really have parsed CLI + flag.CommandLine.Parse([]string{}) + + if len(pflag.Args()) > 1 { + fmt.Printf("syntax: %s [OPTIONS] [DRIVER]\n", os.Args[0]) + os.Exit(1) + } + + driver := "" + if len(pflag.Args()) == 1 { + driver = pflag.Args()[0] + } + + files, err := ioutil.ReadDir(rulesdir) + if err != nil { + fmt.Printf("Unable to load rules from '%s': %s\n", rulesdir, err) + os.Exit(1) + } + var lists []vl.FactList + for _, file := range files { + path := filepath.Join(rulesdir, file.Name()) + if !strings.HasSuffix(path, ".yaml") { + continue + } + facts, err := vl.NewFactList(path) + if err != nil { + fmt.Printf("Unable to load facts '%s': %s\n", path, err) + os.Exit(1) + } + lists = append(lists, *facts) + } + + var output vl.EngineOutput + if !quiet { + output |= vl.ENGINE_OUTPUT_REPORTS + } + if showfacts { + output |= vl.ENGINE_OUTPUT_FACTS + } + + engine := vl.NewEngine(output, driver) + + failed, err := engine.Validate(vl.MergeFactLists(lists)) + if err != nil { + fmt.Printf("Unable to validate facts: %s\n", err) + os.Exit(1) + } + if failed != 0 { + os.Exit(2) + } + + os.Exit(0) +} diff --git a/tools/host-validate/pkg/engine.go b/tools/host-validate/pkg/engine.go new file mode 100644 index 0000000000..61676cdce8 --- /dev/null +++ b/tools/host-validate/pkg/engine.go @@ -0,0 +1,481 @@ +/* + * This file is part of the libvirt project + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Copyright (C) 2019 Red Hat, Inc. + * + */ + +package pkg + +// This defines the logic that executes the checks defined +// for each fact that is loaded. It is responsible for printing +// out messages as the facts are processed too. + +import ( + "fmt" + "golang.org/x/sys/unix" + "io/ioutil" + "os" + "os/exec" + "regexp" + "runtime" + "strings" +) + +type Engine struct { + Facts map[string]string + Errors uint + Output EngineOutput + Driver string +} + +type EngineOutput int + +const ( + // Print the raw key, value pairs for each fact set + ENGINE_OUTPUT_FACTS = EngineOutput(1 << 0) + + // Print the human targetted reports for facts set + ENGINE_OUTPUT_REPORTS = EngineOutput(1 << 1) +) + +// Create an engine able to process a list of facts +func NewEngine(output EngineOutput, driver string) *Engine { + engine := &Engine{} + + engine.Output = output + engine.Facts = make(map[string]string) + engine.Driver = driver + + return engine +} + +// Set the value associated with a fact +func (engine *Engine) SetFact(name, value string) { + engine.Facts[name] = value + if (engine.Output & ENGINE_OUTPUT_FACTS) != 0 { + fmt.Printf("Set fact '%s' = '%s'\n", name, value) + } +} + +func (engine *Engine) EvalExpression(expr *Expression) (bool, error) { + if expr.Any != nil { + for _, subexpr := range expr.Any.Expressions { + ok, err := engine.EvalExpression(&subexpr) + if err != nil { + return false, err + } + if ok { + return true, nil + } + } + return false, nil + } else if expr.All != nil { + for _, subexpr := range expr.All.Expressions { + ok, err := engine.EvalExpression(&subexpr) + if err != nil { + return false, err + } + if !ok { + return false, nil + } + } + return true, nil + } else if expr.Fact != nil { + val, ok := engine.Facts[expr.Fact.Name] + if !ok { + return false, nil + } + if expr.Fact.Match == "regex" { + match, err := regexp.Match(expr.Fact.Value, []byte(val)) + if err != nil { + return false, err + } + return match, nil + } else if expr.Fact.Match == "exists" { + return true, nil + } else { + return val == expr.Fact.Value, nil + } + } else { + return false, fmt.Errorf("Expected expression any or all or fact") + } +} + +// Report a fact that failed to have the desired value +func (engine *Engine) Fail(fact *Fact) { + engine.Errors++ + if fact.Report == nil { + return + } + if (engine.Output & ENGINE_OUTPUT_REPORTS) != 0 { + hint := "" + if fact.Hint != nil { + hint = " (" + fact.Hint.Message + ")" + } + if fact.Report.Level == "note" { + fmt.Printf("\033[34mNOTE\033[0m%s\n", hint) + } else if fact.Report.Level == "warn" { + fmt.Printf("\033[33mWARN\033[0m%s\n", hint) + } else { + fmt.Printf("\033[31mFAIL\033[0m%s\n", hint) + } + } +} + +// Report a fact that has the desired value +func (engine *Engine) Pass(fact *Fact) { + if fact.Report == nil { + return + } + if (engine.Output & ENGINE_OUTPUT_REPORTS) != 0 { + fmt.Printf("\033[32mPASS\033[0m\n") + } +} + +func utsString(v []byte) string { + n := 0 + for i, _ := range v { + if v[i] == 0 { + break + } + n++ + } + return string(v[0:n]) +} + +// Populate the engine with values for a built-in fact +func (engine *Engine) SetValueBuiltIn(fact *Fact) error { + var uts unix.Utsname + err := unix.Uname(&uts) + if err != nil { + return err + } + + if fact.Name == "os.kernel" { + engine.SetFact(fact.Name, utsString(uts.Sysname[0:])) + } else if fact.Name == "os.release" { + engine.SetFact(fact.Name, utsString(uts.Release[0:])) + } else if fact.Name == "os.version" { + engine.SetFact(fact.Name, utsString(uts.Version[0:])) + } else if fact.Name == "cpu.arch" { + engine.SetFact(fact.Name, utsString(uts.Machine[0:])) + } else if fact.Name == "libvirt.driver" { + if engine.Driver != "" { + engine.SetFact(fact.Name+"."+engine.Driver, "true") + } else { + if runtime.GOOS == "linux" { + engine.SetFact(fact.Name+".qemu", "true") + engine.SetFact(fact.Name+".lxc", "true") + engine.SetFact(fact.Name+".parallels", "true") + } else if runtime.GOOS == "freebsd" { + engine.SetFact(fact.Name+".bhyve", "true") + } + } + } else { + return fmt.Errorf("Unknown built-in fact '%s'", fact.Name) + } + + return nil +} + +func (engine *Engine) SetValueBool(fact *Fact) error { + ok, err := engine.EvalExpression(fact.Value.Bool) + if err != nil { + return err + } + got := "true" + want := "true" + if !ok { + got = "false" + } + if fact.Report != nil && fact.Report.Pass != "" { + want = fact.Report.Pass + } + engine.SetFact(fact.Name, got) + if got == want { + engine.Pass(fact) + } else { + engine.Fail(fact) + } + return nil +} + +func unescape(val string) (string, error) { + escapes := map[rune]string{ + 'a': "\x07", + 'b': "\x08", + 'e': "\x1b", + 'f': "\x0c", + 'n': "\x0a", + 'r': "\x0d", + 't': "\x09", + 'v': "\x0b", + '\\': "\x5c", + '0': "\x00", + } + var ret string + escape := false + for _, c := range val { + if c == '\\' { + escape = true + } else if escape { + unesc, ok := escapes[c] + if !ok { + return "", fmt.Errorf("Unknown escape '\\%c'", c) + } + ret += string(unesc) + escape = false + } else { + ret += string(c) + } + } + return ret, nil +} + +func (engine *Engine) SetValueParse(fact *Fact, parse *Parse, context string, val string) error { + if parse == nil { + engine.SetFact(context, val) + return nil + } + if parse.Whitespace == "trim" { + val = strings.TrimSpace(val) + } + if parse.Scalar != nil { + if parse.Scalar.Regex != "" { + re, err := regexp.Compile(parse.Scalar.Regex) + if err != nil { + return err + } + matches := re.FindStringSubmatch(val) + if parse.Scalar.Match >= uint(len(matches)) { + val = "" + } else { + val = matches[parse.Scalar.Match] + } + } + engine.SetFact(context, val) + } else if parse.List != nil { + if val == "" { + return nil + } + sep, err := unescape(parse.List.Separator) + if err != nil { + return err + } + bits := strings.Split(val, sep) + count := uint(0) + for i, bit := range bits { + if i < int(parse.List.SkipHead) { + continue + } + if i >= (len(bits) - int(parse.List.SkipTail)) { + continue + } + subcontext := fmt.Sprintf("%s.%d", context, i) + err := engine.SetValueParse(fact, parse.List.Parse, subcontext, bit) + if err != nil { + return err + } + count++ + if count >= parse.List.Limit { + break + } + } + } else if parse.Set != nil { + if val == "" { + return nil + } + sep, err := unescape(parse.Set.Separator) + if err != nil { + return err + } + bits := strings.Split(val, sep) + for i, bit := range bits { + if i < int(parse.Set.SkipHead) { + continue + } + if i >= (len(bits) - int(parse.Set.SkipTail)) { + continue + } + if parse.Set.Regex != "" { + re, err := regexp.Compile(parse.Set.Regex) + if err != nil { + return err + } + matches := re.FindStringSubmatch(bit) + if parse.Set.Match >= uint(len(matches)) { + bit = "" + } else { + bit = matches[parse.Set.Match] + } + } + subcontext := fmt.Sprintf("%s.%s", context, bit) + engine.SetFact(subcontext, "true") + } + } else if parse.Dict != nil { + sep, err := unescape(parse.Dict.Separator) + if err != nil { + return err + } + dlm, err := unescape(parse.Dict.Delimiter) + if err != nil { + return err + } + bits := strings.Split(val, sep) + for _, bit := range bits { + pair := strings.SplitN(bit, dlm, 2) + if len(pair) != 2 { + //return fmt.Errorf("Cannot split %s value '%s' on '%s'", fact.Name, pair, parse.Dict.Delimiter) + continue + } + key := strings.TrimSpace(pair[0]) + subcontext := fmt.Sprintf("%s.%s", context, key) + err := engine.SetValueParse(fact, parse.Dict.Parse, subcontext, pair[1]) + if err != nil { + return err + } + } + } else { + return fmt.Errorf("Expecting scalar or list or dict to parse") + } + + return nil +} + +func (engine *Engine) SetValueString(fact *Fact) error { + val, ok := engine.Facts[fact.Value.String.Fact] + if !ok { + return fmt.Errorf("Fact %s not present", fact.Value.String.Fact) + } + + return engine.SetValueParse(fact, fact.Value.String.Parse, fact.Name, string(val)) +} + +func (engine *Engine) SetValueFile(fact *Fact) error { + data, err := ioutil.ReadFile(fact.Value.File.Path) + if err != nil { + if os.IsNotExist(err) && fact.Value.File.IgnoreMissing { + return nil + } + return err + } + + return engine.SetValueParse(fact, fact.Value.File.Parse, fact.Name, string(data)) +} + +func (engine *Engine) SetValueDirEnt(fact *Fact) error { + files, err := ioutil.ReadDir(fact.Value.DirEnt.Path) + if err != nil { + if os.IsNotExist(err) && fact.Value.DirEnt.IgnoreMissing { + return nil + } + return err + } + for _, file := range files { + engine.SetFact(fmt.Sprintf("%s.%s", fact.Name, file.Name()), "true") + } + return nil +} + +func (engine *Engine) SetValueCommand(fact *Fact) error { + var args []string + for _, arg := range fact.Value.Command.Args { + args = append(args, arg) + } + cmd := exec.Command(fact.Value.Command.Name, args...) + out, err := cmd.Output() + if err != nil { + return err + } + + return engine.SetValueParse(fact, fact.Value.Command.Parse, fact.Name, string(out)) +} + +func (engine *Engine) SetValueAccess(fact *Fact) error { + var flags uint32 + if fact.Value.Access.Check == "exists" { + flags = 0 + } else if fact.Value.Access.Check == "readable" { + flags = unix.R_OK + } else if fact.Value.Access.Check == "writable" { + flags = unix.W_OK + } else if fact.Value.Access.Check == "executable" { + flags = unix.X_OK + } else { + return fmt.Errorf("No access check type specified for %s", + fact.Value.Access.Path) + } + err := unix.Access(fact.Value.Access.Path, flags) + if err != nil { + engine.SetFact(fact.Name, "false") + engine.Fail(fact) + } else { + engine.SetFact(fact.Name, "true") + engine.Pass(fact) + } + return nil +} + +func (engine *Engine) ValidateFact(fact *Fact) error { + if fact.Filter != nil { + ok, err := engine.EvalExpression(fact.Filter) + if err != nil { + return err + } + if !ok { + return nil + } + } + if fact.Report != nil && (engine.Output&ENGINE_OUTPUT_REPORTS) != 0 { + fmt.Printf("Checking %s...", fact.Report.Message) + } + + if fact.Value.BuiltIn != nil { + return engine.SetValueBuiltIn(fact) + } else if fact.Value.Bool != nil { + return engine.SetValueBool(fact) + } else if fact.Value.String != nil { + return engine.SetValueString(fact) + } else if fact.Value.File != nil { + return engine.SetValueFile(fact) + } else if fact.Value.DirEnt != nil { + return engine.SetValueDirEnt(fact) + } else if fact.Value.Command != nil { + return engine.SetValueCommand(fact) + } else if fact.Value.Access != nil { + return engine.SetValueAccess(fact) + } else { + return fmt.Errorf("No information provided for value in fact %s", fact.Name) + } +} + +// Validate all facts in the list, returning a count of +// any non-fatal errors encountered. +func (engine *Engine) Validate(facts FactList) (uint, error) { + err := facts.Sort() + if err != nil { + return 0, err + } + for _, fact := range facts.Facts { + err = engine.ValidateFact(fact) + if err != nil { + return 0, err + } + } + return engine.Errors, nil +} diff --git a/tools/host-validate/pkg/facts.go b/tools/host-validate/pkg/facts.go new file mode 100644 index 0000000000..4bb935cc50 --- /dev/null +++ b/tools/host-validate/pkg/facts.go @@ -0,0 +1,585 @@ +/* + * This file is part of the libvirt project + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Copyright (C) 2019 Red Hat, Inc. + * + */ + +package pkg + +// This file defines the data structures that are being +// parsed from the YAML files. Note that the YAML parser +// internally uses the Golang JSON parser, hence the 'json:' +// comments against struct fields. + +import ( + "fmt" + "github.com/ghodss/yaml" + "io/ioutil" + "strings" +) + +// A list of all the facts we are going to validate +type FactList struct { + Facts []*Fact `json:"facts"` +} + +// A fact is a description of a single piece of information +// we wish to check. Conceptually a fact is simply a plain +// key, value pair where both parts are strings. +// +// Every fact has a name which is a dot separated list of +// strings, eg 'cpu.family.arm'. By convention the dots +// are forming an explicit hierarchy, so a common prefix +// on names is used to group related facts. +// +// Optionally a report can be associated with a fact +// This is a freeform string intended to be read by +// humans, eg 'hardware virt possible' +// +// If a report is given, there can also be an optional +// hint given, which is used when a fact fails to match +// some desired condition. This is another freeform string +// intended to be read by humans, eg +// 'enable cpuset cgroup controller in Kconfig' +// +// The optional filter is an expression that can be used +// to skip the processing of this fact when certain +// conditions are not met. eg, a filter might skip +// the checking of cgroups when the os kernel is not "linux" +// +// Finally there is a mandatory value. This defines how +// to extract the value for setting the fact. +// +// +// +type Fact struct { + Name string `json:"name"` + Report *Report `json:"report,omitempty"` + Hint *Report `json:"hint,omitempty"` + Filter *Expression `json:"filter,omitempty"` + Value Value `json:"value"` +} + +// A report is a message intended to be targetted at humans +// +// The message can be an arbitrary string, informing them +// of some relevant information +// +// The level is one of 'warn' or 'note' or 'error', with +// 'error' being assumed if no value is given +type Report struct { + Message string `json:"message"` + Level string `json:"level,omitempty"` + Pass string `json:"pass,omitempty"` +} + +// An expression is used to evaluate some complex conditions +// +// Expressions can be simple, comparing a single fact to +// some desired match. +// +// Expressions can be compound, requiring any or all of a +// list of sub-expressions to evaluate to true. +type Expression struct { + Any *ExpressionCompound `json:"any,omitempty"` + All *ExpressionCompound `json:"all,omitempty"` + Fact *ExpressionFact `json:"fact,omitempty"` +} + +// A compound expression is simply a list of expressions +// to be evaluated +type ExpressionCompound struct { + Expressions []Expression `json:"expressions,omitempty"` +} + +// A fact expression defines a rule that compares the +// value associated with the fact, to some desired +// match. +// +// The name gives the name of the fact to check +// +// The semantics of value vary according to the match +// type +// +// If the match type is 'regex', then the value must +// match against the declared regular expression. +// +// If the match type is 'exists', the value is ignored +// and the fact must simply exist. +// +// If the match type is not set, then a plain string +// equality comparison is done +type ExpressionFact struct { + Name string `json:"name"` + Value string `json:"value,omitempty"` + Match string `json:"match,omitempty"` +} + +// A value defines the various data sources for +// setting a fact's value. Only one of the data +// sources is permitted to be non-nil for each +// fact +// +// A builtin value is one of the standard facts +// defined in code. +// +// A bool value is one set to 'true' or 'false' +// depending on the results of evaluating an +// expression. It is user error to construct +// an expression which is self-referential +// +// A string value is one set by parsing the +// the value of another fact. A typical use +// case would be to split a string based on +// a whitespace separator +// +// A file value is one set by parsing the +// contents of a file on disk +// +// A dirent value results in creation of +// many facts, one for each directory entry +// seen +// +// An access value is one that sets a value +// to 'true' or 'false' depending on the +// permissions of a file +// +// A command value is one set by parsing +// the output of a command's stdout +type Value struct { + BuiltIn *ValueBuiltIn `json:"builtin,omitempty"` + Bool *Expression `json:"bool,omitempty"` + String *ValueString `json:"string,omitempty"` + File *ValueFile `json:"file,omitempty"` + DirEnt *ValueDirEnt `json:"dirent,omitempty"` + Access *ValueAccess `json:"access,omitempty"` + Command *ValueCommand `json:"command,omitempty"` +} + +// Valid built-in fact names are +// - os.{kernel,release,version} and cpu.arch, +// set from uname() syscall results +// - libvirt.driver set from a command line arg +type ValueBuiltIn struct { +} + +// Sets a value from a command. +// +// The name is the binary command name, either +// unqualified and resolved against $PATH, or +// or fully qualified +// +// A command can be given an arbitray set of +// arguments when run +// +// By default the entire contents of stdout +// will be set as the fact value. +// +// It is possible to instead parse the stdout +// data to extract interesting pieces of information +// from it +type ValueCommand struct { + Name string `json:"name"` + Args []string `json:"args,omitempty"` + Parse *Parse `json:"parse,omitempty"` +} + +// Sets a value from a file contents +// +// The path is the fully qualified filename path +// +// By default the entire contents of the file +// will be set as the fact value. +// +// It is possible to instead parse the file +// data to extract interesting pieces of information +// from it +type ValueFile struct { + Path string `json:"path"` + Parse *Parse `json:"parse,omitempty"` + IgnoreMissing bool `json:"ignoreMissing,omitempty"` +} + +// Sets a value from another fact +// +// The fact is the name of the other fact +// +// By default the entire contents of the other fact +// will be set as the fact value. +// +// More usually though the other fact value will be +// parsed to extract interesting pieces of information +// from it +type ValueString struct { + Fact string `json:"fact"` + Parse *Parse `json:"parse,omitempty"` +} + +// Sets a value from a list of directory entries +// +// The path is the fully qualified path of the directory +// +// By default an error will be raised if the directory +// does not exist. Typically a filter rule would be +// desired to skip processing of the fact in cases +// where it is known the directory may not exist. +// +// If filters are not practical though, missing directory +// can be made non-fatal +type ValueDirEnt struct { + Path string `json:"path"` + IgnoreMissing bool `json:"ignoreMissing,omitempty"` +} + +// Sets a value from the access permissions of a file +// +// The path is the fully qualified path of the file +// +// The check can be one of the strings 'readable', +// 'writable' or 'executable'. +type ValueAccess struct { + Path string `json:"path"` + Check string `json:"check"` +} + +// The parse object defines a set of rules for +// parsing strings to extract interesting +// pieces of data +// +// The optional whitespace attribute can be set to +// 'trim' to cause leading & trailing whitespace to +// be removed before further processing +// +// To extract a single data item from the string, +// the scalar parsing rule can be used +// +// To extract an ordered list of data items from +// the string, the list parsing rule can be used +// +// To extract an unordered list of data items, +// with duplicates excluded, the set parsing rule +// can be used +// +// To extract a list of key, value pairs from the +// string, the dict parsing rule can be used +type Parse struct { + Whitespace string `json:"whitespace,omitempty"` + Scalar *ParseScalar `json:"scalar,omitempty"` + List *ParseList `json:"list,omitempty"` + Set *ParseSet `json:"set,omitempty"` + Dict *ParseDict `json:"dict,omitempty"` +} + +// Parsing a string to extract a single data item +// using a regular expression. +// +// The regular expression should contain at least +// one capturing group. The match attribute indicates +// which capturing group will be used to set the +// fact value. +type ParseScalar struct { + Regex string `json:"regex,omitempty"` + Match uint `json:"match,omitempty"` +} + +// Parsing a string to extract an ordered list of +// data items +// +// The separator declares the boundary on which +// the string will be split +// +// The skip head attribute should be non-zero if +// any leading elements in the list are to be +// discarded. This is typically useful if the +// list contains a header/label as the first +// entry +// +// The skip tail attribute should be non-zero if +// any trailing elements in the list are to be +// discarded +// +// The limit attribute sets an upper bound on +// the number of elements that will be kept in +// the list. It is applied after discarding any +// leading or trailing elements. +// +// Each element in the list is then itself parsed +type ParseList struct { + Separator string `json:"separator,omitempty"` + SkipHead uint `json:"skiphead,omitempty"` + SkipTail uint `json:"skiptail,omitempty"` + Limit uint `json:"limit,omitempty"` + Parse *Parse `json:"parse,omitempty"` +} + +// Parsing a string to extract an unordered list of +// data items with duplicates removed +// +// The separator declares the boundary on which +// the string will be split +// +// The skip head attribute should be non-zero if +// any leading elements in the list are to be +// discarded. This is typically useful if the +// list contains a header/label as the first +// entry +// +// The skip tail attribute should be non-zero if +// any trailing elements in the list are to be +// discarded +// +// Each element is then parsed using a regular +// expression +// +// The regular expression should contain at least +// one capturing group. The match attribute indicates +// which capturing group will be used to set the +// fact value. +type ParseSet struct { + Separator string `json:"separator,omitempty"` + SkipHead uint `json:"skiphead,omitempty"` + SkipTail uint `json:"skiptail,omitempty"` + Regex string `json:"regex,omitempty"` + Match uint `json:"match,omitempty"` +} + +// Parsing a string to extract an unordered list of +// data items with duplicates removed +// +// The separator declares the boundary on which +// the string will be split to acquire the list +// of pairs +// +// The delimiter declares the boundary to separate +// the key from the value +// +// The value is then further parsed with the declared +// rules +type ParseDict struct { + Separator string `json:"separator,omitempty"` + Delimiter string `json:"delimiter,omitempty"` + Parse *Parse `json:"parse,omitempty"` +} + +// Helper for parsing a string containing an YAML +// doc defining a list of facts +func (f *FactList) Unmarshal(doc string) error { + return yaml.Unmarshal([]byte(doc), f) +} + +// Create a new fact list, loading from the +// specified plain file +func NewFactList(filename string) (*FactList, error) { + yamlstr, err := ioutil.ReadFile(filename) + if err != nil { + return nil, err + } + + facts := &FactList{} + err = facts.Unmarshal(string(yamlstr)) + if err != nil { + return nil, err + } + + return facts, nil +} + +// Used to ensure that no fact has a name which is a sub-string of +// another fact. +func validateNames(names map[string]*Fact) error { + for name, _ := range names { + bits := strings.Split(name, ".") + subname := "" + for _, bit := range bits[0 : len(bits)-1] { + if subname == "" { + subname = bit + } else { + subname = subname + "." + bit + } + _, ok := names[subname] + + if ok { + return fmt.Errorf("Fact name '%s' has fact '%s' as a substring", + name, subname) + } + } + } + + return nil +} + +// Identify the name of the corresponding fact that +// is referenced, by chopping off suffixes until a +// match is found +func findFactReference(names map[string]*Fact, name string) (string, error) { + bits := strings.Split(name, ".") + subname := "" + for _, bit := range bits { + if subname == "" { + subname = bit + } else { + subname = subname + "." + bit + } + _, ok := names[subname] + if ok { + return subname, nil + } + } + + return "", fmt.Errorf("Cannot find fact providing %s", name) +} + +// Build up a list of dependant facts referenced by an expression +func addDepsExpr(deps *map[string][]string, names map[string]*Fact, fact *Fact, expr *Expression) error { + if expr.Any != nil { + for _, sub := range expr.Any.Expressions { + err := addDepsExpr(deps, names, fact, &sub) + if err != nil { + return err + } + } + } else if expr.All != nil { + for _, sub := range expr.All.Expressions { + err := addDepsExpr(deps, names, fact, &sub) + if err != nil { + return err + } + } + } else if expr.Fact != nil { + ref, err := findFactReference(names, expr.Fact.Name) + if err != nil { + return err + } + entries, _ := (*deps)[fact.Name] + entries = append(entries, ref) + (*deps)[fact.Name] = entries + } + return nil +} + +// Build up a list of dependancies between facts +func addDeps(deps *map[string][]string, names map[string]*Fact, fact *Fact) error { + if fact.Filter != nil { + err := addDepsExpr(deps, names, fact, fact.Filter) + if err != nil { + return err + } + } + if fact.Value.Bool != nil { + err := addDepsExpr(deps, names, fact, fact.Value.Bool) + if err != nil { + return err + } + } + if fact.Value.String != nil { + ref, err := findFactReference(names, fact.Value.String.Fact) + if err != nil { + return err + } + entries, _ := (*deps)[fact.Name] + entries = append(entries, ref) + (*deps)[fact.Name] = entries + } + return nil +} + +// Perform a topological sort on facts so that they can be +// processed in the order required to satisfy dependancies +// between facts +func (facts *FactList) Sort() error { + deps := make(map[string][]string) + names := make(map[string]*Fact) + + var remaining []string + for _, fact := range facts.Facts { + deps[fact.Name] = []string{} + names[fact.Name] = fact + remaining = append(remaining, fact.Name) + } + + err := validateNames(names) + if err != nil { + return err + } + + for _, fact := range facts.Facts { + err = addDeps(&deps, names, fact) + if err != nil { + return err + } + } + + var sorted []string + done := make(map[string]bool) + for len(remaining) > 0 { + prev_done := len(done) + var skipped []string + for _, fact := range remaining { + using, ok := deps[fact] + if !ok { + done[fact] = true + sorted = append(sorted, fact) + } else { + unsolved := false + for _, entry := range using { + _, ok := done[entry] + if !ok { + unsolved = true + break + } + } + if !unsolved { + sorted = append(sorted, fact) + done[fact] = true + } else { + skipped = append(skipped, fact) + } + } + } + + if len(done) == prev_done { + return fmt.Errorf("Cycle detected in facts") + } + + remaining = skipped + } + + var newfacts []*Fact + for _, name := range sorted { + newfacts = append(newfacts, names[name]) + } + + facts.Facts = newfacts + + return nil +} + +// Create a new fact list that contains all the facts +// from the passed in list of fact lists +func MergeFactLists(lists []FactList) FactList { + var allfacts []*Fact + for _, list := range lists { + for _, fact := range list.Facts { + allfacts = append(allfacts, fact) + } + } + + facts := FactList{} + facts.Facts = allfacts + return facts +} diff --git a/tools/host-validate/pkg/facts_test.go b/tools/host-validate/pkg/facts_test.go new file mode 100644 index 0000000000..850ba50bfb --- /dev/null +++ b/tools/host-validate/pkg/facts_test.go @@ -0,0 +1,50 @@ +/* + * This file is part of the libvirt project + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Copyright (C) 2019 Red Hat, Inc. + * + */ + +package pkg + +import ( + "fmt" + "io/ioutil" + "path" + "strings" + "testing" +) + +func testYAMLFile(t *testing.T, filename string) { + _, err := NewFactList(filename) + if err != nil { + t.Fatal(fmt.Errorf("Cannot parse %s: %s", filename, err)) + } +} + +func TestParse(t *testing.T) { + dir := path.Join("..", "rules") + files, err := ioutil.ReadDir(dir) + if err != nil { + t.Fatal(fmt.Errorf("Cannot read %s: %s", dir, err)) + } + for _, file := range files { + if strings.HasSuffix(file.Name(), ".yaml") { + testYAMLFile(t, path.Join(dir, file.Name())) + } + } +} -- 2.21.0

Daniel P. Berrangé <berrange@redhat.com> [2019-09-27, 01:52PM +0100]:
+package main + +import ( + "flag" + "fmt" + "github.com/spf13/pflag"
I don't like external dependencies like this at all. Not from a administrative POV (there are likely a number of build environments, including ours, that want to build libvirt offline) and not from a security POV (this reminds me of the npm-debacles we see every now and then).
+ "io/ioutil" + vl "libvirt.org/host-validate/pkg" + "os" + "path/filepath" + "strings" +)

On Tue, Oct 01, 2019 at 01:33:34PM +0200, Bjoern Walk wrote:
Daniel P. Berrangé <berrange@redhat.com> [2019-09-27, 01:52PM +0100]:
+package main + +import ( + "flag" + "fmt" + "github.com/spf13/pflag"
I don't like external dependencies like this at all. Not from a administrative POV (there are likely a number of build environments, including ours, that want to build libvirt offline) and not from a security POV (this reminds me of the npm-debacles we see every now and then).
This is an inaccurate view of Go build/dependancies. Nothing prevents you building Go code offline. This is exactly what Fedora does for all Go code. By default the Go toolchain will pull from the upstream directly, but you can tell it to use locally packaged source, or can pre-populate a cache of downloaded content. As for external dependencies in general, libvirt.so already links to 60+ libraries right now. Using external dependancies is a good thing because you are not re-inventing the wheel constantly, instead picking a solution that is already robust & tested. 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 :|

This commit defines a set of YAML rules that result in the same set of logical checks as the existing hardcoded virt-host-validate implementation does. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/host-validate/rules/builtin.yaml | 20 + tools/host-validate/rules/cpu.yaml | 50 ++ tools/host-validate/rules/freebsd-kernel.yaml | 77 +++ tools/host-validate/rules/linux-acpi.yaml | 39 ++ tools/host-validate/rules/linux-cgroups.yaml | 470 ++++++++++++++++++ tools/host-validate/rules/linux-cpu.yaml | 134 +++++ tools/host-validate/rules/linux-devices.yaml | 71 +++ tools/host-validate/rules/linux-iommu.yaml | 113 +++++ .../host-validate/rules/linux-namespaces.yaml | 119 +++++ tools/host-validate/rules/linux-pci.yaml | 10 + 10 files changed, 1103 insertions(+) create mode 100644 tools/host-validate/rules/builtin.yaml create mode 100644 tools/host-validate/rules/cpu.yaml create mode 100644 tools/host-validate/rules/freebsd-kernel.yaml create mode 100644 tools/host-validate/rules/linux-acpi.yaml create mode 100644 tools/host-validate/rules/linux-cgroups.yaml create mode 100644 tools/host-validate/rules/linux-cpu.yaml create mode 100644 tools/host-validate/rules/linux-devices.yaml create mode 100644 tools/host-validate/rules/linux-iommu.yaml create mode 100644 tools/host-validate/rules/linux-namespaces.yaml create mode 100644 tools/host-validate/rules/linux-pci.yaml diff --git a/tools/host-validate/rules/builtin.yaml b/tools/host-validate/rules/builtin.yaml new file mode 100644 index 0000000000..f69b069e09 --- /dev/null +++ b/tools/host-validate/rules/builtin.yaml @@ -0,0 +1,20 @@ +# +# Define boilerplate to activate various built-in facts +# + +facts: +- name: libvirt.driver + value: + builtin: {} +- name: cpu.arch + value: + builtin: {} +- name: os.kernel + value: + builtin: {} +- name: os.release + value: + builtin: {} +- name: os.version + value: + builtin: {} diff --git a/tools/host-validate/rules/cpu.yaml b/tools/host-validate/rules/cpu.yaml new file mode 100644 index 0000000000..5af61b1ea1 --- /dev/null +++ b/tools/host-validate/rules/cpu.yaml @@ -0,0 +1,50 @@ +# +# Define facts related to host CPU properties +# + +facts: +- name: cpu.family.x86 + value: + bool: + any: + expressions: + - fact: + name: cpu.arch + value: x86_64 + - fact: + name: cpu.arch + value: i386 + - fact: + name: cpu.arch + value: i486 + - fact: + name: cpu.arch + value: i586 + - fact: + name: cpu.arch + value: i686 +- name: cpu.family.arm + value: + bool: + any: + expressions: + - fact: + name: cpu.arch + value: aarch64 + - fact: + name: cpu.arch + value: armv6 + - fact: + name: cpu.arch + value: armv7 +- name: cpu.family.s390 + value: + bool: + any: + expressions: + - fact: + name: cpu.arch + value: s390 + - fact: + name: cpu.arch + value: s390x diff --git a/tools/host-validate/rules/freebsd-kernel.yaml b/tools/host-validate/rules/freebsd-kernel.yaml new file mode 100644 index 0000000000..345e78dbc2 --- /dev/null +++ b/tools/host-validate/rules/freebsd-kernel.yaml @@ -0,0 +1,77 @@ +# +# Define facts related to BHyve on FreeBSD +# + +facts: +- name: os.kmod + filter: + fact: + name: os.kernel + value: FreeBSD + value: + command: + name: kldstat + parse: + set: + separator: \n + skiphead: 1 + skiptail: 1 + regex: \s+\d+\s+\d+\s+0x[0-9a-f]+\s+[0-9a-f]+\s+(\w+) + match: 1 +- name: kmod.vmm + filter: + fact: + name: libvirt.driver.bhyve + value: "true" + report: + message: BHyve VMs can be run + hint: + message: load the 'vmm' kernel module + value: + bool: + fact: + name: os.kmod.vmm + value: "true" +- name: kmod.if_tap + filter: + fact: + name: libvirt.driver.bhyve + value: "true" + report: + message: BHyve VMs can use networking + hint: + message: load the 'if_tap' kernel module + value: + bool: + fact: + name: os.kmod.if_tap + value: "true" +- name: kmod.if_bridge + filter: + fact: + name: libvirt.driver.bhyve + value: "true" + report: + message: BHyve VMs can use bridged network + hint: + message: load the 'if_bridge' kernel module + value: + bool: + fact: + name: os.kmod.if_bridge + value: "true" +- name: kmod.nmdm + filter: + fact: + name: libvirt.driver.bhyve + value: "true" + report: + message: BHyve VMs can use nmdm console + level: warn + hint: + message: load the 'nmdm' kernel module + value: + bool: + fact: + name: os.kmod.nmdm + value: "true" diff --git a/tools/host-validate/rules/linux-acpi.yaml b/tools/host-validate/rules/linux-acpi.yaml new file mode 100644 index 0000000000..cad324dd96 --- /dev/null +++ b/tools/host-validate/rules/linux-acpi.yaml @@ -0,0 +1,39 @@ +# +# Define facts for interesting ACPI tables on the host +# + +facts: +- name: cpu.acpi.dmar + filter: + all: + expressions: + - fact: + name: cpu.family.x86 + value: "true" + - fact: + name: cpu.vendor.intel + value: "true" + - fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/firmware/acpi/tables/DMAR +- name: cpu.acpi.ivrs + filter: + all: + expressions: + - fact: + name: cpu.family.x86 + value: "true" + - fact: + name: cpu.vendor.amd + value: "true" + - fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/firmware/acpi/tables/IVRS diff --git a/tools/host-validate/rules/linux-cgroups.yaml b/tools/host-validate/rules/linux-cgroups.yaml new file mode 100644 index 0000000000..e886bccd17 --- /dev/null +++ b/tools/host-validate/rules/linux-cgroups.yaml @@ -0,0 +1,470 @@ +# +# Define facts for Linux control cgroups v1/v2 +# + +facts: +- name: os.cgroup.controller + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /proc/cgroups + parse: + set: + separator: \n + skiphead: 1 + skiptail: 1 + regex: ^(\w+) + match: 1 +- name: os.cgroup.v2only + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/cgroup.subtree_control +- name: os.cgroup.v2hybrid + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/fs/cgroup/unified/cgroup.controllers + ignoreMissing: true + parse: + whitespace: trim + set: + skiphead: 0 + skiptail: 0 + separator: ' ' +- name: os.cgroup.mount.v2 + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/fs/cgroup/cgroup.controllers + ignoreMissing: true + parse: + whitespace: trim + set: + skiphead: 0 + skiptail: 0 + separator: ' ' +- name: os.cgroup.mount.v1.blkio + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/blkio/cgroup.procs +- name: os.cgroup.mount.v1.cpu + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/cpu/cgroup.procs +- name: os.cgroup.mount.v1.cpuacct + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/cpuacct/cgroup.procs +- name: os.cgroup.mount.v1.cpuset + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/cpuset/cgroup.procs +- name: os.cgroup.mount.v1.devices + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/devices/cgroup.procs +- name: os.cgroup.mount.v1.freezer + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/freezer/cgroup.procs +- name: os.cgroup.mount.v1.hugetlb + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/hugetlb/cgroup.procs +- name: os.cgroup.mount.v1.memory + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/memory/cgroup.procs +- name: os.cgroup.mount.v1.net_cls + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/net_cls/cgroup.procs +- name: os.cgroup.mount.v1.net_prio + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/net_prio/cgroup.procs +- name: os.cgroup.mount.v1.perf_event + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/perf_event/cgroup.procs +- name: os.cgroup.mount.v1.pids + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/pids/cgroup.procs +- name: os.cgroup.mount.unified + filter: + fact: + name: os.kernel + value: Linux + value: + access: + check: exists + path: /sys/fs/cgroup/unified/cgroup.procs +- name: os.cgroup.memory.present + filter: + all: + expressions: + - any: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: cgroup memory controller present + hint: + message: enable memory cgroup controller in Kconfig + value: + bool: + fact: + name: os.cgroup.controller.memory + value: "true" +- name: os.cgroup.memory.mounted + filter: + fact: + name: os.cgroup.memory.present + value: "true" + report: + message: cgroup memory controller mounted + hint: + message: mount the memory cgroup controller under /sys/fs/cgroup + value: + bool: + any: + expressions: + - fact: + name: os.cgroup.mount.v1.memory + value: "true" + - fact: + name: os.cgroup.mount.v2.memory + value: "true" +- name: os.cgroup.cpu.present + filter: + all: + expressions: + - any: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: cgroup cpu controller present + hint: + message: enable cpu cgroup controller in Kconfig + value: + bool: + fact: + name: os.cgroup.controller.cpu + value: "true" +- name: os.cgroup.cpu.mounted + filter: + fact: + name: os.cgroup.cpu.present + value: "true" + report: + message: cgroup cpu controller mounted + hint: + message: mount the cpu cgroup controller under /sys/fs/cgroup + value: + bool: + any: + expressions: + - fact: + name: os.cgroup.mount.v1.cpu + value: "true" + - fact: + name: os.cgroup.mount.v2.cpu + value: "true" +- name: os.cgroup.cpuacct.present + filter: + all: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: cgroup cpuacct controller present + hint: + message: enable cpuacct cgroup controller in Kconfig + value: + bool: + fact: + name: os.cgroup.controller.cpuacct + value: "true" +- name: os.cgroup.cpuacct.mounted + filter: + fact: + name: os.cgroup.cpuacct.present + value: "true" + report: + message: cgroup cpuacct controller mounted + hint: + message: mount the cpuacct cgroup controller under /sys/fs/cgroup + value: + bool: + any: + expressions: + - fact: + name: os.cgroup.mount.v1.cpuacct + value: "true" + - fact: + name: os.cgroup.mount.v2.cpuacct + value: "true" +- name: os.cgroup.cpuset.present + filter: + all: + expressions: + - any: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: cgroup cpuset controller present + hint: + message: enable cpuset cgroup controller in Kconfig + value: + bool: + fact: + name: os.cgroup.controller.cpuset + value: "true" +- name: os.cgroup.cpuset.mounted + filter: + fact: + name: os.cgroup.cpuset.present + value: "true" + report: + message: cgroup cpuset controller mounted + hint: + message: mount the cpuset cgroup controller under /sys/fs/cgroup + value: + bool: + any: + expressions: + - fact: + name: os.cgroup.mount.v1.cpuset + value: "true" + - fact: + name: os.cgroup.mount.v2.cpuset + value: "true" +- name: os.cgroup.devices.present + filter: + all: + expressions: + - any: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: cgroup devices controller present + hint: + message: enable devices cgroup controller in Kconfig + value: + bool: + fact: + name: os.cgroup.controller.devices + value: "true" +- name: os.cgroup.devices.mounted + filter: + fact: + name: os.cgroup.devices.present + value: "true" + report: + message: cgroup devices controller mounted + hint: + message: mount the devices cgroup controller under /sys/fs/cgroup + value: + bool: + any: + expressions: + - fact: + name: os.cgroup.mount.v1.devices + value: "true" + - fact: + name: os.cgroup.v2hybrid + value: "true" + - fact: + name: os.cgroup.v2only + value: "true" +- name: os.cgroup.blkio.present + filter: + all: + expressions: + - any: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: cgroup blkio controller present + hint: + message: enable blkio cgroup controller in Kconfig + value: + bool: + fact: + name: os.cgroup.controller.blkio + value: "true" +- name: os.cgroup.blkio.mounted + filter: + fact: + name: os.cgroup.blkio.present + value: "true" + report: + message: cgroup blkio controller mounted + hint: + message: mount the blkio cgroup controller under /sys/fs/cgroup + value: + bool: + any: + expressions: + - fact: + name: os.cgroup.mount.v1.blkio + value: "true" + - fact: + name: os.cgroup.mount.v2.io + value: "true" +- name: os.cgroup.freezer.present + filter: + all: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: cgroup freezer controller present + hint: + message: enable freezer cgroup controller in Kconfig + value: + bool: + fact: + name: os.cgroup.controller.freezer + value: "true" +- name: os.cgroup.freezer.mounted + filter: + fact: + name: os.cgroup.freezer.present + value: "true" + report: + message: cgroup freezer controller mounted + hint: + message: mount the freezer cgroup controller under /sys/fs/cgroup + value: + bool: + any: + expressions: + - fact: + name: os.cgroup.mount.v1.freezer + value: "true" + - fact: + name: os.cgroup.mount.v2.freezer + value: "true" diff --git a/tools/host-validate/rules/linux-cpu.yaml b/tools/host-validate/rules/linux-cpu.yaml new file mode 100644 index 0000000000..81aee516bf --- /dev/null +++ b/tools/host-validate/rules/linux-cpu.yaml @@ -0,0 +1,134 @@ +# +# Define facts related to host CPU properties +# + +facts: +- name: cpu.info + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /proc/cpuinfo + parse: + list: + limit: 1 + separator: \n\n + parse: + whitespace: trim + dict: + separator: \n + delimiter: ':' + parse: + whitespace: trim + scalar: {} +- name: cpu.vendor.intel + filter: + fact: + name: cpu.family.x86 + value: "true" + value: + bool: + fact: + name: cpu.info.0.vendor_id + value: GenuineIntel +- name: cpu.vendor.amd + filter: + fact: + name: cpu.family.x86 + value: "true" + value: + bool: + fact: + name: cpu.info.0.vendor_id + value: AuthenticAMD +- name: cpu.features.x86 + filter: + fact: + name: cpu.family.x86 + value: "true" + value: + string: + fact: cpu.info.0.flags + parse: + whitespace: trim + set: + skiphead: 0 + skiptail: 0 + separator: ' ' +- name: cpu.features.arm + filter: + fact: + name: cpu.family.arm + value: "true" + value: + string: + fact: cpu.info.0.Features + parse: + whitespace: trim + set: + skiphead: 0 + skiptail: 0 + separator: ' ' +- name: cpu.features.s390 + filter: + fact: + name: cpu.family.s390 + value: "true" + value: + string: + fact: cpu.info.0.features + parse: + whitespace: trim + set: + skiphead: 0 + skiptail: 0 + separator: ' ' +- name: cpu.virt.possible + filter: + fact: + name: libvirt.driver.qemu + value: "true" + report: + message: hardware virt possible + value: + bool: + any: + expressions: + - fact: + name: cpu.family.x86 + value: "true" +- name: cpu.virt.present + filter: + fact: + name: cpu.virt.possible + value: "true" + report: + message: hardware virt present + level: warn + hint: + message: only emulated CPUs are available, performance will be significantly limited + value: + bool: + any: + expressions: + - all: + expressions: + - fact: + name: cpu.vendor.amd + value: "true" + - fact: + name: cpu.features.x86.svm + value: "true" + - all: + expressions: + - fact: + name: cpu.vendor.intel + value: "true" + - fact: + name: cpu.features.x86.vmx + value: "true" + - fact: + name: cpu.features.s390.sie + value: "true" diff --git a/tools/host-validate/rules/linux-devices.yaml b/tools/host-validate/rules/linux-devices.yaml new file mode 100644 index 0000000000..791f35a0b6 --- /dev/null +++ b/tools/host-validate/rules/linux-devices.yaml @@ -0,0 +1,71 @@ +# +# Define facts related to device nodes on the host +# + +facts: +- name: os.kvm.loaded + filter: + all: + expressions: + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: cpu.virt.present + value: "true" + report: + message: /dev/kvm loaded + value: + access: + check: exists + path: /dev/kvm +- name: os.kvm.accessible + filter: + fact: + name: os.kvm.loaded + value: "true" + report: + message: /dev/kvm accessible + hint: + message: Check /dev/kvm is world writable or you are in a group that is allowed + to access it + value: + access: + check: writable + path: /dev/kvm +- name: os.vhostnet.present + filter: + all: + expressions: + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: os.kvm.loaded + value: "true" + report: + message: /dev/vhost-net present + hint: + message: Load the 'vhost_net' module to improve performance of virtio networking + value: + access: + check: exists + path: /dev/vhost-net +- name: os.tun.present + filter: + all: + expressions: + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: os.kvm.loaded + value: "true" + report: + message: /dev/net/tun present + hint: + message: Load the 'tun' module to enable networking for QEMU guests + value: + access: + check: exists + path: /dev/net/tun diff --git a/tools/host-validate/rules/linux-iommu.yaml b/tools/host-validate/rules/linux-iommu.yaml new file mode 100644 index 0000000000..4f056e92ba --- /dev/null +++ b/tools/host-validate/rules/linux-iommu.yaml @@ -0,0 +1,113 @@ +# +# Define facts related to IOMMU availability +# + +facts: +- name: cpu.iommu.x86.intel.present + filter: + all: + expressions: + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: cpu.family.x86 + value: "true" + - fact: + name: cpu.vendor.intel + value: "true" + report: + message: Intel device assignment IOMMU present + level: note + hint: + message: IOMMU either disabled in BIOS or not supported by this hardware + value: + bool: + fact: + name: cpu.acpi.dmar + value: "true" +- name: cpu.iommu.x86.amd.present + filter: + all: + expressions: + - fact: + name: cpu.family.x86 + value: "true" + - fact: + name: cpu.vendor.amd + value: "true" + report: + message: AMD device assignment IOMMU present + level: note + hint: + message: IOMMU either disabled in BIOS or not supported by this hardware + value: + bool: + fact: + name: cpu.acpi.ivrs + value: "true" +- name: cpu.iommu.s390x.present + filter: + fact: + name: cpu.family.s390 + value: "true" + report: + message: s390x device assignment IOMMU present + level: note + hint: + message: IOMMU either disabled in BIOS or not supported by this hardware + value: + bool: + fact: + match: exists + name: os.pci.devices.0 +- name: os.iommu.groups + value: + dirent: + ignoreMissing: true + path: /sys/kernel/iommu_groups +- name: os.iommu.x86.intel.enabled + filter: + fact: + name: cpu.iommu.x86.intel.present + value: "true" + report: + message: Intel device assignment IOMMU enabled + level: warn + hint: + message: IOMMU disabled by the kernel. Pass 'intel_iommu=on' on the kernel command line + value: + bool: + fact: + name: os.iommu.groups.0 + match: exists +- name: os.iommu.x86.amd.enabled + filter: + fact: + name: cpu.iommu.x86.amd.present + value: "true" + report: + message: AMD device assignment IOMMU enabled + level: warn + hint: + message: IOMMU disabled by the kernel. Pass 'iommu=pt iommu=1' on the kernel command line + value: + bool: + fact: + name: os.iommu.groups.0 + match: exists +- name: os.iommu.s390x.enabled + filter: + fact: + name: cpu.iommu.s390x.present + value: "true" + report: + message: s390x device assignment IOMMU enabled + level: warn + hint: + message: IOMMU disabled by the kernel + value: + bool: + fact: + name: os.iommu.groups.0 + match: exists diff --git a/tools/host-validate/rules/linux-namespaces.yaml b/tools/host-validate/rules/linux-namespaces.yaml new file mode 100644 index 0000000000..239d0c58e4 --- /dev/null +++ b/tools/host-validate/rules/linux-namespaces.yaml @@ -0,0 +1,119 @@ +# +# Define facts related to Linux kernel namespaces +# + +facts: +- name: os.namespace.ipc + filter: + all: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: ipc process namespace + hint: + message: Enable ipc namespace in Kconfig + value: + access: + path: /proc/self/ns/ipc + check: exists +- name: os.namespace.mnt + filter: + all: + expressions: + - any: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: libvirt.driver.qemu + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: mnt process namespace + hint: + message: Enable mnt namespace in Kconfig + value: + access: + path: /proc/self/ns/mnt + check: exists +- name: os.namespace.pid + filter: + all: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: pid process namespace + hint: + message: Enable pid namespace in Kconfig + value: + access: + path: /proc/self/ns/pid + check: exists +- name: os.namespace.uts + filter: + all: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: uts process namespace + hint: + message: Enable uts namespace in Kconfig + value: + access: + path: /proc/self/ns/uts + check: exists +- name: os.namespace.net + filter: + all: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: os.kernel + value: Linux + report: + message: net process namespace + hint: + message: Enable net namespace in Kconfig + value: + access: + path: /proc/self/ns/net + check: exists +- name: os.namespace.user + filter: + all: + expressions: + - fact: + name: libvirt.driver.lxc + value: "true" + - fact: + name: os.kernel + value: Linux + report: + level: warn + message: user process namespace + hint: + message: Enable user namespace in Kconfig + value: + access: + path: /proc/self/ns/user + check: exists diff --git a/tools/host-validate/rules/linux-pci.yaml b/tools/host-validate/rules/linux-pci.yaml new file mode 100644 index 0000000000..facb67f4e9 --- /dev/null +++ b/tools/host-validate/rules/linux-pci.yaml @@ -0,0 +1,10 @@ +# +# Define facts related to physical PCI devices on the host system +# + +facts: +- name: os.pci.devices + value: + dirent: + path: /sys/bus/pci/devices + ignoreMissing: true -- 2.21.0

On Fri, Sep 27, 2019 at 13:52:22 +0100, Daniel Berrange wrote:
This commit defines a set of YAML rules that result in the same set of logical checks as the existing hardcoded virt-host-validate implementation does.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/host-validate/rules/builtin.yaml | 20 + tools/host-validate/rules/cpu.yaml | 50 ++ tools/host-validate/rules/freebsd-kernel.yaml | 77 +++ tools/host-validate/rules/linux-acpi.yaml | 39 ++ tools/host-validate/rules/linux-cgroups.yaml | 470 ++++++++++++++++++ tools/host-validate/rules/linux-cpu.yaml | 134 +++++ tools/host-validate/rules/linux-devices.yaml | 71 +++ tools/host-validate/rules/linux-iommu.yaml | 113 +++++ .../host-validate/rules/linux-namespaces.yaml | 119 +++++ tools/host-validate/rules/linux-pci.yaml | 10 + 10 files changed, 1103 insertions(+) create mode 100644 tools/host-validate/rules/builtin.yaml create mode 100644 tools/host-validate/rules/cpu.yaml create mode 100644 tools/host-validate/rules/freebsd-kernel.yaml create mode 100644 tools/host-validate/rules/linux-acpi.yaml create mode 100644 tools/host-validate/rules/linux-cgroups.yaml create mode 100644 tools/host-validate/rules/linux-cpu.yaml create mode 100644 tools/host-validate/rules/linux-devices.yaml create mode 100644 tools/host-validate/rules/linux-iommu.yaml create mode 100644 tools/host-validate/rules/linux-namespaces.yaml create mode 100644 tools/host-validate/rules/linux-pci.yaml
[...]
diff --git a/tools/host-validate/rules/cpu.yaml b/tools/host-validate/rules/cpu.yaml new file mode 100644 index 0000000000..5af61b1ea1 --- /dev/null +++ b/tools/host-validate/rules/cpu.yaml @@ -0,0 +1,50 @@ +# +# Define facts related to host CPU properties +# + +facts: +- name: cpu.family.x86 + value: + bool: + any: + expressions: + - fact: + name: cpu.arch + value: x86_64 + - fact: + name: cpu.arch + value: i386 + - fact: + name: cpu.arch + value: i486 + - fact: + name: cpu.arch + value: i586 + - fact: + name: cpu.arch + value: i686
Honestly I don't think that adding a custom declarative language for this does not make the maintenance any simpler or conforms with our vision of decreasing the maintenance burden by introducing many random languages. Also additionally we'd have the parser engine to maintain. Many of the check would look much simpler when described by actual code. I'm not arguing against the actual list of more granular facts, but the same can be achieved with much more understandable regular code.

This stop building the existing C based virt-host-validate implementation and instead builds the Go implementation. For the RPM spec this is written to rely on pre-packaged RPMs for the 3rd party Go dependencies. These are all already present in supported Fedora releases. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- libvirt.spec.in | 15 ++++++++ m4/virt-host-validate.m4 | 8 ++-- tools/Makefile.am | 75 ++++++++++++++++-------------------- tools/virt-host-validate.pod | 12 ++++-- 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 1c74dbb252..f336296a08 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -402,6 +402,9 @@ BuildRequires: firewalld-filesystem %endif BuildRequires: golang >= 1.11 +BuildRequires: golang-ipath(github.com/spf13/pflag) +BuildRequires: golang-ipath(golang.org/x/sys) +BuildRequires: golang-ipath(github.com/ghodss/yaml) Provides: bundled(gnulib) @@ -1167,6 +1170,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/%{name}.spec) # must make a local Go root with the old style # dir naming scheme/hierarchy mkdir -p gocode/src/libvirt.org +ln -s `pwd`/tools/host-validate `pwd`/gocode/src/libvirt.org/host-validate export GO111MODULE=off export GOPATH=/usr/share/gocode:`pwd`/gocode @@ -1890,6 +1894,17 @@ exit 0 %if %{with_qemu} %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp %endif +%dir %{_datadir}/libvirt/host-validate +%{_datadir}/libvirt/host-validate/builtin.yaml +%{_datadir}/libvirt/host-validate/cpu.yaml +%{_datadir}/libvirt/host-validate/freebsd-kernel.yaml +%{_datadir}/libvirt/host-validate/linux-acpi.yaml +%{_datadir}/libvirt/host-validate/linux-cgroups.yaml +%{_datadir}/libvirt/host-validate/linux-cpu.yaml +%{_datadir}/libvirt/host-validate/linux-devices.yaml +%{_datadir}/libvirt/host-validate/linux-iommu.yaml +%{_datadir}/libvirt/host-validate/linux-namespaces.yaml +%{_datadir}/libvirt/host-validate/linux-pci.yaml %if %{with_bash_completion} %{_datadir}/bash-completion/completions/virsh diff --git a/m4/virt-host-validate.m4 b/m4/virt-host-validate.m4 index e43cec5366..16f2d36acd 100644 --- a/m4/virt-host-validate.m4 +++ b/m4/virt-host-validate.m4 @@ -21,14 +21,14 @@ AC_DEFUN([LIBVIRT_ARG_HOST_VALIDATE], [ AC_DEFUN([LIBVIRT_CHECK_HOST_VALIDATE], [ if test "x$with_host_validate" != "xno"; then - if test "x$with_win" = "xyes"; then + if test "$with_go" = "no"; then if test "x$with_host_validate" = "xyes"; then - AC_MSG_ERROR([virt-host-validate is not supported on Windows]) + AC_MSG_ERROR([Cannot build virt-host-validate without Go toolchain]) else - with_host_validate=no; + with_host_validate=no fi else - with_host_validate=yes; + with_host_validate=yes fi fi diff --git a/tools/Makefile.am b/tools/Makefile.am index 29fdbfe846..728de475a2 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -157,50 +157,41 @@ libvirt_shell_la_SOURCES = \ vsh-table.c vsh-table.h virt_host_validate_SOURCES = \ - virt-host-validate.c \ - virt-host-validate-common.c virt-host-validate-common.h - -VIRT_HOST_VALIDATE_QEMU = \ - virt-host-validate-qemu.c \ - virt-host-validate-qemu.h -VIRT_HOST_VALIDATE_LXC = \ - virt-host-validate-lxc.c \ - virt-host-validate-lxc.h -VIRT_HOST_VALIDATE_BHYVE = \ - virt-host-validate-bhyve.c \ - virt-host-validate-bhyve.h -if WITH_QEMU -virt_host_validate_SOURCES += $(VIRT_HOST_VALIDATE_QEMU) -else ! WITH_QEMU -EXTRA_DIST += $(VIRT_HOST_VALIDATE_QEMU) -endif ! WITH_QEMU - -if WITH_LXC -virt_host_validate_SOURCES += $(VIRT_HOST_VALIDATE_LXC) -else ! WITH_LXC -EXTRA_DIST += $(VIRT_HOST_VALIDATE_LXC) -endif ! WITH_LXC - -if WITH_BHYVE -virt_host_validate_SOURCES += $(VIRT_HOST_VALIDATE_BHYVE) -else ! WITH_BHYVE -EXTRA_DIST += $(VIRT_HOST_VALIDATE_BHYVE) -endif ! WITH_BHYVE - -virt_host_validate_LDFLAGS = \ - $(AM_LDFLAGS) \ - $(PIE_LDFLAGS) \ - $(COVERAGE_LDFLAGS) \ - $(NULL) + $(srcdir)/host-validate/go.mod \ + $(srcdir)/host-validate/go.sum \ + $(srcdir)/host-validate/main.go \ + $(srcdir)/host-validate/pkg/facts.go \ + $(srcdir)/host-validate/pkg/facts_test.go \ + $(srcdir)/host-validate/pkg/engine.go \ + $(NULL) -virt_host_validate_LDADD = \ - ../src/libvirt.la \ - ../gnulib/lib/libgnu.la \ - $(NULL) +virt_host_validate_rulesdir = $(pkgdatadir)/host-validate +virt_host_validate_rules_DATA = \ + $(srcdir)/host-validate/rules/builtin.yaml \ + $(srcdir)/host-validate/rules/cpu.yaml \ + $(srcdir)/host-validate/rules/freebsd-kernel.yaml \ + $(srcdir)/host-validate/rules/linux-acpi.yaml \ + $(srcdir)/host-validate/rules/linux-cgroups.yaml \ + $(srcdir)/host-validate/rules/linux-cpu.yaml \ + $(srcdir)/host-validate/rules/linux-devices.yaml \ + $(srcdir)/host-validate/rules/linux-iommu.yaml \ + $(srcdir)/host-validate/rules/linux-namespaces.yaml \ + $(srcdir)/host-validate/rules/linux-pci.yaml \ + $(NULL) -virt_host_validate_CFLAGS = \ - $(AM_CFLAGS) \ - $(NULL) +EXTRA_DIST += $(virt_host_validate_rules_DATA) $(virt_host_validate_SOURCES) + +virt-host-validate$(EXEEXT): $(virt_host_validate_SOURCES) + $(AM_V_CC) cd $(srcdir)/host-validate && \ + $(GO) build $(GOBUILDFLAGS) -o $(abs_builddir)/$@ main.go +if WITH_HOST_VALIDATE +check-host-validate: + cd $(srcdir)/host-validate && $(GO) test $(GOTESTFLAGS) ./... +else ! WITH_HOST_VALIDATE +check-host-validate: +endif ! WITH_HOST_VALIDATE + +check-local: check-host-validate # virt-login-shell will be setuid, and must not link to anything # except glibc. It wil scrub the environment and then invoke the diff --git a/tools/virt-host-validate.pod b/tools/virt-host-validate.pod index 121bb7ed7a..df10530916 100644 --- a/tools/virt-host-validate.pod +++ b/tools/virt-host-validate.pod @@ -19,9 +19,13 @@ to those relevant for that virtualization technology =over 4 -=item C<-v>, C<--version> +=item C<-f>, C<--facts> -Display the command version +Display all the key, value pairs set for facts + +=item C<-r>, C<--rules-dir> + +Override the default location of the XML rule files =item C<-h>, C<--help> @@ -52,11 +56,11 @@ Alternatively report bugs to your software distributor / vendor. =head1 COPYRIGHT -Copyright (C) 2012 by Red Hat, Inc. +Copyright (C) 2019 by Red Hat, Inc. =head1 LICENSE -virt-host-validate is distributed under the terms of the GNU GPL v2+. +virt-host-validate is distributed under the terms of the GNU LGPL v2.1+. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE -- 2.21.0

The C based virt-host-validate implementation is no longer required so can be deleted entirely. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- po/POTFILES | 5 - tools/virt-host-validate-bhyve.c | 77 ------ tools/virt-host-validate-bhyve.h | 24 -- tools/virt-host-validate-common.c | 419 ------------------------------ tools/virt-host-validate-common.h | 85 ------ tools/virt-host-validate-lxc.c | 87 ------- tools/virt-host-validate-lxc.h | 24 -- tools/virt-host-validate-qemu.c | 116 --------- tools/virt-host-validate-qemu.h | 24 -- tools/virt-host-validate.c | 152 ----------- 10 files changed, 1013 deletions(-) delete mode 100644 tools/virt-host-validate-bhyve.c delete mode 100644 tools/virt-host-validate-bhyve.h delete mode 100644 tools/virt-host-validate-common.c delete mode 100644 tools/virt-host-validate-common.h delete mode 100644 tools/virt-host-validate-lxc.c delete mode 100644 tools/virt-host-validate-lxc.h delete mode 100644 tools/virt-host-validate-qemu.c delete mode 100644 tools/virt-host-validate-qemu.h delete mode 100644 tools/virt-host-validate.c diff --git a/po/POTFILES b/po/POTFILES index 35fc26c4b9..9939114bec 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -311,11 +311,6 @@ tools/virsh-util.c tools/virsh-volume.c tools/virsh.c tools/virt-admin.c -tools/virt-host-validate-bhyve.c -tools/virt-host-validate-common.c -tools/virt-host-validate-lxc.c -tools/virt-host-validate-qemu.c -tools/virt-host-validate.c tools/virt-login-shell.c tools/vsh.c tools/vsh.h diff --git a/tools/virt-host-validate-bhyve.c b/tools/virt-host-validate-bhyve.c deleted file mode 100644 index 2f0ec1e36c..0000000000 --- a/tools/virt-host-validate-bhyve.c +++ /dev/null @@ -1,77 +0,0 @@ -/* - * virt-host-validate-bhyve.c: Sanity check a bhyve hypervisor host - * - * Copyright (C) 2017 Roman Bogorodskiy - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include <config.h> - -#include <sys/param.h> -#include <sys/linker.h> - -#include "virt-host-validate-bhyve.h" -#include "virt-host-validate-common.h" - -#define MODULE_STATUS(mod, err_msg, err_code) \ - virHostMsgCheck("BHYVE", _("for %s module"), #mod); \ - if (mod ## _loaded) { \ - virHostMsgPass(); \ - } else { \ - virHostMsgFail(err_code, \ - _("%s module is not loaded, " err_msg), \ - #mod); \ - ret = -1; \ - } - -#define MODULE_STATUS_FAIL(mod, err_msg) \ - MODULE_STATUS(mod, err_msg, VIR_HOST_VALIDATE_FAIL) - -#define MODULE_STATUS_WARN(mod, err_msg) \ - MODULE_STATUS(mod, err_msg, VIR_HOST_VALIDATE_WARN) - - -int virHostValidateBhyve(void) -{ - int ret = 0; - int fileid = 0; - struct kld_file_stat stat; - bool vmm_loaded = false, if_tap_loaded = false; - bool if_bridge_loaded = false, nmdm_loaded = false; - - for (fileid = kldnext(0); fileid > 0; fileid = kldnext(fileid)) { - stat.version = sizeof(struct kld_file_stat); - if (kldstat(fileid, &stat) < 0) - continue; - - if (STREQ(stat.name, "vmm.ko")) - vmm_loaded = true; - else if (STREQ(stat.name, "if_tap.ko")) - if_tap_loaded = true; - else if (STREQ(stat.name, "if_bridge.ko")) - if_bridge_loaded = true; - else if (STREQ(stat.name, "nmdm.ko")) - nmdm_loaded = true; - } - - MODULE_STATUS_FAIL(vmm, "will not be able to start VMs"); - MODULE_STATUS_WARN(if_tap, "networking will not work"); - MODULE_STATUS_WARN(if_bridge, "bridged networking will not work"); - MODULE_STATUS_WARN(nmdm, "nmdm console will not work"); - - return ret; -} diff --git a/tools/virt-host-validate-bhyve.h b/tools/virt-host-validate-bhyve.h deleted file mode 100644 index a5fd22c871..0000000000 --- a/tools/virt-host-validate-bhyve.h +++ /dev/null @@ -1,24 +0,0 @@ -/* - * virt-host-validate-bhyve.h: Sanity check a bhyve hypervisor host - * - * Copyright (C) 2017 Roman Bogorodskiy - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#pragma once - -int virHostValidateBhyve(void); diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c deleted file mode 100644 index 804c0adc2d..0000000000 --- a/tools/virt-host-validate-common.c +++ /dev/null @@ -1,419 +0,0 @@ -/* - * virt-host-validate-common.c: Sanity check helper APIs - * - * Copyright (C) 2012, 2014 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include <config.h> - -#include <stdarg.h> -#include <unistd.h> -#include <sys/utsname.h> -#include <sys/stat.h> - -#include "viralloc.h" -#include "vircgroup.h" -#include "virfile.h" -#include "virt-host-validate-common.h" -#include "virstring.h" -#include "virarch.h" - -#define VIR_FROM_THIS VIR_FROM_NONE - -VIR_ENUM_IMPL(virHostValidateCPUFlag, - VIR_HOST_VALIDATE_CPU_FLAG_LAST, - "vmx", - "svm", - "sie"); - -static bool quiet; - -void virHostMsgSetQuiet(bool quietFlag) -{ - quiet = quietFlag; -} - -void virHostMsgCheck(const char *prefix, - const char *format, - ...) -{ - va_list args; - char *msg; - - if (quiet) - return; - - va_start(args, format); - if (virVasprintf(&msg, format, args) < 0) { - perror("malloc"); - abort(); - } - va_end(args); - - fprintf(stdout, _("%6s: Checking %-60s: "), prefix, msg); - VIR_FREE(msg); -} - -static bool virHostMsgWantEscape(void) -{ - static bool detectTty = true; - static bool wantEscape; - if (detectTty) { - if (isatty(STDOUT_FILENO)) - wantEscape = true; - detectTty = false; - } - return wantEscape; -} - -void virHostMsgPass(void) -{ - if (quiet) - return; - - if (virHostMsgWantEscape()) - fprintf(stdout, "\033[32m%s\033[0m\n", _("PASS")); - else - fprintf(stdout, "%s\n", _("PASS")); -} - - -static const char * failMessages[] = { - N_("FAIL"), - N_("WARN"), - N_("NOTE"), -}; - -verify(ARRAY_CARDINALITY(failMessages) == VIR_HOST_VALIDATE_LAST); - -static const char *failEscapeCodes[] = { - "\033[31m", - "\033[33m", - "\033[34m", -}; - -verify(ARRAY_CARDINALITY(failEscapeCodes) == VIR_HOST_VALIDATE_LAST); - -void virHostMsgFail(virHostValidateLevel level, - const char *format, - ...) -{ - va_list args; - char *msg; - - if (quiet) - return; - - va_start(args, format); - if (virVasprintf(&msg, format, args) < 0) { - perror("malloc"); - abort(); - } - va_end(args); - - if (virHostMsgWantEscape()) - fprintf(stdout, "%s%s\033[0m (%s)\n", - failEscapeCodes[level], _(failMessages[level]), msg); - else - fprintf(stdout, "%s (%s)\n", - _(failMessages[level]), msg); - VIR_FREE(msg); -} - - -int virHostValidateDeviceExists(const char *hvname, - const char *dev_name, - virHostValidateLevel level, - const char *hint) -{ - virHostMsgCheck(hvname, "if device %s exists", dev_name); - - if (access(dev_name, F_OK) < 0) { - virHostMsgFail(level, "%s", hint); - return -1; - } - - virHostMsgPass(); - return 0; -} - - -int virHostValidateDeviceAccessible(const char *hvname, - const char *dev_name, - virHostValidateLevel level, - const char *hint) -{ - virHostMsgCheck(hvname, "if device %s is accessible", dev_name); - - if (access(dev_name, R_OK|W_OK) < 0) { - virHostMsgFail(level, "%s", hint); - return -1; - } - - virHostMsgPass(); - return 0; -} - - -int virHostValidateNamespace(const char *hvname, - const char *ns_name, - virHostValidateLevel level, - const char *hint) -{ - virHostMsgCheck(hvname, "for namespace %s", ns_name); - char nspath[100]; - - snprintf(nspath, sizeof(nspath), "/proc/self/ns/%s", ns_name); - - if (access(nspath, F_OK) < 0) { - virHostMsgFail(level, "%s", hint); - return -1; - } - - virHostMsgPass(); - return 0; -} - - -virBitmapPtr virHostValidateGetCPUFlags(void) -{ - FILE *fp; - virBitmapPtr flags = NULL; - - if (!(fp = fopen("/proc/cpuinfo", "r"))) - return NULL; - - if (!(flags = virBitmapNewQuiet(VIR_HOST_VALIDATE_CPU_FLAG_LAST))) - goto cleanup; - - do { - char line[1024]; - char *start; - char **tokens; - size_t ntokens; - size_t i; - - if (!fgets(line, sizeof(line), fp)) - break; - - /* The line we're interested in is marked differently depending - * on the architecture, so check possible prefixes */ - if (!STRPREFIX(line, "flags") && - !STRPREFIX(line, "Features") && - !STRPREFIX(line, "features")) - continue; - - /* fgets() includes the trailing newline in the output buffer, - * so we need to clean that up ourselves. We can safely access - * line[strlen(line) - 1] because the checks above would cause - * us to skip empty strings */ - line[strlen(line) - 1] = '\0'; - - /* Skip to the separator */ - if (!(start = strchr(line, ':'))) - continue; - - /* Split the line using " " as a delimiter. The first token - * will always be ":", but that's okay */ - if (!(tokens = virStringSplitCount(start, " ", 0, &ntokens))) - continue; - - /* Go through all flags and check whether one of those we - * might want to check for later on is present; if that's - * the case, set the relevant bit in the bitmap */ - for (i = 0; i < ntokens; i++) { - int value; - - if ((value = virHostValidateCPUFlagTypeFromString(tokens[i])) >= 0) - ignore_value(virBitmapSetBit(flags, value)); - } - - virStringListFreeCount(tokens, ntokens); - } while (1); - - cleanup: - VIR_FORCE_FCLOSE(fp); - - return flags; -} - - -int virHostValidateLinuxKernel(const char *hvname, - int version, - virHostValidateLevel level, - const char *hint) -{ - struct utsname uts; - unsigned long thisversion; - - uname(&uts); - - virHostMsgCheck(hvname, _("for Linux >= %d.%d.%d"), - ((version >> 16) & 0xff), - ((version >> 8) & 0xff), - (version & 0xff)); - - if (STRNEQ(uts.sysname, "Linux")) { - virHostMsgFail(level, "%s", hint); - return -1; - } - - if (virParseVersionString(uts.release, &thisversion, true) < 0) { - virHostMsgFail(level, "%s", hint); - return -1; - } - - if (thisversion < version) { - virHostMsgFail(level, "%s", hint); - return -1; - } else { - virHostMsgPass(); - return 0; - } -} - -#ifdef __linux__ -int virHostValidateCGroupControllers(const char *hvname, - int controllers, - virHostValidateLevel level) -{ - virCgroupPtr group = NULL; - int ret = 0; - size_t i; - - if (virCgroupNewSelf(&group) < 0) - return -1; - - for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { - int flag = 1 << i; - const char *cg_name = virCgroupControllerTypeToString(i); - - if (!(controllers & flag)) - continue; - - virHostMsgCheck(hvname, "for cgroup '%s' controller support", cg_name); - - if (!virCgroupHasController(group, i)) { - ret = -1; - virHostMsgFail(level, "Enable '%s' in kernel Kconfig file or " - "mount/enable cgroup controller in your system", - cg_name); - } else { - virHostMsgPass(); - } - } - - virCgroupFree(&group); - - return ret; -} -#else /* !__linux__ */ -int virHostValidateCGroupControllers(const char *hvname ATTRIBUTE_UNUSED, - int controllers ATTRIBUTE_UNUSED, - virHostValidateLevel level) -{ - virHostMsgFail(level, "%s", "This platform does not support cgroups"); - return -1; -} -#endif /* !__linux__ */ - -int virHostValidateIOMMU(const char *hvname, - virHostValidateLevel level) -{ - virBitmapPtr flags; - struct stat sb; - const char *bootarg = NULL; - bool isAMD = false, isIntel = false; - virArch arch = virArchFromHost(); - struct dirent *dent; - DIR *dir; - int rc; - - flags = virHostValidateGetCPUFlags(); - - if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX)) - isIntel = true; - else if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SVM)) - isAMD = true; - - virBitmapFree(flags); - - if (isIntel) { - virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU support")); - if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0) { - virHostMsgPass(); - bootarg = "intel_iommu=on"; - } else { - virHostMsgFail(level, - "No ACPI DMAR table found, IOMMU either " - "disabled in BIOS or not supported by this " - "hardware platform"); - return -1; - } - } else if (isAMD) { - virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU support")); - if (access("/sys/firmware/acpi/tables/IVRS", F_OK) == 0) { - virHostMsgPass(); - bootarg = "iommu=pt iommu=1"; - } else { - virHostMsgFail(level, - "No ACPI IVRS table found, IOMMU either " - "disabled in BIOS or not supported by this " - "hardware platform"); - return -1; - } - } else if (ARCH_IS_PPC64(arch)) { - /* Empty Block */ - } else if (ARCH_IS_S390(arch)) { - /* On s390x, we skip the IOMMU check if there are no PCI - * devices (which is quite usual on s390x). If there are - * no PCI devices the directory is still there but is - * empty. */ - if (!virDirOpen(&dir, "/sys/bus/pci/devices")) - return 0; - rc = virDirRead(dir, &dent, NULL); - VIR_DIR_CLOSE(dir); - if (rc <= 0) - return 0; - } else { - virHostMsgFail(level, - "Unknown if this platform has IOMMU support"); - return -1; - } - - - /* We can only check on newer kernels with iommu groups & vfio */ - if (stat("/sys/kernel/iommu_groups", &sb) < 0) - return 0; - - if (!S_ISDIR(sb.st_mode)) - return 0; - - virHostMsgCheck(hvname, "%s", _("if IOMMU is enabled by kernel")); - if (sb.st_nlink <= 2) { - if (bootarg) - virHostMsgFail(level, - "IOMMU appears to be disabled in kernel. " - "Add %s to kernel cmdline arguments", bootarg); - else - virHostMsgFail(level, "IOMMU capability not compiled into kernel."); - return -1; - } - virHostMsgPass(); - return 0; -} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h deleted file mode 100644 index c4e4fa2175..0000000000 --- a/tools/virt-host-validate-common.h +++ /dev/null @@ -1,85 +0,0 @@ -/* - * virt-host-validate-common.h: Sanity check helper APIs - * - * Copyright (C) 2012 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#pragma once - -#include "internal.h" -#include "virutil.h" -#include "virbitmap.h" -#include "virenum.h" - -typedef enum { - VIR_HOST_VALIDATE_FAIL, - VIR_HOST_VALIDATE_WARN, - VIR_HOST_VALIDATE_NOTE, - - VIR_HOST_VALIDATE_LAST, -} virHostValidateLevel; - -typedef enum { - VIR_HOST_VALIDATE_CPU_FLAG_VMX = 0, - VIR_HOST_VALIDATE_CPU_FLAG_SVM, - VIR_HOST_VALIDATE_CPU_FLAG_SIE, - - VIR_HOST_VALIDATE_CPU_FLAG_LAST, -} virHostValidateCPUFlag; - -VIR_ENUM_DECL(virHostValidateCPUFlag); - -void virHostMsgSetQuiet(bool quietFlag); - -void virHostMsgCheck(const char *prefix, - const char *format, - ...) ATTRIBUTE_FMT_PRINTF(2, 3); - -void virHostMsgPass(void); -void virHostMsgFail(virHostValidateLevel level, - const char *format, - ...) ATTRIBUTE_FMT_PRINTF(2, 3); - -int virHostValidateDeviceExists(const char *hvname, - const char *dev_name, - virHostValidateLevel level, - const char *hint); - -int virHostValidateDeviceAccessible(const char *hvname, - const char *dev_name, - virHostValidateLevel level, - const char *hint); - -virBitmapPtr virHostValidateGetCPUFlags(void); - -int virHostValidateLinuxKernel(const char *hvname, - int version, - virHostValidateLevel level, - const char *hint); - -int virHostValidateNamespace(const char *hvname, - const char *ns_name, - virHostValidateLevel level, - const char *hint); - -int virHostValidateCGroupControllers(const char *hvname, - int controllers, - virHostValidateLevel level); - -int virHostValidateIOMMU(const char *hvname, - virHostValidateLevel level); diff --git a/tools/virt-host-validate-lxc.c b/tools/virt-host-validate-lxc.c deleted file mode 100644 index 8613f37cc7..0000000000 --- a/tools/virt-host-validate-lxc.c +++ /dev/null @@ -1,87 +0,0 @@ -/* - * virt-host-validate-lxc.c: Sanity check a LXC hypervisor host - * - * Copyright (C) 2012 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include <config.h> - -#include "virt-host-validate-lxc.h" -#include "virt-host-validate-common.h" -#include "vircgroup.h" - -int virHostValidateLXC(void) -{ - int ret = 0; - - if (virHostValidateLinuxKernel("LXC", (2 << 16) | (6 << 8) | 26, - VIR_HOST_VALIDATE_FAIL, - _("Upgrade to a kernel supporting namespaces")) < 0) - ret = -1; - - if (virHostValidateNamespace("LXC", "ipc", - VIR_HOST_VALIDATE_FAIL, - _("IPC namespace support is required")) < 0) - ret = -1; - - if (virHostValidateNamespace("LXC", "mnt", - VIR_HOST_VALIDATE_FAIL, - _("Mount namespace support is required")) < 0) - ret = -1; - - if (virHostValidateNamespace("LXC", "pid", - VIR_HOST_VALIDATE_FAIL, - _("PID namespace support is required")) < 0) - ret = -1; - - if (virHostValidateNamespace("LXC", "uts", - VIR_HOST_VALIDATE_FAIL, - _("UTS namespace support is required")) < 0) - ret = -1; - - if (virHostValidateNamespace("LXC", "net", - VIR_HOST_VALIDATE_WARN, - _("Network namespace support is recommended")) < 0) - ret = -1; - - if (virHostValidateNamespace("LXC", "user", - VIR_HOST_VALIDATE_WARN, - _("User namespace support is recommended")) < 0) - ret = -1; - - if (virHostValidateCGroupControllers("LXC", - (1 << VIR_CGROUP_CONTROLLER_MEMORY) | - (1 << VIR_CGROUP_CONTROLLER_CPU) | - (1 << VIR_CGROUP_CONTROLLER_CPUACCT) | - (1 << VIR_CGROUP_CONTROLLER_CPUSET) | - (1 << VIR_CGROUP_CONTROLLER_DEVICES) | - (1 << VIR_CGROUP_CONTROLLER_FREEZER) | - (1 << VIR_CGROUP_CONTROLLER_BLKIO), - VIR_HOST_VALIDATE_FAIL) < 0) { - ret = -1; - } - -#if WITH_FUSE - if (virHostValidateDeviceExists("LXC", "/sys/fs/fuse/connections", - VIR_HOST_VALIDATE_FAIL, - _("Load the 'fuse' module to enable /proc/ overrides")) < 0) - ret = -1; -#endif - - return ret; -} diff --git a/tools/virt-host-validate-lxc.h b/tools/virt-host-validate-lxc.h deleted file mode 100644 index fefab17552..0000000000 --- a/tools/virt-host-validate-lxc.h +++ /dev/null @@ -1,24 +0,0 @@ -/* - * virt-host-validate-lxc.h: Sanity check a LXC hypervisor host - * - * Copyright (C) 2012 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#pragma once - -int virHostValidateLXC(void); diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c deleted file mode 100644 index ff3c1f0231..0000000000 --- a/tools/virt-host-validate-qemu.c +++ /dev/null @@ -1,116 +0,0 @@ -/* - * virt-host-validate-qemu.c: Sanity check a QEMU hypervisor host - * - * Copyright (C) 2012 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include <config.h> -#include <unistd.h> - -#include "virt-host-validate-qemu.h" -#include "virt-host-validate-common.h" -#include "virarch.h" -#include "virbitmap.h" -#include "vircgroup.h" - -int virHostValidateQEMU(void) -{ - virBitmapPtr flags; - int ret = 0; - bool hasHwVirt = false; - bool hasVirtFlag = false; - virArch arch = virArchFromHost(); - const char *kvmhint = _("Check that CPU and firmware supports virtualization " - "and kvm module is loaded"); - - if (!(flags = virHostValidateGetCPUFlags())) - return -1; - - switch ((int)arch) { - case VIR_ARCH_I686: - case VIR_ARCH_X86_64: - hasVirtFlag = true; - kvmhint = _("Check that the 'kvm-intel' or 'kvm-amd' modules are " - "loaded & the BIOS has enabled virtualization"); - if (virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SVM) || - virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX)) - hasHwVirt = true; - break; - case VIR_ARCH_S390: - case VIR_ARCH_S390X: - hasVirtFlag = true; - if (virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SIE)) - hasHwVirt = true; - break; - default: - hasHwVirt = false; - } - - if (hasVirtFlag) { - virHostMsgCheck("QEMU", "%s", _("for hardware virtualization")); - if (hasHwVirt) { - virHostMsgPass(); - } else { - virHostMsgFail(VIR_HOST_VALIDATE_FAIL, - _("Only emulated CPUs are available, performance will be significantly limited")); - ret = -1; - } - } - - if (hasHwVirt || !hasVirtFlag) { - if (virHostValidateDeviceExists("QEMU", "/dev/kvm", - VIR_HOST_VALIDATE_FAIL, - kvmhint) <0) - ret = -1; - else if (virHostValidateDeviceAccessible("QEMU", "/dev/kvm", - VIR_HOST_VALIDATE_FAIL, - _("Check /dev/kvm is world writable or you are in " - "a group that is allowed to access it")) < 0) - ret = -1; - } - - virBitmapFree(flags); - - if (virHostValidateDeviceExists("QEMU", "/dev/vhost-net", - VIR_HOST_VALIDATE_WARN, - _("Load the 'vhost_net' module to improve performance " - "of virtio networking")) < 0) - ret = -1; - - if (virHostValidateDeviceExists("QEMU", "/dev/net/tun", - VIR_HOST_VALIDATE_FAIL, - _("Load the 'tun' module to enable networking for QEMU guests")) < 0) - ret = -1; - - if (virHostValidateCGroupControllers("QEMU", - (1 << VIR_CGROUP_CONTROLLER_MEMORY) | - (1 << VIR_CGROUP_CONTROLLER_CPU) | - (1 << VIR_CGROUP_CONTROLLER_CPUACCT) | - (1 << VIR_CGROUP_CONTROLLER_CPUSET) | - (1 << VIR_CGROUP_CONTROLLER_DEVICES) | - (1 << VIR_CGROUP_CONTROLLER_BLKIO), - VIR_HOST_VALIDATE_WARN) < 0) { - ret = -1; - } - - if (virHostValidateIOMMU("QEMU", - VIR_HOST_VALIDATE_WARN) < 0) - ret = -1; - - return ret; -} diff --git a/tools/virt-host-validate-qemu.h b/tools/virt-host-validate-qemu.h deleted file mode 100644 index ddb86aa52c..0000000000 --- a/tools/virt-host-validate-qemu.h +++ /dev/null @@ -1,24 +0,0 @@ -/* - * virt-host-validate-qemu.h: Sanity check a QEMU hypervisor host - * - * Copyright (C) 2012 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#pragma once - -int virHostValidateQEMU(void); diff --git a/tools/virt-host-validate.c b/tools/virt-host-validate.c deleted file mode 100644 index e797e63475..0000000000 --- a/tools/virt-host-validate.c +++ /dev/null @@ -1,152 +0,0 @@ -/* - * virt-host-validate.c: Sanity check a hypervisor host - * - * Copyright (C) 2012 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - */ - -#include <config.h> - -#ifdef HAVE_LIBINTL_H -# include <libintl.h> -#endif /* HAVE_LIBINTL_H */ -#include <getopt.h> - -#include "internal.h" -#include "virgettext.h" - -#include "virt-host-validate-common.h" -#if WITH_QEMU -# include "virt-host-validate-qemu.h" -#endif -#if WITH_LXC -# include "virt-host-validate-lxc.h" -#endif -#if WITH_BHYVE -# include "virt-host-validate-bhyve.h" -#endif - -static void -show_help(FILE *out, const char *argv0) -{ - fprintf(out, - _("\n" - "syntax: %s [OPTIONS] [HVTYPE]\n" - "\n" - " Hypervisor types:\n" - "\n" - " - qemu\n" - " - lxc\n" - " - bhyve\n" - "\n" - " Options:\n" - " -h, --help Display command line help\n" - " -v, --version Display command version\n" - " -q, --quiet Don't display progress information\n" - "\n"), - argv0); -} - -static void -show_version(FILE *out, const char *argv0) -{ - fprintf(out, "version: %s %s\n", argv0, VERSION); -} - -static const struct option argOptions[] = { - { "help", 0, NULL, 'h', }, - { "version", 0, NULL, 'v', }, - { "quiet", 0, NULL, 'q', }, - { NULL, 0, NULL, '\0', } -}; - -int -main(int argc, char **argv) -{ - const char *hvname = NULL; - int c; - int ret = EXIT_SUCCESS; - bool quiet = false; - bool usedHvname = false; - - if (virGettextInitialize() < 0) - return EXIT_FAILURE; - - while ((c = getopt_long(argc, argv, "hvq", argOptions, NULL)) != -1) { - switch (c) { - case 'v': - show_version(stdout, argv[0]); - return EXIT_SUCCESS; - - case 'h': - show_help(stdout, argv[0]); - return EXIT_SUCCESS; - - case 'q': - quiet = true; - break; - - case '?': - default: - show_help(stderr, argv[0]); - return EXIT_FAILURE; - } - } - - if ((argc-optind) > 2) { - fprintf(stderr, _("%s: too many command line arguments\n"), argv[0]); - show_help(stderr, argv[0]); - return EXIT_FAILURE; - } - - if (argc > 1) - hvname = argv[optind]; - - virHostMsgSetQuiet(quiet); - -#if WITH_QEMU - if (!hvname || STREQ(hvname, "qemu")) { - usedHvname = true; - if (virHostValidateQEMU() < 0) - ret = EXIT_FAILURE; - } -#endif - -#if WITH_LXC - if (!hvname || STREQ(hvname, "lxc")) { - usedHvname = true; - if (virHostValidateLXC() < 0) - ret = EXIT_FAILURE; - } -#endif - -#if WITH_BHYVE - if (!hvname || STREQ(hvname, "bhyve")) { - usedHvname = true; - if (virHostValidateBhyve() < 0) - ret = EXIT_FAILURE; - } -#endif - - if (hvname && !usedHvname) { - fprintf(stderr, _("%s: unsupported hypervisor name %s\n"), - argv[0], hvname); - return EXIT_FAILURE; - } - - return ret; -} -- 2.21.0

Add a check reporting if any CPU vulnerabilities have not been mitigated by the kernel. It further reports whether it is safe to use Intel SMT for KVM guests or not, as several of the vulnerabilities are dangerous when combined with SMT and KVM, even if mitigations are in effect. eg on a host with mitigations, but unsafe SMT still enabled: Checking CPU hardware vulnerability mitigation...PASS Checking CPU hardware vulnerability SMT safety...FAIL Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- libvirt.spec.in | 1 + tools/Makefile.am | 1 + .../rules/linux-cpu-hardware-flaws.yaml | 165 ++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 tools/host-validate/rules/linux-cpu-hardware-flaws.yaml diff --git a/libvirt.spec.in b/libvirt.spec.in index f336296a08..8aa226798a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1901,6 +1901,7 @@ exit 0 %{_datadir}/libvirt/host-validate/linux-acpi.yaml %{_datadir}/libvirt/host-validate/linux-cgroups.yaml %{_datadir}/libvirt/host-validate/linux-cpu.yaml +%{_datadir}/libvirt/host-validate/linux-cpu-hardware-flaws.yaml %{_datadir}/libvirt/host-validate/linux-devices.yaml %{_datadir}/libvirt/host-validate/linux-iommu.yaml %{_datadir}/libvirt/host-validate/linux-namespaces.yaml diff --git a/tools/Makefile.am b/tools/Makefile.am index 728de475a2..907b0195c2 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -173,6 +173,7 @@ virt_host_validate_rules_DATA = \ $(srcdir)/host-validate/rules/linux-acpi.yaml \ $(srcdir)/host-validate/rules/linux-cgroups.yaml \ $(srcdir)/host-validate/rules/linux-cpu.yaml \ + $(srcdir)/host-validate/rules/linux-cpu-hardware-flaws.yaml \ $(srcdir)/host-validate/rules/linux-devices.yaml \ $(srcdir)/host-validate/rules/linux-iommu.yaml \ $(srcdir)/host-validate/rules/linux-namespaces.yaml \ diff --git a/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml new file mode 100644 index 0000000000..6a243df96d --- /dev/null +++ b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml @@ -0,0 +1,165 @@ +# +# Define facts related to CPU hardware vulnerabilities +# + +facts: +- name: cpu.vulnerability.meltdown + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/meltdown + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.spectre_v1 + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/spectre_v1 + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.spectre_v2 + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/spectre_v2 + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.spec_store_bypass + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/spec_store_bypass + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.mds + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/mds + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.mds_smt + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/mds + ignoreMissing: true + parse: + scalar: + regex: SMT (\w+) + match: 1 +- name: cpu.vulnerability.l1tf + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/l1tf + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.l1tf_smt + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/l1tf + ignoreMissing: true + parse: + scalar: + regex: SMT (\w+) + match: 1 +- name: cpu.vulnerability.unsafe + filter: + fact: + name: os.kernel + value: Linux + report: + message: CPU hardware vulnerability mitigation + pass: false + value: + bool: + any: + expressions: + - fact: + name: cpu.vulnerability.meltdown + value: Vulnerable + - fact: + name: cpu.vulnerability.spectre_v1 + value: Vulnerable + - fact: + name: cpu.vulnerability.spectre_v2 + value: Vulnerable + - fact: + name: cpu.vulnerability.spec_store_bypass + value: Vulnerable + - fact: + name: cpu.vulnerability.mds + value: Vulnerable + - fact: + name: cpu.vulnerability.l1tf + value: Vulnerable +- name: cpu.vulnerability.unsafe_smt + filter: + all: + expressions: + - fact: + name: os.kernel + value: Linux + - fact: + name: cpu.vendor.intel + value: "true" + - fact: + name: cpu.virt.present + value: "true" + report: + message: CPU hardware vulnerability SMT safety + pass: false + value: + bool: + any: + expressions: + - fact: + name: cpu.vulnerability.mds_smt + value: vulnerable + - fact: + name: cpu.vulnerability.l1tf_smt + value: vulnerable -- 2.21.0

On Fri, Sep 27, 2019 at 01:52:25PM +0100, Daniel P. Berrangé wrote:
Add a check reporting if any CPU vulnerabilities have not been mitigated by the kernel. It further reports whether it is safe to use Intel SMT for KVM guests or not, as several of the vulnerabilities are dangerous when combined with SMT and KVM, even if mitigations are in effect.
eg on a host with mitigations, but unsafe SMT still enabled:
Checking CPU hardware vulnerability mitigation...PASS Checking CPU hardware vulnerability SMT safety...FAIL
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- libvirt.spec.in | 1 + tools/Makefile.am | 1 + .../rules/linux-cpu-hardware-flaws.yaml | 165 ++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
diff --git a/libvirt.spec.in b/libvirt.spec.in index f336296a08..8aa226798a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1901,6 +1901,7 @@ exit 0 %{_datadir}/libvirt/host-validate/linux-acpi.yaml %{_datadir}/libvirt/host-validate/linux-cgroups.yaml %{_datadir}/libvirt/host-validate/linux-cpu.yaml +%{_datadir}/libvirt/host-validate/linux-cpu-hardware-flaws.yaml %{_datadir}/libvirt/host-validate/linux-devices.yaml %{_datadir}/libvirt/host-validate/linux-iommu.yaml %{_datadir}/libvirt/host-validate/linux-namespaces.yaml diff --git a/tools/Makefile.am b/tools/Makefile.am index 728de475a2..907b0195c2 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -173,6 +173,7 @@ virt_host_validate_rules_DATA = \ $(srcdir)/host-validate/rules/linux-acpi.yaml \ $(srcdir)/host-validate/rules/linux-cgroups.yaml \ $(srcdir)/host-validate/rules/linux-cpu.yaml \ + $(srcdir)/host-validate/rules/linux-cpu-hardware-flaws.yaml \ $(srcdir)/host-validate/rules/linux-devices.yaml \ $(srcdir)/host-validate/rules/linux-iommu.yaml \ $(srcdir)/host-validate/rules/linux-namespaces.yaml \ diff --git a/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml new file mode 100644 index 0000000000..6a243df96d --- /dev/null +++ b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml @@ -0,0 +1,165 @@ +# +# Define facts related to CPU hardware vulnerabilities +# + +facts: +- name: cpu.vulnerability.meltdown + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/meltdown + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.spectre_v1 + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/spectre_v1 + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.spectre_v2 + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/spectre_v2 + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.spec_store_bypass + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/spec_store_bypass + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.mds + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/mds + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.mds_smt + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/mds + ignoreMissing: true + parse: + scalar: + regex: SMT (\w+) + match: 1 +- name: cpu.vulnerability.l1tf + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/l1tf + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.l1tf_smt + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/l1tf + ignoreMissing: true + parse: + scalar: + regex: SMT (\w+) + match: 1
Given the fact that most of these could just be virFileReadValueUint() it does not even make it easier to read or write the code. Every time someone will want to add a new check or a fact they will need to find a similar one, copy-paste it, change it and hope for the best. This introduces yet another "language" on top of the two you are adding already. I really do not see any benefit in this. If I was to pick a new feature we could benefit from, I would much rather prefer having an opt-in for report-home of HW features and usage for some very rough anonymous statistics. Martin

On Mon, Sep 30, 2019 at 10:55:00AM +0200, Martin Kletzander wrote:
On Fri, Sep 27, 2019 at 01:52:25PM +0100, Daniel P. Berrangé wrote:
Add a check reporting if any CPU vulnerabilities have not been mitigated by the kernel. It further reports whether it is safe to use Intel SMT for KVM guests or not, as several of the vulnerabilities are dangerous when combined with SMT and KVM, even if mitigations are in effect.
eg on a host with mitigations, but unsafe SMT still enabled:
Checking CPU hardware vulnerability mitigation...PASS Checking CPU hardware vulnerability SMT safety...FAIL
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- libvirt.spec.in | 1 + tools/Makefile.am | 1 + .../rules/linux-cpu-hardware-flaws.yaml | 165 ++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
diff --git a/libvirt.spec.in b/libvirt.spec.in index f336296a08..8aa226798a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1901,6 +1901,7 @@ exit 0 %{_datadir}/libvirt/host-validate/linux-acpi.yaml %{_datadir}/libvirt/host-validate/linux-cgroups.yaml %{_datadir}/libvirt/host-validate/linux-cpu.yaml +%{_datadir}/libvirt/host-validate/linux-cpu-hardware-flaws.yaml %{_datadir}/libvirt/host-validate/linux-devices.yaml %{_datadir}/libvirt/host-validate/linux-iommu.yaml %{_datadir}/libvirt/host-validate/linux-namespaces.yaml diff --git a/tools/Makefile.am b/tools/Makefile.am index 728de475a2..907b0195c2 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -173,6 +173,7 @@ virt_host_validate_rules_DATA = \ $(srcdir)/host-validate/rules/linux-acpi.yaml \ $(srcdir)/host-validate/rules/linux-cgroups.yaml \ $(srcdir)/host-validate/rules/linux-cpu.yaml \ + $(srcdir)/host-validate/rules/linux-cpu-hardware-flaws.yaml \ $(srcdir)/host-validate/rules/linux-devices.yaml \ $(srcdir)/host-validate/rules/linux-iommu.yaml \ $(srcdir)/host-validate/rules/linux-namespaces.yaml \ diff --git a/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml new file mode 100644 index 0000000000..6a243df96d --- /dev/null +++ b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml @@ -0,0 +1,165 @@ +# +# Define facts related to CPU hardware vulnerabilities +# + +facts: +- name: cpu.vulnerability.meltdown + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/meltdown + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.spectre_v1 + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/spectre_v1 + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.spectre_v2 + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/spectre_v2 + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.spec_store_bypass + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/spec_store_bypass + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.mds + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/mds + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.mds_smt + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/mds + ignoreMissing: true + parse: + scalar: + regex: SMT (\w+) + match: 1 +- name: cpu.vulnerability.l1tf + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/l1tf + ignoreMissing: true + parse: + scalar: + regex: (\w+) + match: 1 +- name: cpu.vulnerability.l1tf_smt + filter: + fact: + name: os.kernel + value: Linux + value: + file: + path: /sys/devices/system/cpu/vulnerabilities/l1tf + ignoreMissing: true + parse: + scalar: + regex: SMT (\w+) + match: 1
Given the fact that most of these could just be virFileReadValueUint() it does not even make it easier to read or write the code.
Errr, virFileReadValueUint reads a single integer from a file. This is extracting a single *word* from a specific field in a file. Doing this in code would involve doing a regex match the same as here.
Every time someone will want to add a new check or a fact they will need to find a similar one, copy-paste it, change it and hope for the best. This introduces yet another "language" on top of the two you are adding already. I really do not see any benefit in this.
Taking this particular example. - name: cpu.vulnerability.l1tf_smt filter: fact: name: os.kernel value: Linux value: file: path: /sys/devices/system/cpu/vulnerabilities/l1tf ignoreMissing: true parse: scalar: regex: SMT (\w+) match: 1 It could be approximately equiv to code if runtime.GOOS == "Linux" { return "", nil } data, err = ioutils.ReadFile("/sys/devices/system/cpu/vulnerabilities/l1tf") if err != nil { if os.IsNotExist(err) { return "", nil } return "", err } match, err := regexp.Find(`SMT (\w+)`, data) if err != nil { return "", err } err = SetFact("cpu.vulnerability.l1tf_smt", match) if err != nil { return "", err } The code does look more familiar, but it is certainly not simpler. Most notabley there are alot of error handling cases here that need to be dealt with, even when written in Go. Doing this in C with its string handling support involves even more error handling. This is a significant maintainence burden that is not well handled in a consistent manner across the code. This is a fairly simple check example, case where we want to extract data from a more more complex file formats result in much more code complexity. In the pure code approach, we have mixed up code related to reading files, running commands, parsing data formats, setting facts, printing messages, skipping execution when certain other conditions applied. Hidden in this is the information about what feature we are actually checking for. A data driven approach to this problem means that we get clear separation between distinct parts/jobs. The engine written in code concerns itself with generic facilities for reading files, running commands, parsing data, and all the error handling and data processing. Thereafter the interesting checks is simply a declarative expression of what data we wish to extract for each fature, and declarative expression of dependancies between features. This engine code has greater complexity than what exists today for sure, but the key thing is that this is mostly a one time upfront code, and is the least interesting part of the tool. What is most interest is the set of features being checked for and the relationships between the features. Having the declarative part separated from the functional part is where the value arises. - Changes to the functional code apply consistently across all the checks made. For example, an improvement to error handling to regex processing was done once and applied to all the checks that used regex. Or consistently handling when reading files that are potentially missing on systems. - New checks can be introduced in a more reliable and consistent manner. These CPU vulnerability checks are an example of this in practice. We merely have to express what files we're interested in and how to match the relevant data in them. No need to worry about reading files, compiling & running regexs, making sure the error handling was done in the same way as other existing checks. - The declarative data can be processed algorithmically. For example given the depedancies expressed between the facts, I saw that it was not neccessary to define all the facts in the same data file. It was possible split them across many files, load those files in any order, and algorithmically determine exactly which subset of checks need to be executed & in what order. - Having separated data from the code it is obviously possible to extend this without introducing any code changes. This is possible to use from outside libvirt. For example there are usage scenarios which may not be supported by certain versions of QEMU. The QEMU package can drop in some facts which will be picked up by the virt-host-validate tool. Alternatively going up the stack, an app like OpenStack which uses libvirt may wish to restrict what they use, or wish to check extra virt host features interesting to their app's usage of libvirt. Again their package can now drop in extra facts. - New features can be built ontop of the declarative data. At first I just set out to replicate the existing tool's output as is. Once that was done, it was obvious that we could trivially add a new feature allowing us to print the raw informationm about each fact that was checked, as an alternative to the human targetted message strings. IOW we got a machine readable output format essentially for free by virtue of using declarative data. - The implementation can be rewritten again at will. If the original C code had this split of data vs processing logic, this conversion of langauge would have been even more compelling, as none of othe declarative data would have needed changing. I'm not expecting another rewrite, but this capability was already valuable, between the v1 and v2 posting of this patch I changed from XML to YAML for the data format. This conversion was entirely automated, because it could be algorithmically processed. These are all typical & expected benefits that arise from separating functional logic from declarative data. In the long term this is a clear win, & as I mentioned I would have done this sooner had it not been for our use of C.
If I was to pick a new feature we could benefit from, I would much rather prefer having an opt-in for report-home of HW features and usage for some very rough anonymous statistics.
I don't think we want to get involved in reporting user data back to any server, as it is a data privacy minefield. This tool can be used by other existing reporting tools though - eg Red Hat distros often include the 'sosreport' tool that bundles stuff up for reporting customer tickets. 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 :|

On Mon, 2019-09-30 at 15:30 +0100, Daniel P. Berrangé wrote:
On Mon, Sep 30, 2019 at 10:55:00AM +0200, Martin Kletzander wrote:
Given the fact that most of these could just be virFileReadValueUint() it does not even make it easier to read or write the code.
Errr, virFileReadValueUint reads a single integer from a file.
This is extracting a single *word* from a specific field in a file.
Doing this in code would involve doing a regex match the same as here.
Every time someone will want to add a new check or a fact they will need to find a similar one, copy-paste it, change it and hope for the best. This introduces yet another "language" on top of the two you are adding already. I really do not see any benefit in this.
Taking this particular example.
- name: cpu.vulnerability.l1tf_smt filter: fact: name: os.kernel value: Linux value: file: path: /sys/devices/system/cpu/vulnerabilities/l1tf ignoreMissing: true parse: scalar: regex: SMT (\w+) match: 1
It could be approximately equiv to code
if runtime.GOOS == "Linux" { return "", nil }
data, err = ioutils.ReadFile("/sys/devices/system/cpu/vulnerabilities/l1tf") if err != nil { if os.IsNotExist(err) { return "", nil } return "", err }
match, err := regexp.Find(`SMT (\w+)`, data) if err != nil { return "", err } err = SetFact("cpu.vulnerability.l1tf_smt", match) if err != nil { return "", err }
The code does look more familiar, but it is certainly not simpler. Most notabley there are alot of error handling cases here that need to be dealt with, even when written in Go. Doing this in C with its string handling support involves even more error handling. This is a significant maintainence burden that is not well handled in a consistent manner across the code.
This is a fairly simple check example, case where we want to extract data from a more more complex file formats result in much more code complexity.
In the pure code approach, we have mixed up code related to reading files, running commands, parsing data formats, setting facts, printing messages, skipping execution when certain other conditions applied. Hidden in this is the information about what feature we are actually checking for.
A data driven approach to this problem means that we get clear separation between distinct parts/jobs.
The engine written in code concerns itself with generic facilities for reading files, running commands, parsing data, and all the error handling and data processing.
Thereafter the interesting checks is simply a declarative expression of what data we wish to extract for each fature, and declarative expression of dependancies between features.
This engine code has greater complexity than what exists today for sure, but the key thing is that this is mostly a one time upfront code, and is the least interesting part of the tool. What is most interest is the set of features being checked for and the relationships between the features.
Having the declarative part separated from the functional part is where the value arises.
- Changes to the functional code apply consistently across all the checks made. For example, an improvement to error handling to regex processing was done once and applied to all the checks that used regex. Or consistently handling when reading files that are potentially missing on systems.
- New checks can be introduced in a more reliable and consistent manner. These CPU vulnerability checks are an example of this in practice. We merely have to express what files we're interested in and how to match the relevant data in them. No need to worry about reading files, compiling & running regexs, making sure the error handling was done in the same way as other existing checks.
All of the above mostly sounds like "use functions" to me. Something like value, err := GetIntFactFromFile( "/sys/devices/system/cpu/vulnerabilities/l1tf", "SMT (\w+)", OsLinux, IgnoreMissing, ) if err != nil { return err } SetIntFact("cpu.vulnerability.l1tf_smt", value) requires basically as much upfront knowledge to understand as the YAML version, but doesn't introduce a new DSL. And hey, it's shorter than either version! :)
- The declarative data can be processed algorithmically. For example given the depedancies expressed between the facts, I saw that it was not neccessary to define all the facts in the same data file. It was possible split them across many files, load those files in any order, and algorithmically determine exactly which subset of checks need to be executed & in what order.
- Having separated data from the code it is obviously possible to extend this without introducing any code changes. This is possible to use from outside libvirt. For example there are usage scenarios which may not be supported by certain versions of QEMU. The QEMU package can drop in some facts which will be picked up by the virt-host-validate tool. Alternatively going up the stack, an app like OpenStack which uses libvirt may wish to restrict what they use, or wish to check extra virt host features interesting to their app's usage of libvirt. Again their package can now drop in extra facts.
This last point is the only advantage I can see very clearly. I'm not sure it's worth introducing a specific format for, though.
- New features can be built ontop of the declarative data. At first I just set out to replicate the existing tool's output as is. Once that was done, it was obvious that we could trivially add a new feature allowing us to print the raw informationm about each fact that was checked, as an alternative to the human targetted message strings. IOW we got a machine readable output format essentially for free by virtue of using declarative data.
This is a benefit of storing the information as facts, which as I said before I think is a good idea; it doesn't mean we have to obtain said facts by processing YAML files.
- The implementation can be rewritten again at will. If the original C code had this split of data vs processing logic, this conversion of langauge would have been even more compelling, as none of othe declarative data would have needed changing. I'm not expecting another rewrite, but this capability was already valuable, between the v1 and v2 posting of this patch I changed from XML to YAML for the data format. This conversion was entirely automated, because it could be algorithmically processed.
If you had used code instead of a pure data format as the source for information, then you wouldn't have needed to perform that conversion in the first place O:-)
These are all typical & expected benefits that arise from separating functional logic from declarative data. In the long term this is a clear win, & as I mentioned I would have done this sooner had it not been for our use of C.
I expect that if you would have posted patches introducing this machinery to the C version of virt-host-validate you would have seen more or less the same reactions.
If I was to pick a new feature we could benefit from, I would much rather prefer having an opt-in for report-home of HW features and usage for some very rough anonymous statistics.
I don't think we want to get involved in reporting user data back to any server, as it is a data privacy minefield. This tool can be used by other existing reporting tools though - eg Red Hat distros often include the 'sosreport' tool that bundles stuff up for reporting customer tickets.
Wholeheartedly agreed. -- Andrea Bolognani / Red Hat / Virtualization

On Mon, Sep 30, 2019 at 05:10:10PM +0200, Andrea Bolognani wrote:
On Mon, 2019-09-30 at 15:30 +0100, Daniel P. Berrangé wrote:
On Mon, Sep 30, 2019 at 10:55:00AM +0200, Martin Kletzander wrote:
Given the fact that most of these could just be virFileReadValueUint() it does not even make it easier to read or write the code.
Errr, virFileReadValueUint reads a single integer from a file.
This is extracting a single *word* from a specific field in a file.
Doing this in code would involve doing a regex match the same as here.
Every time someone will want to add a new check or a fact they will need to find a similar one, copy-paste it, change it and hope for the best. This introduces yet another "language" on top of the two you are adding already. I really do not see any benefit in this.
Taking this particular example.
- name: cpu.vulnerability.l1tf_smt filter: fact: name: os.kernel value: Linux value: file: path: /sys/devices/system/cpu/vulnerabilities/l1tf ignoreMissing: true parse: scalar: regex: SMT (\w+) match: 1
It could be approximately equiv to code
if runtime.GOOS == "Linux" { return "", nil }
data, err = ioutils.ReadFile("/sys/devices/system/cpu/vulnerabilities/l1tf") if err != nil { if os.IsNotExist(err) { return "", nil } return "", err }
match, err := regexp.Find(`SMT (\w+)`, data) if err != nil { return "", err } err = SetFact("cpu.vulnerability.l1tf_smt", match) if err != nil { return "", err }
The code does look more familiar, but it is certainly not simpler. Most notabley there are alot of error handling cases here that need to be dealt with, even when written in Go. Doing this in C with its string handling support involves even more error handling. This is a significant maintainence burden that is not well handled in a consistent manner across the code.
This is a fairly simple check example, case where we want to extract data from a more more complex file formats result in much more code complexity.
In the pure code approach, we have mixed up code related to reading files, running commands, parsing data formats, setting facts, printing messages, skipping execution when certain other conditions applied. Hidden in this is the information about what feature we are actually checking for.
A data driven approach to this problem means that we get clear separation between distinct parts/jobs.
The engine written in code concerns itself with generic facilities for reading files, running commands, parsing data, and all the error handling and data processing.
Thereafter the interesting checks is simply a declarative expression of what data we wish to extract for each fature, and declarative expression of dependancies between features.
This engine code has greater complexity than what exists today for sure, but the key thing is that this is mostly a one time upfront code, and is the least interesting part of the tool. What is most interest is the set of features being checked for and the relationships between the features.
Having the declarative part separated from the functional part is where the value arises.
- Changes to the functional code apply consistently across all the checks made. For example, an improvement to error handling to regex processing was done once and applied to all the checks that used regex. Or consistently handling when reading files that are potentially missing on systems.
- New checks can be introduced in a more reliable and consistent manner. These CPU vulnerability checks are an example of this in practice. We merely have to express what files we're interested in and how to match the relevant data in them. No need to worry about reading files, compiling & running regexs, making sure the error handling was done in the same way as other existing checks.
All of the above mostly sounds like "use functions" to me. Something like
value, err := GetIntFactFromFile( "/sys/devices/system/cpu/vulnerabilities/l1tf", "SMT (\w+)", OsLinux, IgnoreMissing, ) if err != nil { return err } SetIntFact("cpu.vulnerability.l1tf_smt", value)
requires basically as much upfront knowledge to understand as the YAML version, but doesn't introduce a new DSL. And hey, it's shorter than either version! :)
"use functions" is another way of saying separate the declarative data from the functional logic. This series has indeed taken this approach and has such functions in the data processing engine. They look more complex than what you've illustrated here, because they cope with many more scenario. Those set of args you've shown and just one combination of many possibilities. So instead of passing the arguments inline like this, you'll want to define a struct to hold the arguments & just pass a struct to the function. This is such a good idea this impl already defines such structs to hold the data. Once you've got this nice separate of data and code, loading the data from a file is just one tiny step more to make a more flexible tool allowing drop in extensions.
- The declarative data can be processed algorithmically. For example given the depedancies expressed between the facts, I saw that it was not neccessary to define all the facts in the same data file. It was possible split them across many files, load those files in any order, and algorithmically determine exactly which subset of checks need to be executed & in what order.
- Having separated data from the code it is obviously possible to extend this without introducing any code changes. This is possible to use from outside libvirt. For example there are usage scenarios which may not be supported by certain versions of QEMU. The QEMU package can drop in some facts which will be picked up by the virt-host-validate tool. Alternatively going up the stack, an app like OpenStack which uses libvirt may wish to restrict what they use, or wish to check extra virt host features interesting to their app's usage of libvirt. Again their package can now drop in extra facts.
This last point is the only advantage I can see very clearly. I'm not sure it's worth introducing a specific format for, though.
- New features can be built ontop of the declarative data. At first I just set out to replicate the existing tool's output as is. Once that was done, it was obvious that we could trivially add a new feature allowing us to print the raw informationm about each fact that was checked, as an alternative to the human targetted message strings. IOW we got a machine readable output format essentially for free by virtue of using declarative data.
This is a benefit of storing the information as facts, which as I said before I think is a good idea; it doesn't mean we have to obtain said facts by processing YAML files.
Of course you could define everything via a set of structs in the code, but its crazy to do that as you've now hardcoded everything at build time, pointlessly throwing away the key extensibility benefit of having it defined via a data file.
- The implementation can be rewritten again at will. If the original C code had this split of data vs processing logic, this conversion of langauge would have been even more compelling, as none of othe declarative data would have needed changing. I'm not expecting another rewrite, but this capability was already valuable, between the v1 and v2 posting of this patch I changed from XML to YAML for the data format. This conversion was entirely automated, because it could be algorithmically processed.
If you had used code instead of a pure data format as the source for information, then you wouldn't have needed to perform that conversion in the first place O:-)
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 :|

On Mon, Sep 30, 2019 at 04:30:18PM +0100, Daniel P. Berrangé wrote:
On Mon, Sep 30, 2019 at 05:10:10PM +0200, Andrea Bolognani wrote:
On Mon, 2019-09-30 at 15:30 +0100, Daniel P. Berrangé wrote:
- Having separated data from the code it is obviously possible to extend this without introducing any code changes. This is possible to use from outside libvirt. For example there are usage scenarios which may not be supported by certain versions of QEMU. The QEMU package can drop in some facts which will be picked up by the virt-host-validate tool. Alternatively going up the stack, an app like OpenStack which uses libvirt may wish to restrict what they use, or wish to check extra virt host features interesting to their app's usage of libvirt. Again their package can now drop in extra facts.
This last point is the only advantage I can see very clearly. I'm not sure it's worth introducing a specific format for, though.
Yes, this one I haven't thought of and it makes sense. It also makes way more sense for it not to be a part of libvirt, but rather a separate project as if the support for loading other files from elsewhere is added, there is no need for this to be a part of libvirt any more. Which would immediately overrule (almost) all issues I have seen with this.
- New features can be built ontop of the declarative data. At first I just set out to replicate the existing tool's output as is. Once that was done, it was obvious that we could trivially add a new feature allowing us to print the raw informationm about each fact that was checked, as an alternative to the human targetted message strings. IOW we got a machine readable output format essentially for free by virtue of using declarative data.
This is a benefit of storing the information as facts, which as I said before I think is a good idea; it doesn't mean we have to obtain said facts by processing YAML files.
Of course you could define everything via a set of structs in the code, but its crazy to do that as you've now hardcoded everything at build time, pointlessly throwing away the key extensibility benefit of having it defined via a data file.
Except when you are using extensive conditionals or building the fact from multiple pieces information using, for example, boolean expressions. At that point you are creating a programming language on top of markup language.
- The implementation can be rewritten again at will. If the original C code had this split of data vs processing logic, this conversion of langauge would have been even more compelling, as none of othe declarative data would have needed changing. I'm not expecting another rewrite, but this capability was already valuable, between the v1 and v2 posting of this patch I changed from XML to YAML for the data format. This conversion was entirely automated, because it could be algorithmically processed.
If you had used code instead of a pure data format as the source for information, then you wouldn't have needed to perform that conversion in the first place O:-)
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 :|

On Tue, 2019-10-01 at 16:44 +0200, Martin Kletzander wrote:
On Mon, Sep 30, 2019 at 04:30:18PM +0100, Daniel P. Berrangé wrote:
Of course you could define everything via a set of structs in the code, but its crazy to do that as you've now hardcoded everything at build time, pointlessly throwing away the key extensibility benefit of having it defined via a data file.
Except when you are using extensive conditionals or building the fact from multiple pieces information using, for example, boolean expressions. At that point you are creating a programming language on top of markup language.
I agree. It just occurred to me that this is pretty much exactly what Ansible does. It makes sense for them to do so, because the primary use case is for people to define their own rules, so the people writing YAML will far outnumber those responsible of the underlying Python engine. I don't think this would be the case for virt-host-validate. -- Andrea Bolognani / Red Hat / Virtualization

On Fri, Sep 27, 2019 at 01:52:19PM +0100, Daniel P. Berrangé wrote:
This is a followup to a previous PoC patch I submitted a month ago:
https://www.redhat.com/archives/libvir-list/2019-September/msg00036.html
The commit messages in the individual patches given quite a bit of detail, so I'll keep this cover letter brief.
In my previous posting I was unhappy with the implications for the RPM packaging, and was considering having this as a separate source repo & RPM. On further investigation such an approach would not in fact solve the RPM packaging problem, because we would still not be using a pure go build toolchain, as we have data files that need installing in the right place.
This forced me to actually address the RPM packaging problems that Fedora had with Go when used from a build tool like make or meson.
After alot of debugging I finally got a viable solution merged into the Fedora go-rpm-macros package:
https://pagure.io/go-rpm-macros/c/67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2
commit 67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Wed Sep 18 16:49:58 2019 +0100
macros: define a %gobuildflags macro
Using the %gobuild macro is fine for a project where the go code is the only thing being built, and can be built directly by invoking the Go toolchain from RPM.
In more complex cases though, the Go code is just a small part of the project and the Go toolchain is invoked by a build system such as make (possibly automake), or meson. In such a case we need to be able to tell this build system what flags to pass to the compiler.
The %gobuildflags macros services this purpose allowing a RPM spec todo
GOBUILDFLAGS="%gobuildflags" %configure
or
%make GOBUILDFLAGS="%gobuildflags"
Ideally the %gobuild macro would in turn reference the %gobuildflags macro, but that does not appear possible given the semantics around quote expansion and escaping across RPM and shell.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
As a result in this series, we're now fully integrated into the RPM build, on Fedora at least. I've not checked what approach RHEL takes for Go, whether it requires separate RPM for each 3rd party dep, or prefers bundling. Either way though, we can deal with the problem now.
The other obvious change is that this is now a patch series, to make it easier to review the code in managable chunks.
The really big difference though is that I replaced the use of XML data files with YAML data files. This was done with the aim of making the data more human friendly. XML is really optimized for machines, not humans, so writing the data files was not pretty. YAML is optimized for human readability, and is actually even easier to consume in Go than the XML was, so its a double win.
Finally, we also add new checks at the end for the various CPU hardware side channel mitigations, and report whether SMT/HT is unsafe or not (any Intel host is basically unsafe before Icelake).
Daniel P. Berrangé (6): build: introduce logic for using golang in libvirt tools: introduce a data driven impl of virt-host-validate tools: define YAML rules for virt-host-validate checks tools: switch to build the new virt-host-validate impl tools: delete the old virt-host-validate impl tools: make virt-host-validate check CPU vulnerabilities
configure.ac | 1 + libvirt.spec.in | 35 +- m4/virt-golang.m4 | 46 ++ m4/virt-host-validate.m4 | 8 +- po/POTFILES | 5 - tools/Makefile.am | 76 +-- tools/host-validate/go.mod | 10 + tools/host-validate/go.sum | 9 + tools/host-validate/main.go | 98 +++ tools/host-validate/pkg/engine.go | 481 ++++++++++++++ tools/host-validate/pkg/facts.go | 585 ++++++++++++++++++ .../pkg/facts_test.go} | 36 +- tools/host-validate/rules/builtin.yaml | 20 + tools/host-validate/rules/cpu.yaml | 50 ++ tools/host-validate/rules/freebsd-kernel.yaml | 77 +++ tools/host-validate/rules/linux-acpi.yaml | 39 ++ tools/host-validate/rules/linux-cgroups.yaml | 470 ++++++++++++++ .../rules/linux-cpu-hardware-flaws.yaml | 165 +++++ tools/host-validate/rules/linux-cpu.yaml | 134 ++++ tools/host-validate/rules/linux-devices.yaml | 71 +++ tools/host-validate/rules/linux-iommu.yaml | 113 ++++ .../host-validate/rules/linux-namespaces.yaml | 119 ++++ tools/host-validate/rules/linux-pci.yaml | 10 + tools/virt-host-validate-bhyve.c | 77 --- tools/virt-host-validate-common.c | 419 ------------- tools/virt-host-validate-common.h | 85 --- tools/virt-host-validate-lxc.c | 87 --- tools/virt-host-validate-lxc.h | 24 - tools/virt-host-validate-qemu.c | 116 ---- tools/virt-host-validate-qemu.h | 24 - tools/virt-host-validate.c | 152 ----- tools/virt-host-validate.pod | 12 +- 32 files changed, 2609 insertions(+), 1045 deletions(-)
So this ^^ plus: 2 languages added, 0 languages removed makes me feel like this goes against what you were trying to do in another series. I understand that adding new fact checks is "easier" and does not require recompilation, but I don't see any use for that benefit. I went through the cover letters for both series just to find a reason for it and I didn't. Sorry, but I don't really like this.

On Mon, Sep 30, 2019 at 10:47:39AM +0200, Martin Kletzander wrote:
On Fri, Sep 27, 2019 at 01:52:19PM +0100, Daniel P. Berrangé wrote:
This is a followup to a previous PoC patch I submitted a month ago:
https://www.redhat.com/archives/libvir-list/2019-September/msg00036.html
The commit messages in the individual patches given quite a bit of detail, so I'll keep this cover letter brief.
In my previous posting I was unhappy with the implications for the RPM packaging, and was considering having this as a separate source repo & RPM. On further investigation such an approach would not in fact solve the RPM packaging problem, because we would still not be using a pure go build toolchain, as we have data files that need installing in the right place.
This forced me to actually address the RPM packaging problems that Fedora had with Go when used from a build tool like make or meson.
After alot of debugging I finally got a viable solution merged into the Fedora go-rpm-macros package:
https://pagure.io/go-rpm-macros/c/67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2
commit 67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Wed Sep 18 16:49:58 2019 +0100
macros: define a %gobuildflags macro
Using the %gobuild macro is fine for a project where the go code is the only thing being built, and can be built directly by invoking the Go toolchain from RPM.
In more complex cases though, the Go code is just a small part of the project and the Go toolchain is invoked by a build system such as make (possibly automake), or meson. In such a case we need to be able to tell this build system what flags to pass to the compiler.
The %gobuildflags macros services this purpose allowing a RPM spec todo
GOBUILDFLAGS="%gobuildflags" %configure
or
%make GOBUILDFLAGS="%gobuildflags"
Ideally the %gobuild macro would in turn reference the %gobuildflags macro, but that does not appear possible given the semantics around quote expansion and escaping across RPM and shell.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
As a result in this series, we're now fully integrated into the RPM build, on Fedora at least. I've not checked what approach RHEL takes for Go, whether it requires separate RPM for each 3rd party dep, or prefers bundling. Either way though, we can deal with the problem now.
The other obvious change is that this is now a patch series, to make it easier to review the code in managable chunks.
The really big difference though is that I replaced the use of XML data files with YAML data files. This was done with the aim of making the data more human friendly. XML is really optimized for machines, not humans, so writing the data files was not pretty. YAML is optimized for human readability, and is actually even easier to consume in Go than the XML was, so its a double win.
Finally, we also add new checks at the end for the various CPU hardware side channel mitigations, and report whether SMT/HT is unsafe or not (any Intel host is basically unsafe before Icelake).
Daniel P. Berrangé (6): build: introduce logic for using golang in libvirt tools: introduce a data driven impl of virt-host-validate tools: define YAML rules for virt-host-validate checks tools: switch to build the new virt-host-validate impl tools: delete the old virt-host-validate impl tools: make virt-host-validate check CPU vulnerabilities
configure.ac | 1 + libvirt.spec.in | 35 +- m4/virt-golang.m4 | 46 ++ m4/virt-host-validate.m4 | 8 +- po/POTFILES | 5 - tools/Makefile.am | 76 +-- tools/host-validate/go.mod | 10 + tools/host-validate/go.sum | 9 + tools/host-validate/main.go | 98 +++ tools/host-validate/pkg/engine.go | 481 ++++++++++++++ tools/host-validate/pkg/facts.go | 585 ++++++++++++++++++ .../pkg/facts_test.go} | 36 +- tools/host-validate/rules/builtin.yaml | 20 + tools/host-validate/rules/cpu.yaml | 50 ++ tools/host-validate/rules/freebsd-kernel.yaml | 77 +++ tools/host-validate/rules/linux-acpi.yaml | 39 ++ tools/host-validate/rules/linux-cgroups.yaml | 470 ++++++++++++++ .../rules/linux-cpu-hardware-flaws.yaml | 165 +++++ tools/host-validate/rules/linux-cpu.yaml | 134 ++++ tools/host-validate/rules/linux-devices.yaml | 71 +++ tools/host-validate/rules/linux-iommu.yaml | 113 ++++ .../host-validate/rules/linux-namespaces.yaml | 119 ++++ tools/host-validate/rules/linux-pci.yaml | 10 + tools/virt-host-validate-bhyve.c | 77 --- tools/virt-host-validate-common.c | 419 ------------- tools/virt-host-validate-common.h | 85 --- tools/virt-host-validate-lxc.c | 87 --- tools/virt-host-validate-lxc.h | 24 - tools/virt-host-validate-qemu.c | 116 ---- tools/virt-host-validate-qemu.h | 24 - tools/virt-host-validate.c | 152 ----- tools/virt-host-validate.pod | 12 +- 32 files changed, 2609 insertions(+), 1045 deletions(-)
So this ^^ plus:
2 languages added, 0 languages removed
makes me feel like this goes against what you were trying to do in another series. I understand that adding new fact checks is "easier" and does not require recompilation, but I don't see any use for that benefit. I went through the cover letters for both series just to find a reason for it and I didn't.
The language consolidation is not about reducing languages as an absolute. Where we were using multiple langauges, to tackle problems in the same problem space, we consolidated on one language for consistency. This opens the door to adding use of other languages in libvirt, without continually increasing the burden of developers. The mix of Perl, Python & Shell all for doing misc build scripts was the prime example as there was nothing that can be done in Perl or SHell that could not be done just as well in Python. There's no justification for using all three languages in this context. In this case, our existing C language for writing our production code is not well suited for the task we're applying it too. You could make an argument that the virt-host-validate could be written in Python, but I think it is preferred to keep all our deployable code written in compiled languages, leaving the scripting languages for our supporting build system. Counting the addition of another language solely against this tool is not a reflection on the long term direction of libvirt code. It is highly likely that we'll incorporate Go code for more use cases over time as it is a good systems programming language, able to replace C where Python could not. The choice of YAML is a valid point to use as that is definitely adding a new language where one already exists (XML) that is expressive enough to cope with the problem. As you saw the original v1 did indeed use XML for this reason. The decision to try YAML was an experiment to see if the simplicity and readability of YAML would outweigh the cost of having another data language. Personally I think it is a net win to use YAML instead of XML, but it would not be the end of the world to go back to the v1 approach using XML if people really want a less readable language just to not have to add another data language. 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 :|

On Mon, Sep 30, 2019 at 15:48:29 +0100, Daniel Berrange wrote:
On Mon, Sep 30, 2019 at 10:47:39AM +0200, Martin Kletzander wrote:
On Fri, Sep 27, 2019 at 01:52:19PM +0100, Daniel P. Berrangé wrote:
This is a followup to a previous PoC patch I submitted a month ago:
[...]
The choice of YAML is a valid point to use as that is definitely adding a new language where one already exists (XML) that is expressive enough to cope with the problem. As you saw the original v1 did indeed use XML for this reason.
The decision to try YAML was an experiment to see if the simplicity and readability of YAML would outweigh the cost of having another data language. Personally I think it is a net win to use YAML instead of XML, but it would not be the end of the world to go back to the v1 approach using XML if people really want a less readable language just to not have to add another data language.
In my opinion this is a misunderstanding. Whether it's YAML or XML used for syntax is not that important. What is important is that the new 'language' is the custom declarative language which uses YAML or XML to express the constructions. It's custom and barely documented. To get to the documentation you need to read the source of the interpreter where you need to transform it mentally to what it translates to.

On Mon, Sep 30, 2019 at 05:21:51PM +0200, Peter Krempa wrote:
On Mon, Sep 30, 2019 at 15:48:29 +0100, Daniel Berrange wrote:
On Mon, Sep 30, 2019 at 10:47:39AM +0200, Martin Kletzander wrote:
On Fri, Sep 27, 2019 at 01:52:19PM +0100, Daniel P. Berrangé wrote:
This is a followup to a previous PoC patch I submitted a month ago:
[...]
The choice of YAML is a valid point to use as that is definitely adding a new language where one already exists (XML) that is expressive enough to cope with the problem. As you saw the original v1 did indeed use XML for this reason.
The decision to try YAML was an experiment to see if the simplicity and readability of YAML would outweigh the cost of having another data language. Personally I think it is a net win to use YAML instead of XML, but it would not be the end of the world to go back to the v1 approach using XML if people really want a less readable language just to not have to add another data language.
In my opinion this is a misunderstanding.
Whether it's YAML or XML used for syntax is not that important. What is important is that the new 'language' is the custom declarative language which uses YAML or XML to express the constructions.
It's custom and barely documented. To get to the documentation you need to read the source of the interpreter where you need to transform it mentally to what it translates to.
Yes, that is the more concerning one, thanks for formulating my thoughts better. I really like YAML and I would definitely go for that if choosing a format for similar purpose as yours. It's like ansible. YAML is a good fit there (debatably), but even if you know python, YAML and jinja templating, you still need to learn ansible to actually be able to write any YAML for it.

Daniel P. Berrangé <berrange@redhat.com> [2019-09-30, 03:48PM +0100]:
In this case, our existing C language for writing our production code is not well suited for the task we're applying it too. You could make an argument that the virt-host-validate could be written in Python, but I think it is preferred to keep all our deployable code written in compiled languages, leaving the scripting languages for our supporting build system.
Any hard arguments on why that's preferential? I don't see a reasoning as to not ship python code as many other project do just fine. -- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On Tue, Oct 01, 2019 at 01:36:07PM +0200, Bjoern Walk wrote:
Daniel P. Berrangé <berrange@redhat.com> [2019-09-30, 03:48PM +0100]:
In this case, our existing C language for writing our production code is not well suited for the task we're applying it too. You could make an argument that the virt-host-validate could be written in Python, but I think it is preferred to keep all our deployable code written in compiled languages, leaving the scripting languages for our supporting build system.
Any hard arguments on why that's preferential? I don't see a reasoning as to not ship python code as many other project do just fine.
The various core system tools which involve python have a history of poor performance. For example, we previously used "firewall-cmd" for talking to firewalld and it caused a massive performance hit, because of all the time spent during startup processing imported modules. This is seen again in OpenStack with its command line tools being so terribly slow - it takes 1.5 seconds just to load the client & do nothing. There's the classic difference of dynamic vs compiled languages where with the former many basic syntax errors don't appear until you hit them at runtime unless. Only mitigated if you invest a tonne of time in unit testing every possible codepath. Using compiled languages also enables us to share code between the tools and library. This would enable us to actually embed the host validation functionality into the virt driver, so its exposed as a normal API in libvirt. 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 :|

On Tue, Oct 01, 2019 at 12:53:50PM +0100, Daniel P. Berrangé wrote:
On Tue, Oct 01, 2019 at 01:36:07PM +0200, Bjoern Walk wrote:
Daniel P. Berrangé <berrange@redhat.com> [2019-09-30, 03:48PM +0100]:
In this case, our existing C language for writing our production code is not well suited for the task we're applying it too. You could make an argument that the virt-host-validate could be written in Python, but I think it is preferred to keep all our deployable code written in compiled languages, leaving the scripting languages for our supporting build system.
Any hard arguments on why that's preferential? I don't see a reasoning as to not ship python code as many other project do just fine.
The various core system tools which involve python have a history of poor performance. For example, we previously used "firewall-cmd" for talking to firewalld and it caused a massive performance hit, because of all the time spent during startup processing imported modules. This is seen again in OpenStack with its command line tools being so terribly slow - it takes 1.5 seconds just to load the client & do nothing.
There's the classic difference of dynamic vs compiled languages where with the former many basic syntax errors don't appear until you hit them at runtime unless. Only mitigated if you invest a tonne of time in unit testing every possible codepath.
Both are good points, but they are greyscale rather than black/white. It depends on the size and complexity of the script. It also contradicts the use of declarative language when you are offloading lot of the logic into the input file. I also think that both points are very much relevant to the build system as well, just not that visible because not all parts of the build system are used/modified by everyone.
Using compiled languages also enables us to share code between the tools and library. This would enable us to actually embed the host validation functionality into the virt driver, so its exposed as a normal API in libvirt.
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 :|

Martin Kletzander <mkletzan@redhat.com> [2019-09-30, 10:47AM +0200]:
On Fri, Sep 27, 2019 at 01:52:19PM +0100, Daniel P. Berrangé wrote:
32 files changed, 2609 insertions(+), 1045 deletions(-)
So this ^^ plus:
2 languages added, 0 languages removed
makes me feel like this goes against what you were trying to do in another series. I understand that adding new fact checks is "easier" and does not require recompilation, but I don't see any use for that benefit. I went through the cover letters for both series just to find a reason for it and I didn't.
Sorry, but I don't really like this.
I totally agree. Not only does this series fail to reduce the number of languages of LOCs, it actually increases the complexity and domain-specific knowledge to understand and contribute to libvirt.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On Fri, 2019-09-27 at 13:52 +0100, Daniel P. Berrangé wrote:
The really big difference though is that I replaced the use of XML data files with YAML data files. This was done with the aim of making the data more human friendly. XML is really optimized for machines, not humans, so writing the data files was not pretty. YAML is optimized for human readability, and is actually even easier to consume in Go than the XML was, so its a double win.
I'll add my own 0.2 $currency to what others have said. I think Go is a fine language to use for this kind of tool, so I'm in favor of that; having a more granular view into the details of the system also looks like a good idea. What I'm not sold on is the advantage of a YAML-driven approach: it seems to me that the same result could be achieved much more conveniently using regular Go code instead. Perhaps it would be useful if you explained in detail why you decided to take this approach in the first place. -- Andrea Bolognani / Red Hat / Virtualization

On Mon, Sep 30, 2019 at 12:03:22PM +0200, Andrea Bolognani wrote:
On Fri, 2019-09-27 at 13:52 +0100, Daniel P. Berrangé wrote:
The really big difference though is that I replaced the use of XML data files with YAML data files. This was done with the aim of making the data more human friendly. XML is really optimized for machines, not humans, so writing the data files was not pretty. YAML is optimized for human readability, and is actually even easier to consume in Go than the XML was, so its a double win.
I'll add my own 0.2 $currency to what others have said.
I think Go is a fine language to use for this kind of tool, so I'm in favor of that; having a more granular view into the details of the system also looks like a good idea.
What I'm not sold on is the advantage of a YAML-driven approach: it seems to me that the same result could be achieved much more conveniently using regular Go code instead.
Perhaps it would be useful if you explained in detail why you decided to take this approach in the first place.
I've essentially answered this in my response to Martin's comment in https://www.redhat.com/archives/libvir-list/2019-September/msg01419.html so to avoid splitting the discussion I won't repeat it here. 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 :|
participants (5)
-
Andrea Bolognani
-
Bjoern Walk
-
Daniel P. Berrangé
-
Martin Kletzander
-
Peter Krempa