[PATCH 00/21] Fix compatibility with netdev-add in qemu-5.0
This is based on top of tests: qemu: Detect deprecation in the QMP schema (deprecation part 1) https://www.redhat.com/archives/libvir-list/2020-April/msg01444.html Both can be fetched at: git fetch https://gitlab.com/pipo.sk/libvirt.git netdev_add-validate QEMU-5.0 released a fully QAPIfied 'netdev_add' command. Since we didn't format the arguments in a way qemu now thinks we should do them network device hotplug broke. Fix it by modifying our internals slightly and add some testing. This series also adds schema testing of -blockdev arguments and prepares for a simple addition of schema testing for arguments of -device and -object if qemu ever finishes the qapification of those. Theoretically we can even do it pre-emtptively but it's almost guaranteed to fail at least for -device. I just hope that qemu let's us know in advance as -device will be harder. Note that if the loading of the schema in the tests I've added will be deemed to be slow I'll add caching. Peter Krempa (21): qemu: domain: Forbid unsupported 'tftp' protocol and handle tests qemuMonitorJSONParseKeywords: remove constant argument qemuBuildChannelsCommandLine: Use typecasted switch for channel type qemuBuildChannelsCommandLine: Extract common formatting of 'chardev' qemuBuildChannelChrDeviceStr: Remove formatting of properties for -netdev qemuBuildHostNetStr: Stop using 'ipv6-net' convenience argument virQEMUBuildCommandLineJSON: Allow skipping certain keys virQEMUBuildCommandLineJSON: Add possibility for using 'on/off' instead of 'yes/no' virCommand: Introduce virCommandGetArgList util: json: Introduce virJSONValueObjectAppendStringPrintf testCompareXMLToArgv: Split out preparation and command formatting qemuMonitorJSON(Add|Remove)Netdev: Refactor cleanup util: virqemu: Introduce virQEMUBuildNetdevCommandlineFromJSON qemu: command: Generate -netdev command line via JSON->cmdline conversion virQEMUBuildNetdevCommandlineFromJSON: Prepare for quirky 'guestfwd' qemuBuildChannelGuestfwdNetdevProps: Convert to generating JSON props qemuMonitorAddNetdev: Convert to the native JSON props object qemu: Prepare for testing of 'netdev_add' props via qemuxml2argvtest testQEMUSchemaLoad: Rename to testQEMUSchemaLoadLatest testutilsqemuschema: Allow loading non-latest schema qemuxml2argvtest: Add QAPI/QMP schema validation for -blockdev and -netdev src/libvirt_private.syms | 3 + src/qemu/qemu_command.c | 289 +++++++++++------- src/qemu/qemu_command.h | 22 +- src/qemu/qemu_domain.c | 10 + src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 31 +- src/qemu/qemu_monitor.c | 8 +- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 76 ++--- src/qemu/qemu_monitor_json.h | 2 +- src/qemu/qemu_process.c | 11 +- src/qemu/qemu_process.h | 1 + src/util/vircommand.c | 23 ++ src/util/vircommand.h | 1 + src/util/virjson.c | 17 ++ src/util/virjson.h | 2 + src/util/virqemu.c | 119 +++++++- src/util/virqemu.h | 18 +- tests/Makefile.am | 2 +- tests/qemublocktest.c | 2 +- tests/qemucommandutiltest.c | 2 +- tests/qemuhotplugtest.c | 2 +- tests/qemumonitorjsontest.c | 6 +- .../qemuxml2argvdata/disk-cdrom-network.args | 3 - .../disk-cdrom-network.x86_64-2.12.0.args | 3 - .../disk-cdrom-network.x86_64-latest.args | 17 +- tests/qemuxml2argvdata/disk-cdrom-network.xml | 9 - tests/qemuxml2argvdata/net-user-addr.args | 3 +- tests/qemuxml2argvtest.c | 236 +++++++++----- tests/testutilsqemu.c | 5 + tests/testutilsqemu.h | 1 + tests/testutilsqemuschema.c | 66 ++-- tests/testutilsqemuschema.h | 5 +- 33 files changed, 667 insertions(+), 332 deletions(-) -- 2.26.2
'tftp' storage protocol was supported by qemu until 2.7.0. Add an interlock when blockdev is used and drop the test case for it as it's IMO not worth adding another test file just for that. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 10 ++++++++++ tests/qemuxml2argvdata/disk-cdrom-network.args | 3 --- .../disk-cdrom-network.x86_64-2.12.0.args | 3 --- .../disk-cdrom-network.x86_64-latest.args | 17 ++++++----------- tests/qemuxml2argvdata/disk-cdrom-network.xml | 9 --------- 5 files changed, 16 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d0528dbfe0..d5e3d1a3cc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5319,6 +5319,16 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src, return -1; } + /* TFTP protocol was not supported for some time, lock it out at least with + * -blockdev */ + if (actualType == VIR_STORAGE_TYPE_NETWORK && + src->protocol == VIR_STORAGE_NET_PROTOCOL_TFTP && + blockdev) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'tftp' protocol is not supported with this QEMU binary")); + return -1; + } + return 0; } diff --git a/tests/qemuxml2argvdata/disk-cdrom-network.args b/tests/qemuxml2argvdata/disk-cdrom-network.args index 81ff324a0f..794fdecdcb 100644 --- a/tests/qemuxml2argvdata/disk-cdrom-network.args +++ b/tests/qemuxml2argvdata/disk-cdrom-network.args @@ -33,7 +33,4 @@ id=drive-ide0-0-1,readonly=on \ -drive 'file=https://host.name:443/url/path/file.iso?test=val,format=raw,\ if=none,id=drive-ide0-1-0,readonly=on' \ -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ --drive file=tftp://host.name:69/url/path/file.iso,format=raw,if=none,\ -id=drive-ide0-1-1,readonly=on \ --device ide-cd,bus=ide.1,unit=1,drive=drive-ide0-1-1,id=ide0-1-1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-2.12.0.args b/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-2.12.0.args index 81f6b400aa..fa5c0ba087 100644 --- a/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-2.12.0.args +++ b/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-2.12.0.args @@ -35,9 +35,6 @@ id=drive-ide0-0-1,readonly=on \ -drive 'file=https://host.name:443/url/path/file.iso?test=val,format=raw,\ if=none,id=drive-ide0-1-0,readonly=on' \ -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ --drive file=tftp://host.name:69/url/path/file.iso,format=raw,if=none,\ -id=drive-ide0-1-1,readonly=on \ --device ide-cd,bus=ide.1,unit=1,drive=drive-ide0-1-1,id=ide0-1-1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ diff --git a/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-latest.args b/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-latest.args index 2515b256d0..1a102949df 100644 --- a/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-latest.args +++ b/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-latest.args @@ -28,26 +28,21 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -boot strict=on \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -blockdev '{"driver":"ftp","url":"ftp://host.name:21/url/path/file.iso",\ -"node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}' \ --blockdev '{"node-name":"libvirt-4-format","read-only":true,"driver":"raw",\ -"file":"libvirt-4-storage"}' \ --device ide-cd,bus=ide.0,unit=0,drive=libvirt-4-format,id=ide0-0-0,bootindex=1 \ --blockdev '{"driver":"ftps","url":"ftps://host.name:990/url/path/file.iso",\ "node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-3-format","read-only":true,"driver":"raw",\ "file":"libvirt-3-storage"}' \ --device ide-cd,bus=ide.0,unit=1,drive=libvirt-3-format,id=ide0-0-1 \ --blockdev '{"driver":"https",\ -"url":"https://host.name:443/url/path/file.iso?test=val",\ +-device ide-cd,bus=ide.0,unit=0,drive=libvirt-3-format,id=ide0-0-0,bootindex=1 \ +-blockdev '{"driver":"ftps","url":"ftps://host.name:990/url/path/file.iso",\ "node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-2-format","read-only":true,"driver":"raw",\ "file":"libvirt-2-storage"}' \ --device ide-cd,bus=ide.1,unit=0,drive=libvirt-2-format,id=ide0-1-0 \ --blockdev '{"driver":"tftp","url":"tftp://host.name:69/url/path/file.iso",\ +-device ide-cd,bus=ide.0,unit=1,drive=libvirt-2-format,id=ide0-0-1 \ +-blockdev '{"driver":"https",\ +"url":"https://host.name:443/url/path/file.iso?test=val",\ "node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-1-format","read-only":true,"driver":"raw",\ "file":"libvirt-1-storage"}' \ --device ide-cd,bus=ide.1,unit=1,drive=libvirt-1-format,id=ide0-1-1 \ +-device ide-cd,bus=ide.1,unit=0,drive=libvirt-1-format,id=ide0-1-0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ diff --git a/tests/qemuxml2argvdata/disk-cdrom-network.xml b/tests/qemuxml2argvdata/disk-cdrom-network.xml index 44473f8ad4..18a56f7156 100644 --- a/tests/qemuxml2argvdata/disk-cdrom-network.xml +++ b/tests/qemuxml2argvdata/disk-cdrom-network.xml @@ -46,15 +46,6 @@ <readonly/> <address type='drive' controller='0' bus='1' target='0' unit='0'/> </disk> - <disk type='network' device='cdrom'> - <driver name='qemu' type='raw'/> - <source protocol='tftp' name='/url/path/file.iso'> - <host name='host.name' port='69'/> - </source> - <target dev='hdd' bus='ide'/> - <readonly/> - <address type='drive' controller='0' bus='1' target='0' unit='1'/> - </disk> <controller type='usb' index='0'/> <controller type='pci' index='0' model='pci-root'/> <controller type='ide' index='0'/> -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
'tftp' storage protocol was supported by qemu until 2.7.0. Add an interlock when blockdev is used and drop the test case for it as it's IMO not worth adding another test file just for that.
Fair enough.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 10 ++++++++++ tests/qemuxml2argvdata/disk-cdrom-network.args | 3 --- .../disk-cdrom-network.x86_64-2.12.0.args | 3 --- .../disk-cdrom-network.x86_64-latest.args | 17 ++++++----------- tests/qemuxml2argvdata/disk-cdrom-network.xml | 9 --------- 5 files changed, 16 insertions(+), 26 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
There's just one caller that always passes '1'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 505b31a78a..66422a8489 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -604,16 +604,15 @@ qemuMonitorJSONParseKeywordsFree(int nkeywords, /* * Takes a string containing a set of key=value,key=value,key... * parameters and splits them up, returning two arrays with - * the individual keys and values. If allowEmptyValue is nonzero, - * the "=value" part is optional and if a key with no value is found, + * the individual keys and values. + * The "=value" part is optional and if a key with no value is found, * NULL is be placed into corresponding place in retvalues. */ static int qemuMonitorJSONParseKeywords(const char *str, char ***retkeywords, char ***retvalues, - int *retnkeywords, - int allowEmptyValue) + int *retnkeywords) { int keywordCount = 0; int keywordAlloc = 0; @@ -645,14 +644,8 @@ qemuMonitorJSONParseKeywords(const char *str, if (!(separator = strchr(start, '='))) separator = end; - if (separator >= endmark) { - if (!allowEmptyValue) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("malformed keyword arguments in '%s'"), str); - goto error; - } + if (separator >= endmark) separator = endmark; - } keyword = g_strndup(start, separator - start); @@ -708,7 +701,7 @@ qemuMonitorJSONKeywordStringToJSON(const char *str, const char *firstkeyword) int nkeywords = 0; size_t i; - if (qemuMonitorJSONParseKeywords(str, &keywords, &values, &nkeywords, 1) < 0) + if (qemuMonitorJSONParseKeywords(str, &keywords, &values, &nkeywords) < 0) goto error; for (i = 0; i < nkeywords; i++) { -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
There's just one caller that always passes '1'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 505b31a78a..66422a8489 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -604,16 +604,15 @@ qemuMonitorJSONParseKeywordsFree(int nkeywords, /* * Takes a string containing a set of key=value,key=value,key... * parameters and splits them up, returning two arrays with - * the individual keys and values. If allowEmptyValue is nonzero, - * the "=value" part is optional and if a key with no value is found, + * the individual keys and values. + * The "=value" part is optional and if a key with no value is found, * NULL is be placed into corresponding place in retvalues.
While you are touching this, s/is be placed into/will be placed into the/ With that, Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Cover all cases of the enum. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bfe70ed228..06568ae585 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8523,7 +8523,7 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, virDomainChrDefPtr channel = def->channels[i]; char *devstr; - switch (channel->targetType) { + switch ((virDomainChrChannelTargetType) channel->targetType) { case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: if (!(devstr = qemuBuildChrChardevStr(logManager, secManager, cmd, cfg, def, @@ -8555,6 +8555,11 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, if (qemuBuildChrDeviceCommandLine(cmd, def, channel, qemuCaps) < 0) return -1; break; + + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN: + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE: + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST: + return -1; } } -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
Cover all cases of the enum.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Both active branches create the same backend chardev. Since there is no other case, extract it before the switch so that we don't have to duplicate it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 06568ae585..56b4aae1dd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8521,37 +8521,27 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, for (i = 0; i < def->nchannels; i++) { virDomainChrDefPtr channel = def->channels[i]; - char *devstr; + g_autofree char *chardevstr = NULL; + g_autofree char *netdevstr = NULL; - switch ((virDomainChrChannelTargetType) channel->targetType) { - case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: - if (!(devstr = qemuBuildChrChardevStr(logManager, secManager, + if (!(chardevstr = qemuBuildChrChardevStr(logManager, secManager, cmd, cfg, def, channel->source, channel->info.alias, qemuCaps, cdevflags))) - return -1; - virCommandAddArg(cmd, "-chardev"); - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); + return -1; - if (qemuBuildChrDeviceStr(&devstr, def, channel, qemuCaps) < 0) + virCommandAddArg(cmd, "-chardev"); + virCommandAddArg(cmd, chardevstr); + + switch ((virDomainChrChannelTargetType) channel->targetType) { + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: + if (qemuBuildChrDeviceStr(&netdevstr, def, channel, qemuCaps) < 0) return -1; - virCommandAddArgList(cmd, "-netdev", devstr, NULL); - VIR_FREE(devstr); + virCommandAddArgList(cmd, "-netdev", netdevstr, NULL); break; case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: - if (!(devstr = qemuBuildChrChardevStr(logManager, secManager, - cmd, cfg, def, - channel->source, - channel->info.alias, - qemuCaps, cdevflags))) - return -1; - virCommandAddArg(cmd, "-chardev"); - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); - if (qemuBuildChrDeviceCommandLine(cmd, def, channel, qemuCaps) < 0) return -1; break; -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
Both active branches create the same backend chardev. Since there is no other case, extract it before the switch so that we don't have to duplicate it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
The output of the function is fed as argument to '-device' command line argument or 'device_add' monitor command except for 'guestfwd' channels where it needs to be fed to -netdev/netdev_add. This is confusing and error prone. Split it up since the caller needs to know which command/option to use anyways, so the caller can call the appropriate function without any magic. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 36 ++++++++++++++++++++---------------- src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_hotplug.c | 18 +++++++++++++----- 3 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 56b4aae1dd..c20176f619 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8536,7 +8536,7 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, switch ((virDomainChrChannelTargetType) channel->targetType) { case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: - if (qemuBuildChrDeviceStr(&netdevstr, def, channel, qemuCaps) < 0) + if (!(netdevstr = qemuBuildChannelGuestfwdNetdevProps(channel))) return -1; virCommandAddArgList(cmd, "-netdev", netdevstr, NULL); break; @@ -9804,36 +9804,40 @@ qemuBuildParallelChrDeviceStr(char **deviceStr, return 0; } -static int -qemuBuildChannelChrDeviceStr(char **deviceStr, - const virDomainDef *def, - virDomainChrDefPtr chr) + +char * +qemuBuildChannelGuestfwdNetdevProps(virDomainChrDefPtr chr) { - int ret = -1; g_autofree char *addr = NULL; int port; - switch ((virDomainChrChannelTargetType)chr->targetType) { - case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: + if (!(addr = virSocketAddrFormat(chr->target.addr))) + return NULL; - addr = virSocketAddrFormat(chr->target.addr); - if (!addr) - return ret; - port = virSocketAddrGetPort(chr->target.addr); + port = virSocketAddrGetPort(chr->target.addr); + + return g_strdup_printf("user,guestfwd=tcp:%s:%i-chardev:char%s,id=%s", + addr, port, chr->info.alias, chr->info.alias); +} - *deviceStr = g_strdup_printf("user,guestfwd=tcp:%s:%i-chardev:char%s,id=%s", - addr, port, chr->info.alias, chr->info.alias); - break; +static int +qemuBuildChannelChrDeviceStr(char **deviceStr, + const virDomainDef *def, + virDomainChrDefPtr chr) +{ + switch ((virDomainChrChannelTargetType)chr->targetType) { case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(def, chr))) return -1; break; + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: + /* guestfwd is as a netdev handled separately */ case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN: case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE: case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST: - return ret; + return -1; } return 0; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 53e05777e7..7665b68548 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -86,6 +86,9 @@ qemuBuildChrDeviceStr(char **deviceStr, virDomainChrDefPtr chr, virQEMUCapsPtr qemuCaps); +char * +qemuBuildChannelGuestfwdNetdevProps(virDomainChrDefPtr chr); + char *qemuBuildHostNetStr(virDomainNetDefPtr net, char **tapfd, size_t tapfdSize, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ab5a7aef84..2976ba7647 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2108,6 +2108,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virErrorPtr orig_err; virDomainDefPtr vmdef = vm->def; g_autofree char *devstr = NULL; + g_autofree char *netdevstr = NULL; virDomainChrSourceDefPtr dev = chr->source; g_autofree char *charAlias = NULL; bool chardevAttached = false; @@ -2146,8 +2147,13 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto cleanup; teardowncgroup = true; - if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) - goto cleanup; + if (guestfwd) { + if (!(netdevstr = qemuBuildChannelGuestfwdNetdevProps(chr))) + goto cleanup; + } else { + if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) + goto cleanup; + } if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) goto cleanup; @@ -2166,11 +2172,13 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto exit_monitor; chardevAttached = true; - if (guestfwd) { - if (qemuMonitorAddNetdev(priv->mon, devstr, + if (netdevstr) { + if (qemuMonitorAddNetdev(priv->mon, netdevstr, NULL, NULL, 0, NULL, NULL, 0, -1, NULL) < 0) goto exit_monitor; - } else { + } + + if (devstr) { if (qemuMonitorAddDevice(priv->mon, devstr) < 0) goto exit_monitor; } -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
The output of the function is fed as argument to '-device' command line argument or 'device_add' monitor command except for 'guestfwd' channels where it needs to be fed to -netdev/netdev_add. This is confusing and error prone. Split it up since the caller needs to know which command/option to use anyways, so the caller can call the appropriate function without any magic.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 36 ++++++++++++++++++++---------------- src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_hotplug.c | 18 +++++++++++++----- 3 files changed, 36 insertions(+), 21 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
In qemu the argument of 'ipv6-net' is split up into 'ipv6-prefix' and 'ipv6-prefixlen'. Additionally now that 'netdev_add' was qapified, only the real properties are allowed. Switch to using them explicitly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 20 ++++++++++---------- tests/qemuxml2argvdata/net-user-addr.args | 3 ++- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c20176f619..2850953bd0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3645,20 +3645,20 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, for (i = 0; i < net->guestIP.nips; i++) { const virNetDevIPAddr *ip = net->guestIP.ips[i]; g_autofree char *addr = NULL; - const char *prefix = ""; if (!(addr = virSocketAddrFormat(&ip->address))) return NULL; - if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET)) - prefix = "net="; - if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6)) - prefix = "ipv6-net="; - - virBufferAsprintf(&buf, "%s%s", prefix, addr); - if (ip->prefix) - virBufferAsprintf(&buf, "/%u", ip->prefix); - virBufferAddChar(&buf, ','); + if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET)) { + virBufferAsprintf(&buf, "net=%s", addr); + if (ip->prefix) + virBufferAsprintf(&buf, "/%u", ip->prefix); + virBufferAddChar(&buf, ','); + } else if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6)) { + virBufferAsprintf(&buf, "ipv6-prefix=%s,", addr); + if (ip->prefix) + virBufferAsprintf(&buf, "ipv6-prefixlen=%u,", ip->prefix); + } } } break; diff --git a/tests/qemuxml2argvdata/net-user-addr.args b/tests/qemuxml2argvdata/net-user-addr.args index 6cc82d9e62..5f1de305e0 100644 --- a/tests/qemuxml2argvdata/net-user-addr.args +++ b/tests/qemuxml2argvdata/net-user-addr.args @@ -27,6 +27,7 @@ server,nowait \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ --netdev user,net=172.17.2.0/24,ipv6-net=2001:db8:ac10:fd01::/64,id=hostnet0 \ +-netdev user,net=172.17.2.0/24,ipv6-prefix=2001:db8:ac10:fd01::,\ +ipv6-prefixlen=64,id=hostnet0 \ -device rtl8139,netdev=hostnet0,id=net0,mac=00:11:22:33:44:55,bus=pci.0,\ addr=0x3 -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
In qemu the argument of 'ipv6-net' is split up into 'ipv6-prefix' and 'ipv6-prefixlen'. Additionally now that 'netdev_add' was qapified, only the real properties are allowed. Switch to using them explicitly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 20 ++++++++++---------- tests/qemuxml2argvdata/net-user-addr.args | 3 ++- 2 files changed, 12 insertions(+), 11 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Allow reusing this for formatting of netdev_add arguments into -netdev. We need to be able to skip the 'type' property as it's used without the prefix. Add infrastructure which allows skipping a certainly named property. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virqemu.c | 30 +++++++++++++++++++++--------- src/util/virqemu.h | 10 +++++++--- tests/qemucommandutiltest.c | 2 +- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/util/virqemu.c b/src/util/virqemu.c index 78a9e0480b..0e6fa412bc 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -36,6 +36,7 @@ VIR_LOG_INIT("util.qemu"); struct virQEMUCommandLineJSONIteratorData { const char *prefix; virBufferPtr buf; + const char *skipKey; virQEMUBuildCommandLineJSONArrayFormatFunc arrayFunc; }; @@ -44,6 +45,7 @@ static int virQEMUBuildCommandLineJSONRecurse(const char *key, virJSONValuePtr value, virBufferPtr buf, + const char *skipKey, virQEMUBuildCommandLineJSONArrayFormatFunc arrayFunc, bool nested); @@ -52,7 +54,8 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, int virQEMUBuildCommandLineJSONArrayBitmap(const char *key, virJSONValuePtr array, - virBufferPtr buf) + virBufferPtr buf, + const char *skipKey G_GNUC_UNUSED) { ssize_t pos = -1; ssize_t end; @@ -80,7 +83,8 @@ virQEMUBuildCommandLineJSONArrayBitmap(const char *key, int virQEMUBuildCommandLineJSONArrayNumbered(const char *key, virJSONValuePtr array, - virBufferPtr buf) + virBufferPtr buf, + const char *skipKey) { virJSONValuePtr member; size_t i; @@ -91,7 +95,7 @@ virQEMUBuildCommandLineJSONArrayNumbered(const char *key, prefix = g_strdup_printf("%s.%zu", key, i); - if (virQEMUBuildCommandLineJSONRecurse(prefix, member, buf, + if (virQEMUBuildCommandLineJSONRecurse(prefix, member, buf, skipKey, virQEMUBuildCommandLineJSONArrayNumbered, true) < 0) return 0; @@ -109,15 +113,20 @@ virQEMUBuildCommandLineJSONIterate(const char *key, { struct virQEMUCommandLineJSONIteratorData *data = opaque; + if (STREQ_NULLABLE(key, data->skipKey)) + return 0; + if (data->prefix) { g_autofree char *tmpkey = NULL; tmpkey = g_strdup_printf("%s.%s", data->prefix, key); return virQEMUBuildCommandLineJSONRecurse(tmpkey, value, data->buf, + data->skipKey, data->arrayFunc, false); } else { return virQEMUBuildCommandLineJSONRecurse(key, value, data->buf, + data->skipKey, data->arrayFunc, false); } } @@ -127,10 +136,11 @@ static int virQEMUBuildCommandLineJSONRecurse(const char *key, virJSONValuePtr value, virBufferPtr buf, + const char *skipKey, virQEMUBuildCommandLineJSONArrayFormatFunc arrayFunc, bool nested) { - struct virQEMUCommandLineJSONIteratorData data = { key, buf, arrayFunc }; + struct virQEMUCommandLineJSONIteratorData data = { key, buf, skipKey, arrayFunc }; virJSONType type = virJSONValueGetType(value); virJSONValuePtr elem; bool tmp; @@ -170,14 +180,14 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, return -1; } - if (!arrayFunc || arrayFunc(key, value, buf) < 0) { + if (!arrayFunc || arrayFunc(key, value, buf, skipKey) < 0) { /* fallback, treat the array as a non-bitmap, adding the key * for each member */ for (i = 0; i < virJSONValueArraySize(value); i++) { elem = virJSONValueArrayGet((virJSONValuePtr)value, i); /* recurse to avoid duplicating code */ - if (virQEMUBuildCommandLineJSONRecurse(key, elem, buf, + if (virQEMUBuildCommandLineJSONRecurse(key, elem, buf, skipKey, arrayFunc, true) < 0) return -1; } @@ -205,6 +215,7 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, * virQEMUBuildCommandLineJSON: * @value: json object containing the value * @buf: otuput buffer + * @skipKey: name of key that will be handled separately by caller * @arrayFunc: array formatter function to allow for different syntax * * Formats JSON value object into command line parameters suitable for use with @@ -215,9 +226,10 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, int virQEMUBuildCommandLineJSON(virJSONValuePtr value, virBufferPtr buf, + const char *skipKey, virQEMUBuildCommandLineJSONArrayFormatFunc array) { - if (virQEMUBuildCommandLineJSONRecurse(NULL, value, buf, array, false) < 0) + if (virQEMUBuildCommandLineJSONRecurse(NULL, value, buf, skipKey, array, false) < 0) return -1; virBufferTrim(buf, ","); @@ -243,7 +255,7 @@ virQEMUBuildObjectCommandlineFromJSONInternal(virBufferPtr buf, if (props) { virBufferAddLit(buf, ","); - if (virQEMUBuildCommandLineJSON(props, buf, + if (virQEMUBuildCommandLineJSON(props, buf, NULL, virQEMUBuildCommandLineJSONArrayBitmap) < 0) return -1; } @@ -270,7 +282,7 @@ virQEMUBuildDriveCommandlineFromJSON(virJSONValuePtr srcdef) virBuffer buf = VIR_BUFFER_INITIALIZER; char *ret = NULL; - if (virQEMUBuildCommandLineJSON(srcdef, &buf, + if (virQEMUBuildCommandLineJSON(srcdef, &buf, NULL, virQEMUBuildCommandLineJSONArrayNumbered) < 0) goto cleanup; diff --git a/src/util/virqemu.h b/src/util/virqemu.h index 227325e80e..9d3db7c2a2 100644 --- a/src/util/virqemu.h +++ b/src/util/virqemu.h @@ -29,16 +29,20 @@ typedef int (*virQEMUBuildCommandLineJSONArrayFormatFunc)(const char *key, virJSONValuePtr array, - virBufferPtr buf); + virBufferPtr buf, + const char *skipKey); int virQEMUBuildCommandLineJSONArrayBitmap(const char *key, virJSONValuePtr array, - virBufferPtr buf); + virBufferPtr buf, + const char *skipKey); int virQEMUBuildCommandLineJSONArrayNumbered(const char *key, virJSONValuePtr array, - virBufferPtr buf); + virBufferPtr buf, + const char *skipKey); int virQEMUBuildCommandLineJSON(virJSONValuePtr value, virBufferPtr buf, + const char *skipKey, virQEMUBuildCommandLineJSONArrayFormatFunc array); int virQEMUBuildObjectCommandlineFromJSON(virBufferPtr buf, diff --git a/tests/qemucommandutiltest.c b/tests/qemucommandutiltest.c index c5b3e7b735..923776e642 100644 --- a/tests/qemucommandutiltest.c +++ b/tests/qemucommandutiltest.c @@ -47,7 +47,7 @@ testQemuCommandBuildFromJSON(const void *opaque) return -1; } - if (virQEMUBuildCommandLineJSON(val, &buf, data->arrayfunc) < 0) { + if (virQEMUBuildCommandLineJSON(val, &buf, NULL, data->arrayfunc) < 0) { fprintf(stderr, "\nvirQEMUBuildCommandlineJSON failed process JSON:\n%s\n", data->props); -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
Allow reusing this for formatting of netdev_add arguments into -netdev. We need to be able to skip the 'type' property as it's used without the prefix.
Not quite true. In qemu's net/net.c, there is: QemuOptsList qemu_netdev_opts = { .name = "netdev", .implied_opt_name = "type", .head = QTAILQ_HEAD_INITIALIZER(qemu_netdev_opts.head), .desc = { /* * no elements => accept any params * validation will happen later */ { /* end of list */ } }, The handling of .implied_opt_name means that the following two commands are identical: qemu-kvm -netdev user,id=net0 qemu-kvm -netdev type=user,id=net0 So you don't have to specifically handle type= specially. That said, I don't see any problem in adding this special-casing.
Add infrastructure which allows skipping a certainly named property.
Maybe s/certainly/specifically/
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virqemu.c | 30 +++++++++++++++++++++--------- src/util/virqemu.h | 10 +++++++--- tests/qemucommandutiltest.c | 2 +- 3 files changed, 29 insertions(+), 13 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
In some cases we use 'on/off' for command line arguments. Add a switch which will select the prefred spelling for a specific usage. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virqemu.c | 44 ++++++++++++++++++++++++------------- src/util/virqemu.h | 10 ++++++--- tests/qemucommandutiltest.c | 2 +- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/src/util/virqemu.c b/src/util/virqemu.c index 0e6fa412bc..549f88fcd5 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -37,6 +37,7 @@ struct virQEMUCommandLineJSONIteratorData { const char *prefix; virBufferPtr buf; const char *skipKey; + bool onOff; virQEMUBuildCommandLineJSONArrayFormatFunc arrayFunc; }; @@ -46,6 +47,7 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, virJSONValuePtr value, virBufferPtr buf, const char *skipKey, + bool onOff, virQEMUBuildCommandLineJSONArrayFormatFunc arrayFunc, bool nested); @@ -55,7 +57,8 @@ int virQEMUBuildCommandLineJSONArrayBitmap(const char *key, virJSONValuePtr array, virBufferPtr buf, - const char *skipKey G_GNUC_UNUSED) + const char *skipKey G_GNUC_UNUSED, + bool onOff G_GNUC_UNUSED) { ssize_t pos = -1; ssize_t end; @@ -84,7 +87,8 @@ int virQEMUBuildCommandLineJSONArrayNumbered(const char *key, virJSONValuePtr array, virBufferPtr buf, - const char *skipKey) + const char *skipKey, + bool onOff) { virJSONValuePtr member; size_t i; @@ -95,7 +99,7 @@ virQEMUBuildCommandLineJSONArrayNumbered(const char *key, prefix = g_strdup_printf("%s.%zu", key, i); - if (virQEMUBuildCommandLineJSONRecurse(prefix, member, buf, skipKey, + if (virQEMUBuildCommandLineJSONRecurse(prefix, member, buf, skipKey, onOff, virQEMUBuildCommandLineJSONArrayNumbered, true) < 0) return 0; @@ -122,11 +126,11 @@ virQEMUBuildCommandLineJSONIterate(const char *key, tmpkey = g_strdup_printf("%s.%s", data->prefix, key); return virQEMUBuildCommandLineJSONRecurse(tmpkey, value, data->buf, - data->skipKey, + data->skipKey, data->onOff, data->arrayFunc, false); } else { return virQEMUBuildCommandLineJSONRecurse(key, value, data->buf, - data->skipKey, + data->skipKey, data->onOff, data->arrayFunc, false); } } @@ -137,10 +141,11 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, virJSONValuePtr value, virBufferPtr buf, const char *skipKey, + bool onOff, virQEMUBuildCommandLineJSONArrayFormatFunc arrayFunc, bool nested) { - struct virQEMUCommandLineJSONIteratorData data = { key, buf, skipKey, arrayFunc }; + struct virQEMUCommandLineJSONIteratorData data = { key, buf, skipKey, onOff, arrayFunc }; virJSONType type = virJSONValueGetType(value); virJSONValuePtr elem; bool tmp; @@ -165,10 +170,17 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, case VIR_JSON_TYPE_BOOLEAN: virJSONValueGetBoolean(value, &tmp); - if (tmp) - virBufferAsprintf(buf, "%s=yes,", key); - else - virBufferAsprintf(buf, "%s=no,", key); + if (onOff) { + if (tmp) + virBufferAsprintf(buf, "%s=on,", key); + else + virBufferAsprintf(buf, "%s=off,", key); + } else { + if (tmp) + virBufferAsprintf(buf, "%s=yes,", key); + else + virBufferAsprintf(buf, "%s=no,", key); + } break; @@ -180,7 +192,7 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, return -1; } - if (!arrayFunc || arrayFunc(key, value, buf, skipKey) < 0) { + if (!arrayFunc || arrayFunc(key, value, buf, skipKey, onOff) < 0) { /* fallback, treat the array as a non-bitmap, adding the key * for each member */ for (i = 0; i < virJSONValueArraySize(value); i++) { @@ -188,7 +200,7 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, /* recurse to avoid duplicating code */ if (virQEMUBuildCommandLineJSONRecurse(key, elem, buf, skipKey, - arrayFunc, true) < 0) + onOff, arrayFunc, true) < 0) return -1; } } @@ -216,6 +228,7 @@ virQEMUBuildCommandLineJSONRecurse(const char *key, * @value: json object containing the value * @buf: otuput buffer * @skipKey: name of key that will be handled separately by caller + * @onOff: Use 'on' and 'off' for boolean values rather than 'yes' and 'no' * @arrayFunc: array formatter function to allow for different syntax * * Formats JSON value object into command line parameters suitable for use with @@ -227,9 +240,10 @@ int virQEMUBuildCommandLineJSON(virJSONValuePtr value, virBufferPtr buf, const char *skipKey, + bool onOff, virQEMUBuildCommandLineJSONArrayFormatFunc array) { - if (virQEMUBuildCommandLineJSONRecurse(NULL, value, buf, skipKey, array, false) < 0) + if (virQEMUBuildCommandLineJSONRecurse(NULL, value, buf, skipKey, onOff, array, false) < 0) return -1; virBufferTrim(buf, ","); @@ -255,7 +269,7 @@ virQEMUBuildObjectCommandlineFromJSONInternal(virBufferPtr buf, if (props) { virBufferAddLit(buf, ","); - if (virQEMUBuildCommandLineJSON(props, buf, NULL, + if (virQEMUBuildCommandLineJSON(props, buf, NULL, false, virQEMUBuildCommandLineJSONArrayBitmap) < 0) return -1; } @@ -282,7 +296,7 @@ virQEMUBuildDriveCommandlineFromJSON(virJSONValuePtr srcdef) virBuffer buf = VIR_BUFFER_INITIALIZER; char *ret = NULL; - if (virQEMUBuildCommandLineJSON(srcdef, &buf, NULL, + if (virQEMUBuildCommandLineJSON(srcdef, &buf, NULL, false, virQEMUBuildCommandLineJSONArrayNumbered) < 0) goto cleanup; diff --git a/src/util/virqemu.h b/src/util/virqemu.h index 9d3db7c2a2..67a5711613 100644 --- a/src/util/virqemu.h +++ b/src/util/virqemu.h @@ -30,19 +30,23 @@ typedef int (*virQEMUBuildCommandLineJSONArrayFormatFunc)(const char *key, virJSONValuePtr array, virBufferPtr buf, - const char *skipKey); + const char *skipKey, + bool onOff); int virQEMUBuildCommandLineJSONArrayBitmap(const char *key, virJSONValuePtr array, virBufferPtr buf, - const char *skipKey); + const char *skipKey, + bool onOff); int virQEMUBuildCommandLineJSONArrayNumbered(const char *key, virJSONValuePtr array, virBufferPtr buf, - const char *skipKey); + const char *skipKey, + bool onOff); int virQEMUBuildCommandLineJSON(virJSONValuePtr value, virBufferPtr buf, const char *skipKey, + bool onOff, virQEMUBuildCommandLineJSONArrayFormatFunc array); int virQEMUBuildObjectCommandlineFromJSON(virBufferPtr buf, diff --git a/tests/qemucommandutiltest.c b/tests/qemucommandutiltest.c index 923776e642..049fd2f0b0 100644 --- a/tests/qemucommandutiltest.c +++ b/tests/qemucommandutiltest.c @@ -47,7 +47,7 @@ testQemuCommandBuildFromJSON(const void *opaque) return -1; } - if (virQEMUBuildCommandLineJSON(val, &buf, NULL, data->arrayfunc) < 0) { + if (virQEMUBuildCommandLineJSON(val, &buf, NULL, false, data->arrayfunc) < 0) { fprintf(stderr, "\nvirQEMUBuildCommandlineJSON failed process JSON:\n%s\n", data->props); -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
In some cases we use 'on/off' for command line arguments. Add a switch which will select the prefred spelling for a specific usage.
preferred
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virqemu.c | 44 ++++++++++++++++++++++++------------- src/util/virqemu.h | 10 ++++++--- tests/qemucommandutiltest.c | 2 +- 3 files changed, 37 insertions(+), 19 deletions(-)
@@ -165,10 +170,17 @@ virQEMUBuildCommandLineJSONRecurse(const char *key,
case VIR_JSON_TYPE_BOOLEAN: virJSONValueGetBoolean(value, &tmp); - if (tmp) - virBufferAsprintf(buf, "%s=yes,", key); - else - virBufferAsprintf(buf, "%s=no,", key); + if (onOff) { + if (tmp) + virBufferAsprintf(buf, "%s=on,", key); + else + virBufferAsprintf(buf, "%s=off,", key); + } else { + if (tmp) + virBufferAsprintf(buf, "%s=yes,", key); + else + virBufferAsprintf(buf, "%s=no,", key); + }
It would be nice if there were a single spelling we could use for all boolean CLI parameters across all supported qemu versions. But this is certainly the most conservative way to deal with the fact that some of the CLI parameters are ad hoc rather than sharing common code, and thus inconsistent on which spellings work where. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
The helper returns a list of arguments of a virCommand. This will be useful in tests where we'll inspect certain already formatted arguments. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 23 +++++++++++++++++++++++ src/util/vircommand.h | 1 + 3 files changed, 25 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 935ef7303b..31813f177f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1841,6 +1841,7 @@ virCommandDaemonize; virCommandDoAsyncIO; virCommandExec; virCommandFree; +virCommandGetArgList; virCommandGetGID; virCommandGetUID; virCommandHandshakeNotify; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 20f196104f..aae0ddb730 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2132,6 +2132,29 @@ virCommandToString(virCommandPtr cmd, bool linebreaks) } +int +virCommandGetArgList(virCommandPtr cmd, + char ***args, + size_t *nargs) +{ + size_t i; + + if (cmd->has_error) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return -1; + } + + *args = g_new0(char *, cmd->nargs); + *nargs = cmd->nargs - 1; + + for (i = 1; i < cmd->nargs; i++) + (*args)[i - 1] = g_strdup(cmd->args[i]); + + return 0; +} + + #ifndef WIN32 /* * Manage input and output to the child process. diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e2be5bcf1c..ff8a785dbe 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -170,6 +170,7 @@ void virCommandWriteArgLog(virCommandPtr cmd, int logfd); char *virCommandToString(virCommandPtr cmd, bool linebreaks) G_GNUC_WARN_UNUSED_RESULT; +int virCommandGetArgList(virCommandPtr cmd, char ***args, size_t *nargs); int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups) G_GNUC_WARN_UNUSED_RESULT; -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
The helper returns a list of arguments of a virCommand. This will be useful in tests where we'll inspect certain already formatted arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 23 +++++++++++++++++++++++ src/util/vircommand.h | 1 + 3 files changed, 25 insertions(+)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Add a variant similar to virJSONValueObjectAppendString which also formats more complex value strings with printf syntax. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 17 +++++++++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 20 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 31813f177f..1bd02fd8ee 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2361,6 +2361,7 @@ virJSONValueObjectAppendNumberLong; virJSONValueObjectAppendNumberUint; virJSONValueObjectAppendNumberUlong; virJSONValueObjectAppendString; +virJSONValueObjectAppendStringPrintf; virJSONValueObjectCreate; virJSONValueObjectCreateVArgs; virJSONValueObjectDeflatten; diff --git a/src/util/virjson.c b/src/util/virjson.c index dc662bf8e9..6921eccb60 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -649,6 +649,23 @@ virJSONValueObjectAppendString(virJSONValuePtr object, } +int +virJSONValueObjectAppendStringPrintf(virJSONValuePtr object, + const char *key, + const char *fmt, + ...) +{ + va_list ap; + g_autofree char *str = NULL; + + va_start(ap, fmt); + str = g_strdup_vprintf(fmt, ap); + va_end(ap); + + return virJSONValueObjectInsertString(object, key, str, false); +} + + int virJSONValueObjectPrependString(virJSONValuePtr object, const char *key, diff --git a/src/util/virjson.h b/src/util/virjson.h index 0894e91b59..588c977650 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -127,6 +127,8 @@ int virJSONValueObjectGetBoolean(virJSONValuePtr object, const char *key, bool * int virJSONValueObjectIsNull(virJSONValuePtr object, const char *key); int virJSONValueObjectAppendString(virJSONValuePtr object, const char *key, const char *value); +int virJSONValueObjectAppendStringPrintf(virJSONValuePtr object, const char *key, const char *fmt, ...) + G_GNUC_PRINTF(3, 4); int virJSONValueObjectPrependString(virJSONValuePtr object, const char *key, const char *value); int virJSONValueObjectAppendNumberInt(virJSONValuePtr object, const char *key, int number); int virJSONValueObjectAppendNumberUint(virJSONValuePtr object, const char *key, unsigned int number); -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
Add a variant similar to virJSONValueObjectAppendString which also formats more complex value strings with printf syntax.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 17 +++++++++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 20 insertions(+)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
There are multiple steps of setting up the domain definition prior to formatting the command line for the tests. Extract it to a separate function so that it's self-contained and also will allow re-running the command line formatting which will be necessary for QMP schema validation tests. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 158 +++++++++++++++++++++------------------ 1 file changed, 86 insertions(+), 72 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 43e76956cc..9fa1afd9d0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -391,6 +391,90 @@ testCheckExclusiveFlags(int flags) } +static virCommandPtr +testCompareXMLToArgvCreateArgs(virQEMUDriverPtr drv, + virDomainObjPtr vm, + const char *migrateURI, + struct testQemuInfo *info, + unsigned int flags) +{ + size_t i; + + for (i = 0; i < vm->def->nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i]; + + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && + hostdev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { + hostdev->source.subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO; + } + } + + for (i = 0; i < vm->def->nfss; i++) { + virDomainFSDefPtr fs = vm->def->fss[i]; + char *s; + + if (fs->fsdriver != VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS || + QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock) + continue; + + s = g_strdup_printf("/tmp/lib/domain--1-guest/fs%zu.vhost-fs.sock", i); + QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock = s; + } + + if (vm->def->vsock) { + virDomainVsockDefPtr vsock = vm->def->vsock; + qemuDomainVsockPrivatePtr vsockPriv = + (qemuDomainVsockPrivatePtr)vsock->privateData; + + if (vsock->auto_cid == VIR_TRISTATE_BOOL_YES) + vsock->guest_cid = 42; + + vsockPriv->vhostfd = 6789; + } + + if (vm->def->tpm) { + switch (vm->def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + VIR_FREE(vm->def->tpm->data.emulator.source.data.file.path); + vm->def->tpm->data.emulator.source.data.file.path = g_strdup("/dev/test"); + vm->def->tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_FILE; + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + } + + for (i = 0; i < vm->def->nvideos; i++) { + virDomainVideoDefPtr video = vm->def->videos[i]; + + if (video->backend == VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER) { + qemuDomainVideoPrivatePtr vpriv = QEMU_DOMAIN_VIDEO_PRIVATE(video); + + vpriv->vhost_user_fd = 1729; + } + } + + if (flags & FLAG_SLIRP_HELPER) { + for (i = 0; i < vm->def->nnets; i++) { + virDomainNetDefPtr net = vm->def->nets[i]; + + if (net->type == VIR_DOMAIN_NET_TYPE_USER && + virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { + qemuSlirpPtr slirp = qemuSlirpNew(); + slirp->fd[0] = 42; + QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp = slirp; + } + } + } + + return qemuProcessCreatePretendCmd(drv, vm, migrateURI, + (flags & FLAG_FIPS), false, + VIR_QEMU_PROCESS_START_COLD); +} + + static int testCompareXMLToArgv(const void *data) { @@ -405,7 +489,6 @@ testCompareXMLToArgv(const void *data) virConnectPtr conn; char *log = NULL; virCommandPtr cmd = NULL; - size_t i; qemuDomainObjPrivatePtr priv = NULL; if (info->arch != VIR_ARCH_NONE && info->arch != VIR_ARCH_X86_64) @@ -482,77 +565,8 @@ testCompareXMLToArgv(const void *data) VIR_FREE(log); virResetLastError(); - for (i = 0; i < vm->def->nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i]; - - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && - hostdev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { - hostdev->source.subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO; - } - } - - for (i = 0; i < vm->def->nfss; i++) { - virDomainFSDefPtr fs = vm->def->fss[i]; - char *s; - - if (fs->fsdriver != VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) - continue; - - s = g_strdup_printf("/tmp/lib/domain--1-guest/fs%zu.vhost-fs.sock", i); - QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock = s; - } - - if (vm->def->vsock) { - virDomainVsockDefPtr vsock = vm->def->vsock; - qemuDomainVsockPrivatePtr vsockPriv = - (qemuDomainVsockPrivatePtr)vsock->privateData; - - if (vsock->auto_cid == VIR_TRISTATE_BOOL_YES) - vsock->guest_cid = 42; - - vsockPriv->vhostfd = 6789; - } - - if (vm->def->tpm) { - switch (vm->def->tpm->type) { - case VIR_DOMAIN_TPM_TYPE_EMULATOR: - VIR_FREE(vm->def->tpm->data.emulator.source.data.file.path); - vm->def->tpm->data.emulator.source.data.file.path = g_strdup("/dev/test"); - vm->def->tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_FILE; - break; - case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - case VIR_DOMAIN_TPM_TYPE_LAST: - break; - } - } - - for (i = 0; i < vm->def->nvideos; i++) { - virDomainVideoDefPtr video = vm->def->videos[i]; - - if (video->backend == VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER) { - qemuDomainVideoPrivatePtr vpriv = QEMU_DOMAIN_VIDEO_PRIVATE(video); - - vpriv->vhost_user_fd = 1729; - } - } - - if (flags & FLAG_SLIRP_HELPER) { - for (i = 0; i < vm->def->nnets; i++) { - virDomainNetDefPtr net = vm->def->nets[i]; - - if (net->type == VIR_DOMAIN_NET_TYPE_USER && - virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { - qemuSlirpPtr slirp = qemuSlirpNew(); - slirp->fd[0] = 42; - QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp = slirp; - } - } - } - - if (!(cmd = qemuProcessCreatePretendCmd(&driver, vm, migrateURI, - (flags & FLAG_FIPS), false, - VIR_QEMU_PROCESS_START_COLD))) { + if (!(cmd = testCompareXMLToArgvCreateArgs(&driver, vm, migrateURI, info, + flags))) { if (flags & FLAG_EXPECT_FAILURE) goto ok; goto cleanup; -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
There are multiple steps of setting up the domain definition prior to formatting the command line for the tests. Extract it to a separate function so that it's self-contained and also will allow re-running the command line formatting which will be necessary for QMP schema validation tests.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 158 +++++++++++++++++++++------------------ 1 file changed, 86 insertions(+), 72 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Use automatic pointer cleanup for virJSONValuePtrs to get rid of the cleanup label and ret variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 52 +++++++++++++++--------------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 66422a8489..1224d1c9cc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3984,13 +3984,13 @@ int qemuMonitorJSONCloseFileHandle(qemuMonitorPtr mon, } -int qemuMonitorJSONAddNetdev(qemuMonitorPtr mon, - const char *netdevstr) +int +qemuMonitorJSONAddNetdev(qemuMonitorPtr mon, + const char *netdevstr) { - int ret = -1; - virJSONValuePtr cmd = NULL; - virJSONValuePtr reply = NULL; - virJSONValuePtr args = NULL; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + g_autoptr(virJSONValue) args = NULL; cmd = qemuMonitorJSONMakeCommand("netdev_add", NULL); if (!cmd) @@ -3998,49 +3998,41 @@ int qemuMonitorJSONAddNetdev(qemuMonitorPtr mon, args = qemuMonitorJSONKeywordStringToJSON(netdevstr, "type"); if (!args) - goto cleanup; + return -1; if (virJSONValueObjectAppend(cmd, "arguments", args) < 0) - goto cleanup; + return -1; args = NULL; /* obj owns reference to args now */ if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virJSONValueFree(args); - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; } -int qemuMonitorJSONRemoveNetdev(qemuMonitorPtr mon, - const char *alias) +int +qemuMonitorJSONRemoveNetdev(qemuMonitorPtr mon, + const char *alias) { - int ret = -1; - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("netdev_del", - "s:id", alias, - NULL); - virJSONValuePtr reply = NULL; + g_autoptr(virJSONValue) cmd = qemuMonitorJSONMakeCommand("netdev_del", + "s:id", alias, + NULL); + g_autoptr(virJSONValue) reply = NULL; + if (!cmd) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; } -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
Use automatic pointer cleanup for virJSONValuePtrs to get rid of the cleanup label and ret variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 52 +++++++++++++++--------------------- 1 file changed, 22 insertions(+), 30 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
In preparation for converting the generator of -netdev to generate JSON which will be used to do the command line rather than the other way around we need to introduce a convertor which properly configures virQEMUBuildCommandLineJSON for the quirks of -netdev. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virqemu.c | 22 ++++++++++++++++++++++ src/util/virqemu.h | 3 +++ 3 files changed, 26 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1bd02fd8ee..fd04fcece3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2915,6 +2915,7 @@ virQEMUBuildCommandLineJSON; virQEMUBuildCommandLineJSONArrayBitmap; virQEMUBuildCommandLineJSONArrayNumbered; virQEMUBuildDriveCommandlineFromJSON; +virQEMUBuildNetdevCommandlineFromJSON; virQEMUBuildObjectCommandlineFromJSON; virQEMUBuildQemuImgKeySecretOpts; diff --git a/src/util/virqemu.c b/src/util/virqemu.c index 549f88fcd5..0f8cab29df 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -252,6 +252,28 @@ virQEMUBuildCommandLineJSON(virJSONValuePtr value, } +/** + * virQEMUBuildNetdevCommandlineFromJSON: + * @props: JSON properties describing a netdev + * + * Converts @props into arguments for -netdev including all the quirks and + * differences between the monitor and command line syntax. + */ +char * +virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props) +{ + const char *type = virJSONValueObjectGetString(props, "type"); + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s,", type); + + if (virQEMUBuildCommandLineJSON(props, &buf, "type", true, NULL) < 0) + return NULL; + + return virBufferContentAndReset(&buf); +} + + static int virQEMUBuildObjectCommandlineFromJSONInternal(virBufferPtr buf, const char *type, diff --git a/src/util/virqemu.h b/src/util/virqemu.h index 67a5711613..22f47851df 100644 --- a/src/util/virqemu.h +++ b/src/util/virqemu.h @@ -49,6 +49,9 @@ int virQEMUBuildCommandLineJSON(virJSONValuePtr value, bool onOff, virQEMUBuildCommandLineJSONArrayFormatFunc array); +char * +virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props); + int virQEMUBuildObjectCommandlineFromJSON(virBufferPtr buf, virJSONValuePtr objprops); -- 2.26.2
On 5/15/20 10:27 AM, Peter Krempa wrote:
In preparation for converting the generator of -netdev to generate JSON which will be used to do the command line rather than the other way around we need to introduce a convertor which properly configures virQEMUBuildCommandLineJSON for the quirks of -netdev.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virqemu.c | 22 ++++++++++++++++++++++ src/util/virqemu.h | 3 +++ 3 files changed, 26 insertions(+)
+/** + * virQEMUBuildNetdevCommandlineFromJSON: + * @props: JSON properties describing a netdev + * + * Converts @props into arguments for -netdev including all the quirks and + * differences between the monitor and command line syntax. + */ +char * +virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props) +{ + const char *type = virJSONValueObjectGetString(props, "type"); + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s,", type); + + if (virQEMUBuildCommandLineJSON(props, &buf, "type", true, NULL) < 0) + return NULL;
Do we actually have to special-case "type"?
+ + return virBufferContentAndReset(&buf); +} + + static int virQEMUBuildObjectCommandlineFromJSONInternal(virBufferPtr buf, const char *type, diff --git a/src/util/virqemu.h b/src/util/virqemu.h index 67a5711613..22f47851df 100644 --- a/src/util/virqemu.h +++ b/src/util/virqemu.h @@ -49,6 +49,9 @@ int virQEMUBuildCommandLineJSON(virJSONValuePtr value, bool onOff, virQEMUBuildCommandLineJSONArrayFormatFunc array);
+char * +virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props); + int virQEMUBuildObjectCommandlineFromJSON(virBufferPtr buf, virJSONValuePtr objprops);
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, May 18, 2020 at 10:07:25 -0500, Eric Blake wrote:
On 5/15/20 10:27 AM, Peter Krempa wrote:
In preparation for converting the generator of -netdev to generate JSON which will be used to do the command line rather than the other way around we need to introduce a convertor which properly configures virQEMUBuildCommandLineJSON for the quirks of -netdev.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virqemu.c | 22 ++++++++++++++++++++++ src/util/virqemu.h | 3 +++ 3 files changed, 26 insertions(+)
+/** + * virQEMUBuildNetdevCommandlineFromJSON: + * @props: JSON properties describing a netdev + * + * Converts @props into arguments for -netdev including all the quirks and + * differences between the monitor and command line syntax. + */ +char * +virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props) +{ + const char *type = virJSONValueObjectGetString(props, "type"); + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s,", type); + + if (virQEMUBuildCommandLineJSON(props, &buf, "type", true, NULL) < 0) + return NULL;
Do we actually have to special-case "type"?
I'd have to go back and see since when we can use type= explicitly with qemu and I didn't really feel doing that across all supported qemu versions. Well, to be fair not even across the reasonable versions.
The 'netdev_add' command was recently formally described in qemu via the QMP schema. This means that it also requires the arguments to be properly formatted. Our current approach is to generate the command line and then use qemuMonitorJSONKeywordStringToJSON to get the JSON properties for the monitor. This will not work if we need to pass some fields as numbers or booleans. In this step we re-do internals of qemuBuildHostNetStr to format a JSON object which is converted back via virQEMUBuildNetdevCommandlineFromJSON to the equivalent command line. This will later allow fixing of the monitor code to use the JSON object directly rather than rely on the conversion. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 163 +++++++++++++++++++++++++--------------- src/qemu/qemu_command.h | 12 +-- src/qemu/qemu_hotplug.c | 13 +++- 3 files changed, 119 insertions(+), 69 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2850953bd0..00d1b4121d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3564,7 +3564,7 @@ qemuBuildNicDevStr(virDomainDefPtr def, } -char * +virJSONValuePtr qemuBuildHostNetStr(virDomainNetDefPtr net, char **tapfd, size_t tapfdSize, @@ -3573,10 +3573,11 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, const char *slirpfd) { bool is_tap = false; - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; virDomainNetType netType = virDomainNetGetActualType(net); size_t i; + g_autoptr(virJSONValue) netprops = NULL; + if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("scripts are not supported on interfaces of type %s"), @@ -3594,54 +3595,75 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_ETHERNET: - virBufferAddLit(&buf, "tap,"); + if (virJSONValueObjectCreate(&netprops, "s:type", "tap", NULL) < 0) + return NULL; + /* for one tapfd 'fd=' shall be used, * for more than one 'fds=' is the right choice */ if (tapfdSize == 1) { - virBufferAsprintf(&buf, "fd=%s,", tapfd[0]); + if (virJSONValueObjectAdd(netprops, "s:fd", tapfd[0], NULL) < 0) + return NULL; } else { - virBufferAddLit(&buf, "fds="); - for (i = 0; i < tapfdSize; i++) { - if (i) - virBufferAddChar(&buf, ':'); - virBufferAdd(&buf, tapfd[i], -1); - } - virBufferAddChar(&buf, ','); + g_auto(virBuffer) fdsbuf = VIR_BUFFER_INITIALIZER; + + for (i = 0; i < tapfdSize; i++) + virBufferAsprintf(&fdsbuf, "%s:", tapfd[i]); + + virBufferTrim(&fdsbuf, ":"); + + if (virJSONValueObjectAdd(netprops, + "s:fds", virBufferCurrentContent(&fdsbuf), + NULL) < 0) + return NULL; } + is_tap = true; break; case VIR_DOMAIN_NET_TYPE_CLIENT: - virBufferAsprintf(&buf, "socket,connect=%s:%d,", - net->data.socket.address, - net->data.socket.port); + if (virJSONValueObjectCreate(&netprops, "s:type", "socket", NULL) < 0 || + virJSONValueObjectAppendStringPrintf(netprops, "connect", "%s:%d", + net->data.socket.address, + net->data.socket.port) < 0) + return NULL; break; case VIR_DOMAIN_NET_TYPE_SERVER: - virBufferAsprintf(&buf, "socket,listen=%s:%d,", - NULLSTR_EMPTY(net->data.socket.address), - net->data.socket.port); + if (virJSONValueObjectCreate(&netprops, "s:type", "socket", NULL) < 0 || + virJSONValueObjectAppendStringPrintf(netprops, "listen", "%s:%d", + NULLSTR_EMPTY(net->data.socket.address), + net->data.socket.port) < 0) + return NULL; break; case VIR_DOMAIN_NET_TYPE_MCAST: - virBufferAsprintf(&buf, "socket,mcast=%s:%d,", - net->data.socket.address, - net->data.socket.port); + if (virJSONValueObjectCreate(&netprops, "s:type", "socket", NULL) < 0 || + virJSONValueObjectAppendStringPrintf(netprops, "mcast", "%s:%d", + net->data.socket.address, + net->data.socket.port) < 0) + return NULL; break; case VIR_DOMAIN_NET_TYPE_UDP: - virBufferAsprintf(&buf, "socket,udp=%s:%d,localaddr=%s:%d,", - net->data.socket.address, - net->data.socket.port, - net->data.socket.localaddr, - net->data.socket.localport); + if (virJSONValueObjectCreate(&netprops, "s:type", "socket", NULL) < 0 || + virJSONValueObjectAppendStringPrintf(netprops, "udp", "%s:%d", + net->data.socket.address, + net->data.socket.port) < 0 || + virJSONValueObjectAppendStringPrintf(netprops, "localaddr", "%s:%d", + net->data.socket.localaddr, + net->data.socket.localport) < 0) + return NULL; break; case VIR_DOMAIN_NET_TYPE_USER: if (slirpfd) { - virBufferAsprintf(&buf, "socket,fd=%s,", slirpfd); + if (virJSONValueObjectCreate(&netprops, "s:type", "socket", NULL) < 0 || + virJSONValueObjectAppendString(netprops, "fd", slirpfd) < 0) + return NULL; } else { - virBufferAddLit(&buf, "user,"); + if (virJSONValueObjectCreate(&netprops, "s:type", "user", NULL) < 0) + return NULL; + for (i = 0; i < net->guestIP.nips; i++) { const virNetDevIPAddr *ip = net->guestIP.ips[i]; g_autofree char *addr = NULL; @@ -3650,29 +3672,40 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, return NULL; if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET)) { - virBufferAsprintf(&buf, "net=%s", addr); + g_autofree char *ipv4netaddr = NULL; + if (ip->prefix) - virBufferAsprintf(&buf, "/%u", ip->prefix); - virBufferAddChar(&buf, ','); + ipv4netaddr = g_strdup_printf("%s/%u", addr, ip->prefix); + else + ipv4netaddr = g_strdup(addr); + + if (virJSONValueObjectAppendString(netprops, "net", ipv4netaddr) < 0) + return NULL; } else if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6)) { - virBufferAsprintf(&buf, "ipv6-prefix=%s,", addr); - if (ip->prefix) - virBufferAsprintf(&buf, "ipv6-prefixlen=%u,", ip->prefix); + if (virJSONValueObjectAppendString(netprops, "ipv6-prefix", addr) < 0) + return NULL; + if (ip->prefix && + virJSONValueObjectAppendNumberUlong(netprops, "ipv6-prefixlen", + ip->prefix) < 0) + return NULL; } } } break; case VIR_DOMAIN_NET_TYPE_INTERNAL: - virBufferAddLit(&buf, "user,"); + if (virJSONValueObjectCreate(&netprops, "s:type", "user", NULL) < 0) + return NULL; break; case VIR_DOMAIN_NET_TYPE_VHOSTUSER: - virBufferAsprintf(&buf, "vhost-user,chardev=char%s,", - net->info.alias); - if (net->driver.virtio.queues > 1) - virBufferAsprintf(&buf, "queues=%u,", - net->driver.virtio.queues); + if (virJSONValueObjectCreate(&netprops, "s:type", "vhost-user", NULL) < 0 || + virJSONValueObjectAppendStringPrintf(netprops, "chardev", "char%s", net->info.alias) < 0) + return NULL; + + if (net->driver.virtio.queues > 1 && + virJSONValueObjectAppendNumberUlong(netprops, "queues", net->driver.virtio.queues) < 0) + return NULL; break; case VIR_DOMAIN_NET_TYPE_HOSTDEV: @@ -3681,31 +3714,38 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, break; } - virBufferAsprintf(&buf, "id=host%s,", net->info.alias); + if (virJSONValueObjectAppendStringPrintf(netprops, "id", "host%s", net->info.alias) < 0) + return NULL; if (is_tap) { if (vhostfdSize) { - virBufferAddLit(&buf, "vhost=on,"); + if (virJSONValueObjectAppendBoolean(netprops, "vhost", true) < 0) + return NULL; + if (vhostfdSize == 1) { - virBufferAsprintf(&buf, "vhostfd=%s,", vhostfd[0]); + if (virJSONValueObjectAdd(netprops, "s:vhostfd", vhostfd[0], NULL) < 0) + return NULL; } else { - virBufferAddLit(&buf, "vhostfds="); - for (i = 0; i < vhostfdSize; i++) { - if (i) - virBufferAddChar(&buf, ':'); - virBufferAdd(&buf, vhostfd[i], -1); - } - virBufferAddChar(&buf, ','); + g_auto(virBuffer) fdsbuf = VIR_BUFFER_INITIALIZER; + + for (i = 0; i < vhostfdSize; i++) + virBufferAsprintf(&fdsbuf, "%s:", vhostfd[i]); + + virBufferTrim(&fdsbuf, ":"); + + if (virJSONValueObjectAdd(netprops, + "s:vhostfds", virBufferCurrentContent(&fdsbuf), + NULL) < 0) + return NULL; } } - if (net->tune.sndbuf_specified) - virBufferAsprintf(&buf, "sndbuf=%lu,", net->tune.sndbuf); - } - - virBufferTrim(&buf, ","); + if (net->tune.sndbuf_specified && + virJSONValueObjectAppendNumberUlong(netprops, "sndbuf", net->tune.sndbuf) < 0) + return NULL; + } - return virBufferContentAndReset(&buf); + return g_steal_pointer(&netprops); } @@ -7670,6 +7710,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, bool requireNicdev = false; qemuSlirpPtr slirp; size_t i; + g_autoptr(virJSONValue) hostnetprops = NULL; if (!bootindex) @@ -7873,11 +7914,15 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, if (chardev) virCommandAddArgList(cmd, "-chardev", chardev, NULL); - if (!(host = qemuBuildHostNetStr(net, - tapfdName, tapfdSize, - vhostfdName, vhostfdSize, - slirpfdName))) + if (!(hostnetprops = qemuBuildHostNetStr(net, + tapfdName, tapfdSize, + vhostfdName, vhostfdSize, + slirpfdName))) goto cleanup; + + if (!(host = virQEMUBuildNetdevCommandlineFromJSON(hostnetprops))) + goto cleanup; + virCommandAddArgList(cmd, "-netdev", host, NULL); /* Possible combinations: diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 7665b68548..6cdd9debe0 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -89,12 +89,12 @@ qemuBuildChrDeviceStr(char **deviceStr, char * qemuBuildChannelGuestfwdNetdevProps(virDomainChrDefPtr chr); -char *qemuBuildHostNetStr(virDomainNetDefPtr net, - char **tapfd, - size_t tapfdSize, - char **vhostfd, - size_t vhostfdSize, - const char *slirpfd); +virJSONValuePtr qemuBuildHostNetStr(virDomainNetDefPtr net, + char **tapfd, + size_t tapfdSize, + char **vhostfd, + size_t vhostfdSize, + const char *slirpfd); /* Current, best practice */ char *qemuBuildNicDevStr(virDomainDefPtr def, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2976ba7647..c6789dcef3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -54,6 +54,7 @@ #include "virstoragefile.h" #include "virstring.h" #include "virtime.h" +#include "virqemu.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1157,6 +1158,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, size_t vhostfdSize = 0; size_t queueSize = 0; g_autofree char *nicstr = NULL; + g_autoptr(virJSONValue) netprops = NULL; g_autofree char *netstr = NULL; int ret = -1; bool releaseaddr = false; @@ -1382,10 +1384,13 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, for (i = 0; i < vhostfdSize; i++) vhostfdName[i] = g_strdup_printf("vhostfd-%s%zu", net->info.alias, i); - if (!(netstr = qemuBuildHostNetStr(net, - tapfdName, tapfdSize, - vhostfdName, vhostfdSize, - slirpfdName))) + if (!(netprops = qemuBuildHostNetStr(net, + tapfdName, tapfdSize, + vhostfdName, vhostfdSize, + slirpfdName))) + goto cleanup; + + if (!(netstr = virQEMUBuildNetdevCommandlineFromJSON(netprops))) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); -- 2.26.2
On 5/15/20 10:28 AM, Peter Krempa wrote:
The 'netdev_add' command was recently formally described in qemu via the QMP schema. This means that it also requires the arguments to be properly formatted. Our current approach is to generate the command line and then use qemuMonitorJSONKeywordStringToJSON to get the JSON properties for the monitor. This will not work if we need to pass some fields as numbers or booleans.
In this step we re-do internals of qemuBuildHostNetStr to format a JSON object which is converted back via virQEMUBuildNetdevCommandlineFromJSON to the equivalent command line. This will later allow fixing of the monitor code to use the JSON object directly rather than rely on the conversion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 163 +++++++++++++++++++++++++--------------- src/qemu/qemu_command.h | 12 +-- src/qemu/qemu_hotplug.c | 13 +++- 3 files changed, 119 insertions(+), 69 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
QEMU models guestfwd as: 'guestfwd': [ { "str": "tcp:10.0.2.1:4600-chardev:charchannel0" }, { "str": "...."}, ] I guess the original idea was to make it extensible while not worrying about adding another object for it. Either way it requires us to add yet another JSON->cmdline convertor for arrays. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virqemu.c | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src/util/virqemu.c b/src/util/virqemu.c index 0f8cab29df..f3e9966598 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -109,6 +109,41 @@ virQEMUBuildCommandLineJSONArrayNumbered(const char *key, } +/** + * This array convertor is for quirky cases where the QMP schema mandates an + * array of objects with only one attribute 'str' which needs to be formatted as + * repeated key-value pairs without the 'str' being printed: + * + * 'guestfwd': [ + * { "str": "tcp:10.0.2.1:4600-chardev:charchannel0" }, + * { "str": "...."}, + * ] + */ +static int +virQEMUBuildCommandLineJSONArrayObjectsStr(const char *key, + virJSONValuePtr array, + virBufferPtr buf, + const char *skipKey G_GNUC_UNUSED, + bool onOff G_GNUC_UNUSED) +{ + g_auto(virBuffer) tmp = VIR_BUFFER_INITIALIZER; + size_t i; + + for (i = 0; i < virJSONValueArraySize(array); i++) { + virJSONValuePtr member = virJSONValueArrayGet(array, i); + const char *str = virJSONValueObjectGetString(member, "str"); + + if (!str) + return -1; + + virBufferAsprintf(&tmp, "%s=%s,", key, str); + } + + virBufferAddBuffer(buf, &tmp); + return 0; +} + + /* internal iterator to handle nested object formatting */ static int virQEMUBuildCommandLineJSONIterate(const char *key, @@ -267,7 +302,8 @@ virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props) virBufferAsprintf(&buf, "%s,", type); - if (virQEMUBuildCommandLineJSON(props, &buf, "type", true, NULL) < 0) + if (virQEMUBuildCommandLineJSON(props, &buf, "type", true, + virQEMUBuildCommandLineJSONArrayObjectsStr) < 0) return NULL; return virBufferContentAndReset(&buf); -- 2.26.2
On 5/15/20 10:28 AM, Peter Krempa wrote:
QEMU models guestfwd as:
'guestfwd': [ { "str": "tcp:10.0.2.1:4600-chardev:charchannel0" }, { "str": "...."}, ]
I guess the original idea was to make it extensible while not worrying about adding another object for it. Either way it requires us to add yet another JSON->cmdline convertor for arrays.
I would have found it easier to mention both the JSON (which you did) and the corresponding CLI (which you did not): -netdev ...,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,guestfwd=... But the idea makes sense - the QMP form uses quite a bit of nesting (although actual qapfication of netdev occurred late, the hand-rolled parser for netdev predated many of the best practices learned for other interfaces, and the CLI does not reflect that nesting). Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Syntax of guestfwd channel also needs to be modified to conform to the QAPI schema. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 37 +++++++++++++++++++++++++++++++------ src/qemu/qemu_command.h | 2 +- src/qemu/qemu_hotplug.c | 6 +++++- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 00d1b4121d..2ed8e66f66 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8567,6 +8567,7 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, for (i = 0; i < def->nchannels; i++) { virDomainChrDefPtr channel = def->channels[i]; g_autofree char *chardevstr = NULL; + g_autoptr(virJSONValue) netdevprops = NULL; g_autofree char *netdevstr = NULL; if (!(chardevstr = qemuBuildChrChardevStr(logManager, secManager, @@ -8581,8 +8582,12 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, switch ((virDomainChrChannelTargetType) channel->targetType) { case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: - if (!(netdevstr = qemuBuildChannelGuestfwdNetdevProps(channel))) + if (!(netdevprops = qemuBuildChannelGuestfwdNetdevProps(channel))) return -1; + + if (!(netdevstr = virQEMUBuildNetdevCommandlineFromJSON(netdevprops))) + return -1; + virCommandAddArgList(cmd, "-netdev", netdevstr, NULL); break; @@ -9850,19 +9855,39 @@ qemuBuildParallelChrDeviceStr(char **deviceStr, } -char * +virJSONValuePtr qemuBuildChannelGuestfwdNetdevProps(virDomainChrDefPtr chr) { + g_autoptr(virJSONValue) guestfwdarr = virJSONValueNewArray(); + g_autoptr(virJSONValue) guestfwdstrobj = virJSONValueNewObject(); g_autofree char *addr = NULL; - int port; + virJSONValuePtr ret = NULL; if (!(addr = virSocketAddrFormat(chr->target.addr))) return NULL; - port = virSocketAddrGetPort(chr->target.addr); + /* this may seem weird, but qemu indeed decided that 'guestfwd' parameter + * is an array of objects which have just one member named 'str' which + * contains the description */ + if (virJSONValueObjectAppendStringPrintf(guestfwdstrobj, "str", + "tcp:%s:%i-chardev:char%s", + addr, + virSocketAddrGetPort(chr->target.addr), + chr->info.alias) < 0) + return NULL; + + if (virJSONValueArrayAppend(guestfwdarr, guestfwdstrobj) < 0) + return NULL; + guestfwdstrobj = NULL; - return g_strdup_printf("user,guestfwd=tcp:%s:%i-chardev:char%s,id=%s", - addr, port, chr->info.alias, chr->info.alias); + if (virJSONValueObjectCreate(&ret, + "s:type", "user", + "a:guestfwd", &guestfwdarr, + "s:id", chr->info.alias, + NULL) < 0) + return NULL; + + return ret; } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 6cdd9debe0..1daa07982c 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -86,7 +86,7 @@ qemuBuildChrDeviceStr(char **deviceStr, virDomainChrDefPtr chr, virQEMUCapsPtr qemuCaps); -char * +virJSONValuePtr qemuBuildChannelGuestfwdNetdevProps(virDomainChrDefPtr chr); virJSONValuePtr qemuBuildHostNetStr(virDomainNetDefPtr net, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c6789dcef3..cd9bc9b2f8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2113,6 +2113,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virErrorPtr orig_err; virDomainDefPtr vmdef = vm->def; g_autofree char *devstr = NULL; + g_autoptr(virJSONValue) netdevprops = NULL; g_autofree char *netdevstr = NULL; virDomainChrSourceDefPtr dev = chr->source; g_autofree char *charAlias = NULL; @@ -2153,7 +2154,10 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, teardowncgroup = true; if (guestfwd) { - if (!(netdevstr = qemuBuildChannelGuestfwdNetdevProps(chr))) + if (!(netdevprops = qemuBuildChannelGuestfwdNetdevProps(chr))) + goto cleanup; + + if (!(netdevstr = virQEMUBuildNetdevCommandlineFromJSON(netdevprops))) goto cleanup; } else { if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) -- 2.26.2
On 5/15/20 10:28 AM, Peter Krempa wrote:
Syntax of guestfwd channel also needs to be modified to conform to the QAPI schema.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 37 +++++++++++++++++++++++++++++++------ src/qemu/qemu_command.h | 2 +- src/qemu/qemu_hotplug.c | 6 +++++- 3 files changed, 37 insertions(+), 8 deletions(-)
- port = virSocketAddrGetPort(chr->target.addr); + /* this may seem weird, but qemu indeed decided that 'guestfwd' parameter + * is an array of objects which have just one member named 'str' which + * contains the description */ + if (virJSONValueObjectAppendStringPrintf(guestfwdstrobj, "str", + "tcp:%s:%i-chardev:char%s", + addr, + virSocketAddrGetPort(chr->target.addr), + chr->info.alias) < 0) + return NULL;
If we were designing the netdev QMP from scratch, it would be an object with multiple components rather than a single string that has to be subsequently parsed. But in the meantime, yes, this looks like you are properly handling existing quirks correctly. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Now that all code paths generate JSON props we can remove the conversion to command line arguments and back in the monitor code. Note that the test which is removed in this commit will be replaced by a stronger testsuite later. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 14 +++----------- src/qemu/qemu_monitor.c | 8 ++++---- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 15 +++------------ src/qemu/qemu_monitor_json.h | 2 +- tests/qemumonitorjsontest.c | 2 -- 6 files changed, 12 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cd9bc9b2f8..2b0cdad4e7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1159,7 +1159,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, size_t queueSize = 0; g_autofree char *nicstr = NULL; g_autoptr(virJSONValue) netprops = NULL; - g_autofree char *netstr = NULL; int ret = -1; bool releaseaddr = false; bool iface_connected = false; @@ -1390,9 +1389,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, slirpfdName))) goto cleanup; - if (!(netstr = virQEMUBuildNetdevCommandlineFromJSON(netprops))) - goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { @@ -1404,7 +1400,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, charDevPlugged = true; } - if (qemuMonitorAddNetdev(priv->mon, netstr, + if (qemuMonitorAddNetdev(priv->mon, &netprops, tapfd, tapfdName, tapfdSize, vhostfd, vhostfdName, vhostfdSize, slirpfd, slirpfdName) < 0) { @@ -2114,7 +2110,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; g_autofree char *devstr = NULL; g_autoptr(virJSONValue) netdevprops = NULL; - g_autofree char *netdevstr = NULL; virDomainChrSourceDefPtr dev = chr->source; g_autofree char *charAlias = NULL; bool chardevAttached = false; @@ -2156,9 +2151,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (guestfwd) { if (!(netdevprops = qemuBuildChannelGuestfwdNetdevProps(chr))) goto cleanup; - - if (!(netdevstr = virQEMUBuildNetdevCommandlineFromJSON(netdevprops))) - goto cleanup; } else { if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup; @@ -2181,8 +2173,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto exit_monitor; chardevAttached = true; - if (netdevstr) { - if (qemuMonitorAddNetdev(priv->mon, netdevstr, + if (netdevprops) { + if (qemuMonitorAddNetdev(priv->mon, &netdevprops, NULL, NULL, 0, NULL, NULL, 0, -1, NULL) < 0) goto exit_monitor; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 9c853ccb93..2911307f0e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2667,7 +2667,7 @@ qemuMonitorCloseFileHandle(qemuMonitorPtr mon, int qemuMonitorAddNetdev(qemuMonitorPtr mon, - const char *netdevstr, + virJSONValuePtr *props, int *tapfd, char **tapfdName, int tapfdSize, int *vhostfd, char **vhostfdName, int vhostfdSize, int slirpfd, char *slirpfdName) @@ -2675,10 +2675,10 @@ qemuMonitorAddNetdev(qemuMonitorPtr mon, int ret = -1; size_t i = 0, j = 0; - VIR_DEBUG("netdevstr=%s tapfd=%p tapfdName=%p tapfdSize=%d" + VIR_DEBUG("props=%p tapfd=%p tapfdName=%p tapfdSize=%d" "vhostfd=%p vhostfdName=%p vhostfdSize=%d" "slirpfd=%d slirpfdName=%s", - netdevstr, tapfd, tapfdName, tapfdSize, + props, tapfd, tapfdName, tapfdSize, vhostfd, vhostfdName, vhostfdSize, slirpfd, slirpfdName); QEMU_CHECK_MONITOR(mon); @@ -2696,7 +2696,7 @@ qemuMonitorAddNetdev(qemuMonitorPtr mon, qemuMonitorSendFileHandle(mon, slirpfdName, slirpfd) < 0) goto cleanup; - ret = qemuMonitorJSONAddNetdev(mon, netdevstr); + ret = qemuMonitorJSONAddNetdev(mon, props); cleanup: if (ret < 0) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2e35d94bda..378e2532c7 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -881,7 +881,7 @@ int qemuMonitorCloseFileHandle(qemuMonitorPtr mon, const char *fdname); int qemuMonitorAddNetdev(qemuMonitorPtr mon, - const char *netdevstr, + virJSONValuePtr *props, int *tapfd, char **tapfdName, int tapfdSize, int *vhostfd, char **vhostfdName, int vhostfdSize, int slirpfd, char *slirpfdName); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 1224d1c9cc..a98479a55d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3986,23 +3986,14 @@ int qemuMonitorJSONCloseFileHandle(qemuMonitorPtr mon, int qemuMonitorJSONAddNetdev(qemuMonitorPtr mon, - const char *netdevstr) + virJSONValuePtr *props) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - g_autoptr(virJSONValue) args = NULL; - - cmd = qemuMonitorJSONMakeCommand("netdev_add", NULL); - if (!cmd) - return -1; - - args = qemuMonitorJSONKeywordStringToJSON(netdevstr, "type"); - if (!args) - return -1; + virJSONValuePtr pr = g_steal_pointer(props); - if (virJSONValueObjectAppend(cmd, "arguments", args) < 0) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("netdev_add", pr))) return -1; - args = NULL; /* obj owns reference to args now */ if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d3d46ec2f7..83c5e25ca5 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -210,7 +210,7 @@ int qemuMonitorJSONCloseFileHandle(qemuMonitorPtr mon, const char *fdname); int qemuMonitorJSONAddNetdev(qemuMonitorPtr mon, - const char *netdevstr); + virJSONValuePtr *props); int qemuMonitorJSONRemoveNetdev(qemuMonitorPtr mon, const char *alias); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index f58b18a110..a50b157dbc 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1315,7 +1315,6 @@ GEN_TEST_FUNC(qemuMonitorJSONDump, "dummy_protocol", "elf", true) GEN_TEST_FUNC(qemuMonitorJSONGraphicsRelocate, VIR_DOMAIN_GRAPHICS_TYPE_SPICE, "localhost", 12345, 12346, "certsubjectval") -GEN_TEST_FUNC(qemuMonitorJSONAddNetdev, "id=net0,type=user") GEN_TEST_FUNC(qemuMonitorJSONRemoveNetdev, "net0") GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0") GEN_TEST_FUNC(qemuMonitorJSONAddDevice, "some_dummy_devicestr") @@ -3226,7 +3225,6 @@ mymain(void) DO_TEST_GEN(qemuMonitorJSONMigrate); DO_TEST_GEN(qemuMonitorJSONDump); DO_TEST_GEN(qemuMonitorJSONGraphicsRelocate); - DO_TEST_GEN(qemuMonitorJSONAddNetdev); DO_TEST_GEN(qemuMonitorJSONRemoveNetdev); DO_TEST_GEN(qemuMonitorJSONDelDevice); DO_TEST_GEN(qemuMonitorJSONAddDevice); -- 2.26.2
On 5/15/20 10:28 AM, Peter Krempa wrote:
Now that all code paths generate JSON props we can remove the conversion to command line arguments and back in the monitor code.
Note that the test which is removed in this commit will be replaced by a stronger testsuite later.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
qemuxml2argv test suite is way more comprehensive than the hotplug suite. Since we share the code paths for monitor and command line hotplug we can easily test the properties of devices against the QAPI schema. To achieve this we'll need to skip the JSON->commandline conversion for the test run so that we can analyze the pure properties. This patch adds flags for the comand line generator and hook them into the JSON->commandline convertor for -netdev. An upcoming patch will make use of this new infrastructure. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 28 +++++++++++++++++----------- src/qemu/qemu_command.h | 7 ++++++- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 11 +++++++++-- src/qemu/qemu_process.h | 1 + src/util/virqemu.c | 9 ++++++++- src/util/virqemu.h | 3 ++- tests/qemuxml2argvtest.c | 6 ++++-- 8 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2ed8e66f66..f9a2732587 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7691,7 +7691,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, virNetDevVPortProfileOp vmop, bool standalone, size_t *nnicindexes, - int **nicindexes) + int **nicindexes, + unsigned int flags) { virDomainDefPtr def = vm->def; int ret = -1; @@ -7920,7 +7921,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, slirpfdName))) goto cleanup; - if (!(host = virQEMUBuildNetdevCommandlineFromJSON(hostnetprops))) + if (!(host = virQEMUBuildNetdevCommandlineFromJSON(hostnetprops, + (flags & QEMU_BUILD_COMMANDLINE_VALIDATE_KEEP_JSON)))) goto cleanup; virCommandAddArgList(cmd, "-netdev", host, NULL); @@ -7996,7 +7998,8 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver, bool standalone, size_t *nnicindexes, int **nicindexes, - unsigned int *bootHostdevNet) + unsigned int *bootHostdevNet, + unsigned int flags) { size_t i; int last_good_net = -1; @@ -8020,7 +8023,7 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver, if (qemuBuildInterfaceCommandLine(driver, vm, logManager, secManager, cmd, net, qemuCaps, bootNet, vmop, standalone, nnicindexes, - nicindexes) < 0) + nicindexes, flags) < 0) goto error; last_good_net = i; @@ -8556,7 +8559,8 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, virQEMUDriverConfigPtr cfg, const virDomainDef *def, virQEMUCapsPtr qemuCaps, - bool chardevStdioLogd) + bool chardevStdioLogd, + unsigned int flags) { size_t i; unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | @@ -8585,7 +8589,8 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, if (!(netdevprops = qemuBuildChannelGuestfwdNetdevProps(channel))) return -1; - if (!(netdevstr = virQEMUBuildNetdevCommandlineFromJSON(netdevprops))) + if (!(netdevstr = virQEMUBuildNetdevCommandlineFromJSON(netdevprops, + (flags & QEMU_BUILD_COMMANDLINE_VALIDATE_KEEP_JSON)))) return -1; virCommandAddArgList(cmd, "-netdev", netdevstr, NULL); @@ -9521,7 +9526,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, bool standalone, bool enableFips, size_t *nnicindexes, - int **nicindexes) + int **nicindexes, + unsigned int flags) { size_t i; char uuid[VIR_UUID_STRING_BUFLEN]; @@ -9534,9 +9540,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, bool chardevStdioLogd = priv->chardevStdioLogd; VIR_DEBUG("driver=%p def=%p mon=%p " - "qemuCaps=%p migrateURI=%s snapshot=%p vmop=%d", + "qemuCaps=%p migrateURI=%s snapshot=%p vmop=%d flags=0x%x", driver, def, priv->monConfig, - qemuCaps, migrateURI, snapshot, vmop); + qemuCaps, migrateURI, snapshot, vmop, flags); if (qemuBuildCommandLineValidate(driver, def) < 0) return NULL; @@ -9681,7 +9687,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildNetCommandLine(driver, vm, logManager, secManager, cmd, qemuCaps, vmop, standalone, - nnicindexes, nicindexes, &bootHostdevNet) < 0) + nnicindexes, nicindexes, &bootHostdevNet, flags) < 0) return NULL; if (qemuBuildSmartcardCommandLine(logManager, secManager, cmd, cfg, def, qemuCaps, @@ -9697,7 +9703,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, return NULL; if (qemuBuildChannelsCommandLine(logManager, secManager, cmd, cfg, def, qemuCaps, - chardevStdioLogd) < 0) + chardevStdioLogd, flags) < 0) return NULL; if (qemuBuildConsoleCommandLine(logManager, secManager, cmd, cfg, def, qemuCaps, diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1daa07982c..55bc67072a 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -43,6 +43,10 @@ VIR_ENUM_DECL(qemuVideo); VIR_ENUM_DECL(qemuSoundCodec); +typedef enum { + QEMU_BUILD_COMMANDLINE_VALIDATE_KEEP_JSON = 1 << 0, +} qemuBuildCommandLineFlags; + virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, virLogManagerPtr logManager, virSecurityManagerPtr secManager, @@ -53,7 +57,8 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, bool standalone, bool enableFips, size_t *nnicindexes, - int **nicindexes); + int **nicindexes, + unsigned int flags); /* Generate the object properties for pr-manager */ virJSONValuePtr qemuBuildPRManagerInfoProps(virStorageSourcePtr src); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index db65862163..dd9ae30bb5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7423,7 +7423,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, } if (!(cmd = qemuProcessCreatePretendCmd(driver, vm, NULL, - qemuCheckFips(), true, + qemuCheckFips(), true, false, VIR_QEMU_PROCESS_START_COLD))) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f7f6793113..ab32d49d6e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6746,7 +6746,7 @@ qemuProcessLaunch(virConnectPtr conn, snapshot, vmop, false, qemuCheckFips(), - &nnicindexes, &nicindexes))) + &nnicindexes, &nicindexes, 0))) goto cleanup; if (incoming && incoming->fd != -1) @@ -7189,8 +7189,11 @@ qemuProcessCreatePretendCmd(virQEMUDriverPtr driver, const char *migrateURI, bool enableFips, bool standalone, + bool jsonPropsValidation, unsigned int flags) { + unsigned int buildflags = 0; + virCheckFlags(VIR_QEMU_PROCESS_START_COLD | VIR_QEMU_PROCESS_START_PAUSED | VIR_QEMU_PROCESS_START_AUTODESTROY, NULL); @@ -7200,6 +7203,9 @@ qemuProcessCreatePretendCmd(virQEMUDriverPtr driver, if (standalone) flags |= VIR_QEMU_PROCESS_START_STANDALONE; + if (jsonPropsValidation) + buildflags = QEMU_BUILD_COMMANDLINE_VALIDATE_KEEP_JSON; + if (qemuProcessInit(driver, vm, NULL, QEMU_ASYNC_JOB_NONE, !!migrateURI, flags) < 0) return NULL; @@ -7218,7 +7224,8 @@ qemuProcessCreatePretendCmd(virQEMUDriverPtr driver, standalone, enableFips, NULL, - NULL); + NULL, + buildflags); } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 3077d3ef9e..15e67b9762 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -101,6 +101,7 @@ virCommandPtr qemuProcessCreatePretendCmd(virQEMUDriverPtr driver, const char *migrateURI, bool enableFips, bool standalone, + bool jsonPropsValidation, unsigned int flags); int qemuProcessInit(virQEMUDriverPtr driver, diff --git a/src/util/virqemu.c b/src/util/virqemu.c index f3e9966598..cf76346bfa 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -290,16 +290,23 @@ virQEMUBuildCommandLineJSON(virJSONValuePtr value, /** * virQEMUBuildNetdevCommandlineFromJSON: * @props: JSON properties describing a netdev + * @rawjson: don't transform to commandline args, but just stringify json * * Converts @props into arguments for -netdev including all the quirks and * differences between the monitor and command line syntax. + * + * @rawjson is meant for testing of the schema in the xml2argvtest */ char * -virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props) +virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props, + bool rawjson) { const char *type = virJSONValueObjectGetString(props, "type"); g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + if (rawjson) + return virJSONValueToString(props, false); + virBufferAsprintf(&buf, "%s,", type); if (virQEMUBuildCommandLineJSON(props, &buf, "type", true, diff --git a/src/util/virqemu.h b/src/util/virqemu.h index 22f47851df..b1296cb657 100644 --- a/src/util/virqemu.h +++ b/src/util/virqemu.h @@ -50,7 +50,8 @@ int virQEMUBuildCommandLineJSON(virJSONValuePtr value, virQEMUBuildCommandLineJSONArrayFormatFunc array); char * -virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props); +virQEMUBuildNetdevCommandlineFromJSON(virJSONValuePtr props, + bool rawjson); int virQEMUBuildObjectCommandlineFromJSON(virBufferPtr buf, virJSONValuePtr objprops); diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9fa1afd9d0..5123580ee2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -396,7 +396,8 @@ testCompareXMLToArgvCreateArgs(virQEMUDriverPtr drv, virDomainObjPtr vm, const char *migrateURI, struct testQemuInfo *info, - unsigned int flags) + unsigned int flags, + bool jsonPropsValidation) { size_t i; @@ -471,6 +472,7 @@ testCompareXMLToArgvCreateArgs(virQEMUDriverPtr drv, return qemuProcessCreatePretendCmd(drv, vm, migrateURI, (flags & FLAG_FIPS), false, + jsonPropsValidation, VIR_QEMU_PROCESS_START_COLD); } @@ -566,7 +568,7 @@ testCompareXMLToArgv(const void *data) virResetLastError(); if (!(cmd = testCompareXMLToArgvCreateArgs(&driver, vm, migrateURI, info, - flags))) { + flags, false))) { if (flags & FLAG_EXPECT_FAILURE) goto ok; goto cleanup; -- 2.26.2
On 5/15/20 10:28 AM, Peter Krempa wrote:
qemuxml2argv test suite is way more comprehensive than the hotplug suite. Since we share the code paths for monitor and command line hotplug we can easily test the properties of devices against the QAPI schema.
To achieve this we'll need to skip the JSON->commandline conversion for the test run so that we can analyze the pure properties. This patch adds flags for the comand line generator and hook them into the JSON->commandline convertor for -netdev. An upcoming patch will make use of this new infrastructure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Reviewed-by: Eric Blake <eblake@redhat.com>
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
It always loads the latest schema. Prepare for loading others as well. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 2 +- tests/qemuhotplugtest.c | 2 +- tests/qemumonitorjsontest.c | 4 ++-- tests/testutilsqemuschema.c | 2 +- tests/testutilsqemuschema.h | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 8057d1e2c0..0cdedb9ad4 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -1056,7 +1056,7 @@ mymain(void) diskxmljsondata.qemuCaps = caps_x86_64; imagecreatedata.qemuCaps = caps_x86_64; - if (!(qmp_schema_x86_64 = testQEMUSchemaLoad("x86_64"))) { + if (!(qmp_schema_x86_64 = testQEMUSchemaLoadLatest("x86_64"))) { ret = -1; goto cleanup; } diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 1843e79d77..ba3fc4d814 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -636,7 +636,7 @@ mymain(void) if (!(driver.domainEventState = virObjectEventStateNew())) return EXIT_FAILURE; - if (!(qmpschema = testQEMUSchemaLoad("x86_64"))) { + if (!(qmpschema = testQEMUSchemaLoadLatest("x86_64"))) { VIR_TEST_VERBOSE("failed to load qapi schema\n"); return EXIT_FAILURE; } diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index a50b157dbc..6d5a1d5fe2 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -3124,7 +3124,7 @@ mymain(void) virEventRegisterDefaultImpl(); - if (!(qapiData.schema = testQEMUSchemaLoad("x86_64"))) { + if (!(qapiData.schema = testQEMUSchemaLoadLatest("x86_64"))) { VIR_TEST_VERBOSE("failed to load qapi schema"); ret = -1; goto cleanup; @@ -3394,7 +3394,7 @@ mymain(void) #undef DO_TEST_QUERY_JOBS virHashFree(qapiData.schema); - if (!(qapiData.schema = testQEMUSchemaLoad("s390x"))) { + if (!(qapiData.schema = testQEMUSchemaLoadLatest("s390x"))) { VIR_TEST_VERBOSE("failed to load qapi schema for s390x"); ret = -1; goto cleanup; diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index 898be68b0a..060a5d7054 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -661,7 +661,7 @@ testQEMUSchemaGetLatest(const char* arch) virHashTablePtr -testQEMUSchemaLoad(const char* arch) +testQEMUSchemaLoadLatest(const char *arch) { virJSONValuePtr schema; diff --git a/tests/testutilsqemuschema.h b/tests/testutilsqemuschema.h index 8b6803afda..acedb77677 100644 --- a/tests/testutilsqemuschema.h +++ b/tests/testutilsqemuschema.h @@ -41,4 +41,4 @@ virJSONValuePtr testQEMUSchemaGetLatest(const char* arch); virHashTablePtr -testQEMUSchemaLoad(const char* arch); +testQEMUSchemaLoadLatest(const char *arch); -- 2.26.2
On 5/15/20 10:28 AM, Peter Krempa wrote:
It always loads the latest schema. Prepare for loading others as well.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 2 +- tests/qemuhotplugtest.c | 2 +- tests/qemumonitorjsontest.c | 4 ++-- tests/testutilsqemuschema.c | 2 +- tests/testutilsqemuschema.h | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Add testQEMUSchemaLoad and refactor internals so that we can load the QMP schema from an arbitrary caps replies file. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 64 ++++++++++++++++++++++++------------- tests/testutilsqemuschema.h | 3 ++ 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index 060a5d7054..d9b55fd787 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -607,37 +607,23 @@ testQEMUSchemaValidateCommand(const char *command, } -/** - * testQEMUSchemaGetLatest: - * - * Returns the schema data as the qemu monitor would reply from the latest - * replies file used for qemucapabilitiestest for the x86_64 architecture. - */ -virJSONValuePtr -testQEMUSchemaGetLatest(const char* arch) +static virJSONValuePtr +testQEMUSchemaLoadReplies(const char *filename) { - g_autofree char *capsLatestFile = NULL; - g_autofree char *capsLatest = NULL; + g_autofree char *caps = NULL; char *schemaReply; char *end; g_autoptr(virJSONValue) reply = NULL; virJSONValuePtr schema = NULL; - if (!(capsLatestFile = testQemuGetLatestCapsForArch(arch, "replies"))) { - VIR_TEST_VERBOSE("failed to find latest caps replies"); + if (virTestLoadFile(filename, &caps) < 0) return NULL; - } - VIR_TEST_DEBUG("replies file: '%s'", capsLatestFile); - - if (virTestLoadFile(capsLatestFile, &capsLatest) < 0) - return NULL; - - if (!(schemaReply = strstr(capsLatest, "\"execute\": \"query-qmp-schema\"")) || + if (!(schemaReply = strstr(caps, "\"execute\": \"query-qmp-schema\"")) || !(schemaReply = strstr(schemaReply, "\n\n")) || !(end = strstr(schemaReply + 2, "\n\n"))) { VIR_TEST_VERBOSE("failed to find reply to 'query-qmp-schema' in '%s'", - capsLatestFile); + filename); return NULL; } @@ -646,13 +632,13 @@ testQEMUSchemaGetLatest(const char* arch) if (!(reply = virJSONValueFromString(schemaReply))) { VIR_TEST_VERBOSE("failed to parse 'query-qmp-schema' reply from '%s'", - capsLatestFile); + filename); return NULL; } if (!(schema = virJSONValueObjectStealArray(reply, "return"))) { VIR_TEST_VERBOSE("missing qapi schema data in reply in '%s'", - capsLatestFile); + filename); return NULL; } @@ -660,6 +646,28 @@ testQEMUSchemaGetLatest(const char* arch) } +/** + * testQEMUSchemaGetLatest: + * + * Returns the schema data as the qemu monitor would reply from the latest + * replies file used for qemucapabilitiestest for the x86_64 architecture. + */ +virJSONValuePtr +testQEMUSchemaGetLatest(const char *arch) +{ + g_autofree char *capsLatestFile = NULL; + + if (!(capsLatestFile = testQemuGetLatestCapsForArch(arch, "replies"))) { + VIR_TEST_VERBOSE("failed to find latest caps replies"); + return NULL; + } + + VIR_TEST_DEBUG("replies file: '%s'", capsLatestFile); + + return testQEMUSchemaLoadReplies(capsLatestFile); +} + + virHashTablePtr testQEMUSchemaLoadLatest(const char *arch) { @@ -670,3 +678,15 @@ testQEMUSchemaLoadLatest(const char *arch) return virQEMUQAPISchemaConvert(schema); } + + +virHashTablePtr +testQEMUSchemaLoad(const char *filename) +{ + virJSONValuePtr schema; + + if (!(schema = testQEMUSchemaLoadReplies(filename))) + return NULL; + + return virQEMUQAPISchemaConvert(schema); +} diff --git a/tests/testutilsqemuschema.h b/tests/testutilsqemuschema.h index acedb77677..c90a6b626d 100644 --- a/tests/testutilsqemuschema.h +++ b/tests/testutilsqemuschema.h @@ -42,3 +42,6 @@ testQEMUSchemaGetLatest(const char* arch); virHashTablePtr testQEMUSchemaLoadLatest(const char *arch); + +virHashTablePtr +testQEMUSchemaLoad(const char *filename); -- 2.26.2
On 5/15/20 10:28 AM, Peter Krempa wrote:
Add testQEMUSchemaLoad and refactor internals so that we can load the QMP schema from an arbitrary caps replies file.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 64 ++++++++++++++++++++++++------------- tests/testutilsqemuschema.h | 3 ++ 2 files changed, 45 insertions(+), 22 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Our hotplug test cases are weak in comparison to the xml2arvtest. Use all the the data to also valiadte the arguments for -netdev and -blockdev against the appropriate commands of the QMP schema. Note that currently it's done just for the _CAPS versions of tests but commenting out a line in the test file allows to validate even cases which don't use real capabilities. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/Makefile.am | 2 +- tests/qemuxml2argvtest.c | 76 ++++++++++++++++++++++++++++++++++++++++ tests/testutilsqemu.c | 5 +++ tests/testutilsqemu.h | 1 + 4 files changed, 83 insertions(+), 1 deletion(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index fc516376b4..f5766a7790 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -570,7 +570,7 @@ qemuxml2argvtest_SOURCES = \ testutils.c testutils.h \ virfilewrapper.c virfilewrapper.h \ $(NULL) -qemuxml2argvtest_LDADD = libqemutestdriver.la \ +qemuxml2argvtest_LDADD = libqemutestdriver.la libqemumonitortestutils.la \ $(LDADDS) $(LIBXML_LIBS) libqemuxml2argvmock_la_SOURCES = \ diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 5123580ee2..4f613e8f1a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -18,6 +18,7 @@ # include "qemu/qemu_migration.h" # include "qemu/qemu_process.h" # include "qemu/qemu_slirp.h" +# include "qemu/qemu_qapi.h" # include "datatypes.h" # include "conf/storage_conf.h" # include "cpu/cpu_map.h" @@ -26,6 +27,8 @@ # include "virmock.h" # include "virfilewrapper.h" # include "configmake.h" +# include "testutilsqemuschema.h" +# include "qemu/qemu_monitor_json.h" # define LIBVIRT_QEMU_CAPSPRIV_H_ALLOW # include "qemu/qemu_capspriv.h" @@ -477,6 +480,76 @@ testCompareXMLToArgvCreateArgs(virQEMUDriverPtr drv, } +static int +testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv, + virDomainObjPtr vm, + const char *migrateURI, + struct testQemuInfo *info, + unsigned int flags) +{ + VIR_AUTOSTRINGLIST args = NULL; + size_t nargs = 0; + size_t i; + g_autoptr(virHashTable) schema = NULL; + g_autoptr(virCommand) cmd = NULL; + + if (info->schemafile) + schema = testQEMUSchemaLoad(info->schemafile); + + /* comment out with line comment to enable schema checking for non _CAPS tests + if (!schema) + schema = testQEMUSchemaLoadLatest(virArchToString(info->arch)); + // */ + + if (!schema) + return 0; + + if (!(cmd = testCompareXMLToArgvCreateArgs(drv, vm, migrateURI, info, flags, + true))) + return -1; + + if (virCommandGetArgList(cmd, &args, &nargs) < 0) + return -1; + + for (i = 0; i < nargs; i++) { + g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER; + g_autoptr(virJSONValue) jsonargs = NULL; + + if (STREQ(args[i], "-blockdev")) { + if (!(jsonargs = virJSONValueFromString(args[i + 1]))) + return -1; + + if (testQEMUSchemaValidateCommand("blockdev-add", jsonargs, + schema, false, false, &debug) < 0) { + VIR_TEST_VERBOSE("failed to validate -blockdev '%s' against QAPI schema: %s", + args[i + 1], virBufferCurrentContent(&debug)); + return -1; + } + + i++; + } else if (STREQ(args[i], "-netdev")) { + if (!(jsonargs = virJSONValueFromString(args[i + 1]))) + return -1; + + /* skip the validation for pre-QAPIfication cases */ + if (virQEMUQAPISchemaPathExists("netdev_add/arg-type/type/!string", schema)) + continue; + + if (testQEMUSchemaValidateCommand("netdev_add", jsonargs, + schema, false, false, &debug) < 0) { + VIR_TEST_VERBOSE("failed to validate -netdev '%s' against QAPI schema: %s", + args[i + 1], virBufferCurrentContent(&debug)); + return -1; + } + + i++; + } + } + + return 0; +} + + static int testCompareXMLToArgv(const void *data) { @@ -578,6 +651,9 @@ testCompareXMLToArgv(const void *data) goto cleanup; } + if (testCompareXMLToArgvValidateSchema(&driver, vm, migrateURI, info, flags) < 0) + goto cleanup; + if (!(actualargv = virCommandToString(cmd, false))) goto cleanup; diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 9f9eb4033c..4b83b8ff25 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -768,6 +768,10 @@ testQemuInfoSetArgs(struct testQemuInfo *info, if (stripmachinealiases) virQEMUCapsStripMachineAliases(qemuCaps); info->flags |= FLAG_REAL_CAPS; + + /* provide path to the replies file for schema testing */ + capsfile[strlen(capsfile) - 3] = '\0'; + info->schemafile = g_strdup_printf("%sreplies", capsfile); } if (!qemuCaps) { @@ -794,5 +798,6 @@ testQemuInfoClear(struct testQemuInfo *info) { VIR_FREE(info->infile); VIR_FREE(info->outfile); + VIR_FREE(info->schemafile); virObjectUnref(info->qemuCaps); } diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index edee6e450c..e7c5032012 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -64,6 +64,7 @@ struct testQemuInfo { unsigned int flags; unsigned int parseFlags; virArch arch; + char *schemafile; }; virCapsPtr testQemuCapsInit(void); -- 2.26.2
On 5/15/20 10:28 AM, Peter Krempa wrote:
Our hotplug test cases are weak in comparison to the xml2arvtest. Use
xml2argvtest
all the the data to also valiadte the arguments for -netdev and
validate
-blockdev against the appropriate commands of the QMP schema.
Note that currently it's done just for the _CAPS versions of tests but commenting out a line in the test file allows to validate even cases which don't use real capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa