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

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(a)redhat.com&gt; > 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(a)redhat.com&gt; > > 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.