September 2019 - Devel - libvirt List Archives

[libvirt] [PATCH v5 00/15] CPU Model Baseline and Comparison for s390x
by Collin Walling 07 Oct '19

07 Oct '19

Note: since I've made some changes to a lot of these patches / split up some patches, I've decided to hold off on adding any r-b's in case there is a specific change that someone does not agree with. Changelog: - Properly refactored code from CPU model expansion function - Introduced a cleanup patch for CPU model expansion function - Introduced patches that modifies the refactored code to suit needs for baseline/comparison - CPU expansion function now accepts a virCPUDefPtr - Removed props parsing from CPU model comparison (they weren't used) - Cleaner error reporting when baselining/comparing with erroneous CPU models / features - Various cleanups based on feedback ___ To run these patches, execute the virsh hypervisor-cpu-compare or hypervisor-cpu-baseline commands and pass an XML file describing one or more CPU definition. You can use the definition from virsh domcapabilities or from a guest XML. There is no need extract it from the file and place it a new one, as the XML parser will look specifically for the CPU tags. ___ These patches hookup the virsh hypervisor-cpu-compare/baseline commands for the s390x architecture. They take an XML file describing some CPU definitions and passes the data to QEMU, where the actual CPU model comparison / baseline calculation is handled (available since QEMU 2.8.5). When baselining CPU models with the --features argument, s390x will report a full CPU model expansion. Thanks. Collin Walling (15): qemu_monitor: refactor cpu model expansion qemu_monitor: expansion cleanups qemu_monitor: use cpu def instead of char for expansion qemu_monitor: add features to CPU model for QMP command qemu_monitor: allow cpu props to be optional qemu_monitor: make qemuMonitorJSONParseCPUModelData command-agnostic qemu_monitor: implement query-cpu-model-baseline qemu_capabilities: introduce QEMU_CAPS_QUERY_CPU_MODEL_BASELINE qemu_driver: hook up query-cpu-model-baseline qemu_driver: expand cpu features after baseline qemu_monitor: implement query-cpu-model-comparison qemu_capabilities: introduce QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON cpu_conf: xml to cpu definition parse helper qemu_driver: hook up query-cpu-model-comparison qemu_driver: improve comparison/baseline error reporting src/conf/cpu_conf.c | 29 +++ src/conf/cpu_conf.h | 5 + src/cpu/cpu.c | 14 +- src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 21 +- src/qemu/qemu_capabilities.h | 4 + src/qemu/qemu_driver.c | 215 +++++++++++++++++ src/qemu/qemu_monitor.c | 39 +++- src/qemu/qemu_monitor.h | 13 +- src/qemu/qemu_monitor_json.c | 279 +++++++++++++++++------ src/qemu/qemu_monitor_json.h | 17 +- tests/cputest.c | 11 +- tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 + 19 files changed, 573 insertions(+), 89 deletions(-) -- 2.7.4

3 33

[libvirt] [PATCH 00/11] Integrate usage of glib into libvirt
by Daniel P. Berrangé 04 Oct '19

04 Oct '19

This is a followup to a previous patch series: https://www.redhat.com/archives/libvir-list/2019-August/msg01374.html The first abort-on-oom parts of that series merged already. As well as fixing the issues mentioned last time, the glib parts now do a little more: - Demonstrate conversion of virObject to GObject - Convert to gbase64 APIs - Start to convert getopt to GOptionContext I have explicitly now confirmed we can freely mix g_malloc/malloc and g_free/free, given our min glib version. my intention with the glib code will be to focus on converting bits of code that allow us to eliminate gnulib modules. About 50% of the gnulib stuff is related to the Windows portability for sockets() and poll(). We can address this by integrate the event loop with GMainLoop, and using GIO for its GSocket APIs. The other gnulib stuff is a random bag of APIs. Some may be replaced by glib APIs, for others we can pull the gnulib fix straight into libvirt, for others the portability problems might be obsolete. Daniel P. Berrangé (11): build: probe for glib-2 library in configure build: link to glib, gobject, gio libraries remote: don't pull anonymous enums into rpc protocol structs util: use glib memory allocation functions util: use glib string allocation/formatting functions util: use glib base64 encoding/decoding APIs util: convert virIdentity class to use GObject src: convert over to use GRegex for regular exprssions virsh: convert command line parsing to use GOptionContext virt-admin: convert command line parsing to use GOptionContext virt-login-shell: convert command line parsing to use GOptionContext bootstrap.conf | 5 - configure.ac | 2 + libvirt.spec.in | 1 + m4/virt-glib.m4 | 36 ++++ mingw-libvirt.spec.in | 2 + src/Makefile.am | 5 +- src/access/viraccessdriverpolkit.c | 21 +- src/admin/admin_server.c | 3 +- src/admin_protocol-structs | 9 - src/conf/domain_event.c | 25 +-- src/conf/virsecretobj.c | 38 +--- src/internal.h | 1 + src/libvirt_private.syms | 1 - src/libxl/libxl_capabilities.c | 44 ++-- src/libxl/libxl_conf.c | 3 +- src/lxc/Makefile.inc.am | 2 + src/qemu/qemu_agent.c | 9 +- src/qemu/qemu_command.c | 5 +- src/qemu/qemu_domain.c | 8 +- src/qemu/qemu_process.c | 4 +- src/remote/Makefile.inc.am | 1 + src/remote/remote_daemon.c | 3 +- src/remote/remote_daemon_dispatch.c | 35 ++-- src/remote_protocol-structs | 9 - src/rpc/virnetserverclient.c | 57 +++--- src/rpc/virnetserverprogram.c | 13 +- src/secret/secret_driver.c | 1 - src/storage/storage_backend_rbd.c | 4 +- src/util/Makefile.inc.am | 1 + src/util/viralloc.c | 29 +-- src/util/viridentity.c | 87 ++++---- src/util/viridentity.h | 7 +- src/util/virstring.c | 40 +--- src/util/virstring.h | 2 - tests/Makefile.am | 3 +- tests/viridentitytest.c | 45 ++--- tests/virnetserverclienttest.c | 3 +- tools/Makefile.am | 1 + tools/virsh-secret.c | 17 +- tools/virsh.c | 303 +++++++++++++--------------- tools/virt-admin.c | 207 +++++++++---------- tools/virt-login-shell-helper.c | 66 ++---- 42 files changed, 495 insertions(+), 663 deletions(-) create mode 100644 m4/virt-glib.m4 -- 2.21.0

6 56

[libvirt] [RFC] cpu_map: Remove pconfig from Icelake-Server CPU model
by Eduardo Habkost 03 Oct '19

03 Oct '19

The pconfig feature never worked, and adding "pconfig=off" to the QEMU command-line triggers a regression in QEMU 3.1.1 and 4.0.0. Signed-off-by: Eduardo Habkost <ehabkost(a)redhat.com> --- I'm sending this as an RFC because I couldn't test it properly, and because I don't know what are the consequences of changing cpu_map between libvirt versions. --- src/cpu_map/x86_Icelake-Server.xml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cpu_map/x86_Icelake-Server.xml b/src/cpu_map/x86_Icelake-Server.xml index fb15977a59..188a781282 100644 --- a/src/cpu_map/x86_Icelake-Server.xml +++ b/src/cpu_map/x86_Icelake-Server.xml @@ -56,7 +56,9 @@ <feature name='pat'/> <feature name='pcid'/> <feature name='pclmuldq'/> - <feature name='pconfig'/> +  <feature name='pdpe1gb'/> <feature name='pge'/> <feature name='pku'/> -- 2.21.0

5 8

[libvirt] [PATCH v4 00/12] remove repetition of URI path validation
by Daniel Henrique Barboza 02 Oct '19

02 Oct '19

This is a code repetition that I crossed a few times, then I noticed that Cole Robinson suggested a solution for it in the wiki. Here it is. changes from v3: - patch 8: fix the exception logic, move the code formatting to patch 1 - patch 9: use lowcase 'qemu' - patch 12: (optional) test case I created to aid in patch 8 logic changes from v2: - use a boolean to determine 'QEMU' and 'vbox' case to avoid block repetition (patch 8) - avoid 80+ chars lines in all patches changes from v1: - handle QEMU and vbox cases separately inside the validation function v3: https://www.redhat.com/archives/libvir-list/2019-September/msg01122.html v2: https://www.redhat.com/archives/libvir-list/2019-September/msg01007.html v1: https://www.redhat.com/archives/libvir-list/2019-September/msg00983.html Daniel Henrique Barboza (12): src/driver.c: add virConnectValidateURIPath() interface_backend_netcf.c: use virConnectValidateURIPath() interface_backend_udev.c: use virConnectValidateURIPath() bridge_driver.c: virConnectValidateURIPath() node_device_driver.c: use virConnectValidateURIPath() secret_driver.c: use virConnectValidateURIPath() storage_driver.c: use virConnectValidateURIPath() driver.c: change URI validation to handle QEMU and vbox case qemu_driver.c: use virConnectValidateURIPath() vbox_common.c: use virConnectValidateURIPath() vbox_driver.c: use virConnectValidateURIPath() tests: add a test for driver.c:virConnectValidateURIPath() src/driver.c | 38 +++++ src/driver.h | 4 + src/interface/interface_backend_netcf.c | 19 +-- src/interface/interface_backend_udev.c | 19 +-- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 19 +-- src/node_device/node_device_driver.c | 19 +-- src/qemu/qemu_driver.c | 20 +-- src/secret/secret_driver.c | 19 +-- src/storage/storage_driver.c | 19 +-- src/vbox/vbox_common.c | 16 +- src/vbox/vbox_driver.c | 16 +- tests/Makefile.am | 7 +- tests/virdriverconnvalidatetest.c | 186 ++++++++++++++++++++++++ 14 files changed, 267 insertions(+), 135 deletions(-) create mode 100644 tests/virdriverconnvalidatetest.c -- 2.21.0

2 16

[libvirt] [PATCH 1/1] src/driver.c: remove duplicated code in virGetConnect* functions
by Daniel Henrique Barboza 02 Oct '19

02 Oct '19

All the 6 virGetConnect* functions in driver.c shares the same code base. This patch creates a new static function virGetConnectGeneric() that contains the common code to be used with all other virGetConnect*. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com> --- CC'ing Cole Robinson since he reviewed similar patches a few days ago. src/driver.c | 100 +++++++++++++-------------------------------------- 1 file changed, 25 insertions(+), 75 deletions(-) diff --git a/src/driver.c b/src/driver.c index ed2d943ddf..8a4bc8ff66 100644 --- a/src/driver.c +++ b/src/driver.c @@ -29,6 +29,7 @@ #include "virfile.h" #include "virlog.h" #include "virmodule.h" +#include "virstring.h" #include "virthread.h" #include "configmake.h" @@ -96,112 +97,61 @@ virConnectCacheOnceInit(void) VIR_ONCE_GLOBAL_INIT(virConnectCache); -virConnectPtr virGetConnectInterface(void) +static virConnectPtr +virGetConnectGeneric(virThreadLocal thread, const char *name) { virConnectPtr conn; if (virConnectCacheInitialize() < 0) return NULL; - conn = virThreadLocalGet(&connectInterface); + conn = virThreadLocalGet(&thread); + if (conn) { - VIR_DEBUG("Return cached interface connection %p", conn); + VIR_DEBUG("Return cached %s connection %p", name, conn); virObjectRef(conn); } else { - conn = virConnectOpen(geteuid() == 0 ? "interface:///system" : "interface:///session"); - VIR_DEBUG("Opened new interface connection %p", conn); + VIR_AUTOFREE(char *) uri = NULL; + const char *uriPath = geteuid() == 0 ? "/system" : "/session"; + + if (virAsprintf(&uri, "%s//%s", name, uriPath) < 0) + return NULL; + + conn = virConnectOpen(uri); + VIR_DEBUG("Opened new %s connection %p", name, conn); } return conn; } -virConnectPtr virGetConnectNetwork(void) -{ - virConnectPtr conn; - if (virConnectCacheInitialize() < 0) - return NULL; +virConnectPtr virGetConnectInterface(void) +{ + return virGetConnectGeneric(connectInterface, "interface"); +} - conn = virThreadLocalGet(&connectNetwork); - if (conn) { - VIR_DEBUG("Return cached network connection %p", conn); - virObjectRef(conn); - } else { - conn = virConnectOpen(geteuid() == 0 ? "network:///system" : "network:///session"); - VIR_DEBUG("Opened new network connection %p", conn); - } - return conn; +virConnectPtr virGetConnectNetwork(void) +{ + return virGetConnectGeneric(connectNetwork, "network"); } virConnectPtr virGetConnectNWFilter(void) { - virConnectPtr conn; - - if (virConnectCacheInitialize() < 0) - return NULL; - - conn = virThreadLocalGet(&connectNWFilter); - if (conn) { - VIR_DEBUG("Return cached nwfilter connection %p", conn); - virObjectRef(conn); - } else { - conn = virConnectOpen(geteuid() == 0 ? "nwfilter:///system" : "nwfilter:///session"); - VIR_DEBUG("Opened new nwfilter connection %p", conn); - } - return conn; + return virGetConnectGeneric(connectNWFilter, "nwfilter"); } virConnectPtr virGetConnectNodeDev(void) { - virConnectPtr conn; - - if (virConnectCacheInitialize() < 0) - return NULL; - - conn = virThreadLocalGet(&connectNodeDev); - if (conn) { - VIR_DEBUG("Return cached nodedev connection %p", conn); - virObjectRef(conn); - } else { - conn = virConnectOpen(geteuid() == 0 ? "nodedev:///system" : "nodedev:///session"); - VIR_DEBUG("Opened new nodedev connection %p", conn); - } - return conn; + return virGetConnectGeneric(connectNodeDev, "nodedev"); } virConnectPtr virGetConnectSecret(void) { - virConnectPtr conn; - - if (virConnectCacheInitialize() < 0) - return NULL; - - conn = virThreadLocalGet(&connectSecret); - if (conn) { - VIR_DEBUG("Return cached secret connection %p", conn); - virObjectRef(conn); - } else { - conn = virConnectOpen(geteuid() == 0 ? "secret:///system" : "secret:///session"); - VIR_DEBUG("Opened new secret connection %p", conn); - } - return conn; + return virGetConnectGeneric(connectSecret, "secret"); } virConnectPtr virGetConnectStorage(void) { - virConnectPtr conn; - - if (virConnectCacheInitialize() < 0) - return NULL; - - conn = virThreadLocalGet(&connectStorage); - if (conn) { - VIR_DEBUG("Return cached storage connection %p", conn); - virObjectRef(conn); - } else { - conn = virConnectOpen(geteuid() == 0 ? "storage:///system" : "storage:///session"); - VIR_DEBUG("Opened new storage connection %p", conn); - } - return conn; + return virGetConnectGeneric(connectStorage, "storage"); } -- 2.21.0

2 1

[libvirt] [PATCH v2 0/2] make attaching disk partition to VM illegal
by Pavel Mores 02 Oct '19

02 Oct '19

This version addresses comments and objections to the original submission. Pavel Mores (2): qemu: make attaching disk partition to VM illegal remove a now redundant call to virDiskNameToIndex() src/qemu/qemu_command.c | 6 ----- src/qemu/qemu_domain.c | 15 +++++++++++ .../disk-attaching-partition-nosupport.xml | 27 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 43 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-attaching-partition-nosupport.xml -- 2.21.0

3 8

[libvirt] [PATCH v2 0/6] rewrite virt-host-validate to be data driven, using Go & YAML
by Daniel P. Berrangé 01 Oct '19

01 Oct '19

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> 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> 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

5 25

[libvirt] [PATCH] docs: document that C & Python are the preferred languages
by Daniel P. Berrangé 01 Oct '19

01 Oct '19

Blacklist Perl and Shell code in favour of Python for sake of readability and portability. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- docs/hacking.html.in | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 90376968c3..8572eb65a6 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -343,6 +343,36 @@ Richard Jones' guide to working with open source projects</a>. </p> + <h2><a id="lang">Language Usage</a></h2> + + <p> + The libvirt repository makes use of a large number of programming + languages. There is a general desire to phase out some of the + existing languages used to reduce the knowledge burden on + developers, and facilitate introduction of new languages in + the future. + </p> + + <p> + The preferred languages at this time are + </p> + + <ul> + <li>C - for the main libvirt codebase. Dialect supported by + GCC/CLang only.</li> + <li>Python - for supporting build scripts / tools. Code must + run with both version 2.7 and 3.x at this time.</li> + </ul> + + <p> + Languages that should not be used for any new contributions. + </p> + + <ul> + <li>Perl - build scripts must be written in Python instead.</li> + <li>Shell - build scripts must be written in Python instead.</li> + </ul> + <h2><a id="tooling">Tooling</a></h2> <p> -- 2.21.0

5 11

[libvirt] [PATCH 0/2] qemu_process: use VIR_AUTO* all around
by Daniel Henrique Barboza 30 Sep '19

30 Sep '19

The usual AUTOFREE() and AUTOUNREF() changes that allows for a bit of cleanup. Daniel Henrique Barboza (2): qemu_process: use VIR_AUTOFREE() qemu_process: use VIR_AUTOUNREF() src/qemu/qemu_process.c | 413 +++++++++++++++------------------------- 1 file changed, 149 insertions(+), 264 deletions(-) -- 2.21.0

1 2

[libvirt] [PATCH] rpc: fix escaping of shell path for netcat binary
by Daniel P. Berrangé 30 Sep '19

30 Sep '19

Consider having a nc binary in the path with a space in its name, for example '/tmp/fo o/nc' This results in libvirt running SSH with the following arg value "'if ''/tmp/fo o/nc'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0; else ARG=;fi;''/tmp/fo o/nc'' $ARG -U /var/run/libvirt/libvirt-sock'" The use of the single quote escaping was introduced by commit 6ac6238de33fc74e7545b245ae273d1bfd658808 Author: Guido Günther <agx(a)sigxcpu.org> Date: Thu Oct 13 21:49:01 2011 +0200 Use virBufferEscapeShell in virNetSocketNewConnectSSH to escape the netcat command since it's passed to the shell. Adjust expected test case output accordingly. While the intention of this change was good, the result is broken as it is still underquoted. On the SSH server side, SSH itself runs the command via the shell. Our command is then invoking the shell again. Thus we see $ virsh -c qemu+ssh://root@domokun/system?netcat=%2Ftmp%2Ffo%20o%2Fnc list error: failed to connect to the hypervisor error: End of file while reading data: sh: /tmp/fo: No such file or directory: Input/output error With the second level of escaping added we can now successfully use a nc binary with a space in the path. The original test case added was misleading as it illustrated using a binary path of 'nc -4' which is not a path, it is a command with a separate argument, which is getting interpreted as a path. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- src/rpc/virnetclient.c | 8 ++++++++ src/rpc/virnetsocket.c | 9 +++++++++ tests/virnetsockettest.c | 6 +++--- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 64855fb8d6..53d8b219ea 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -490,6 +490,10 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, DEFAULT_VALUE(knownHostsVerify, "normal"); virBufferEscapeShell(&buf, netcatPath); + if (!(nc = virBufferContentAndReset(&buf))) + goto no_memory; + virBufferEscapeShell(&buf, nc); + VIR_FREE(nc); if (!(nc = virBufferContentAndReset(&buf))) goto no_memory; @@ -596,6 +600,10 @@ virNetClientPtr virNetClientNewLibssh(const char *host, DEFAULT_VALUE(knownHostsVerify, "normal"); virBufferEscapeShell(&buf, netcatPath); + if (!(nc = virBufferContentAndReset(&buf))) + goto no_memory; + virBufferEscapeShell(&buf, nc); + VIR_FREE(nc); if (!(nc = virBufferContentAndReset(&buf))) goto no_memory; diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index ebd304707a..a469907779 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -903,6 +903,15 @@ int virNetSocketNewConnectSSH(const char *nodename, return -1; } quoted = virBufferContentAndReset(&buf); + + virBufferEscapeShell(&buf, quoted); + VIR_FREE(quoted); + if (virBufferCheckError(&buf) < 0) { + virCommandFree(cmd); + return -1; + } + quoted = virBufferContentAndReset(&buf); + /* * This ugly thing is a shell script to detect availability of * the -q option for 'nc': debian and suse based distros need this diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index bb8357f7cd..8cad351605 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -661,15 +661,15 @@ mymain(void) struct testSSHData sshData7 = { .nodename = "somehost", - .netcat = "nc -4", + .netcat = "/tmp/fo o/nc", .path = "/tmp/socket", .expectOut = "-T -e none -- somehost sh -c '" - "if ''nc -4'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if \'''\\''/tmp/fo o/nc'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0;" "else " "ARG=;" "fi;" - "''nc -4'' $ARG -U /tmp/socket'\n", + "'''\\''/tmp/fo o/nc'\\'''' $ARG -U /tmp/socket'\n", }; if (virTestRun("SSH test 7", testSocketSSH, &sshData7) < 0) ret = -1; -- 2.21.0

2 1