Re: [PATCH 2/2 for 8.0] qemu: add index for isa-serial device using target.port

On 1/13/22 08:33, Divya Garg wrote: > VM XML accepts target.port but this does not get passed while building the qemu > command line for this VM. > Signed-off-by: Divya Garg <divya.garg(a)nutanix.com> --- > src/qemu/qemu_command.c | 25 +++++++++++++++---- > tests/qemuxml2argvdata/bios.args | 2 +- > .../qemuxml2argvdata/console-compat-auto.args | 2 +- > .../console-compat-auto.x86_64-latest.args | 2 +- > .../console-compat-chardev.args | 2 +- > .../console-compat-chardev.x86_64-latest.args | 2 +- > tests/qemuxml2argvdata/console-compat.args | 2 +- > .../console-compat.x86_64-latest.args | 2 +- > .../qemuxml2argvdata/console-virtio-many.args | 2 +- > tests/qemuxml2argvdata/controller-order.args | 2 +- > .../name-escape.x86_64-2.11.0.args | 4 +-- > .../name-escape.x86_64-latest.args | 4 +-- > .../q35-virt-manager-basic.args | 2 +- > .../serial-dev-chardev-iobase.args | 2 +- > ...rial-dev-chardev-iobase.x86_64-latest.args | 2 +- > .../qemuxml2argvdata/serial-dev-chardev.args | 2 +- > .../serial-dev-chardev.x86_64-latest.args | 2 +- > .../qemuxml2argvdata/serial-file-chardev.args | 2 +- > .../serial-file-chardev.x86_64-latest.args | 2 +- > tests/qemuxml2argvdata/serial-file-log.args | 2 +- > .../serial-file-log.x86_64-latest.args | 2 +- > .../qemuxml2argvdata/serial-many-chardev.args | 4 +-- > .../serial-many-chardev.x86_64-latest.args | 4 +-- > .../qemuxml2argvdata/serial-pty-chardev.args | 2 +- > .../serial-pty-chardev.x86_64-latest.args | 2 +- > tests/qemuxml2argvdata/serial-spiceport.args | 2 +- > .../serial-spiceport.x86_64-latest.args | 2 +- > .../qemuxml2argvdata/serial-tcp-chardev.args | 2 +- > .../serial-tcp-chardev.x86_64-latest.args | 2 +- > .../serial-tcp-telnet-chardev.args | 2 +- > ...rial-tcp-telnet-chardev.x86_64-latest.args | 2 +- > .../serial-tcp-tlsx509-chardev-notls.args | 4 +-- > ...p-tlsx509-chardev-notls.x86_64-latest.args | 4 +-- > .../serial-tcp-tlsx509-chardev-verify.args | 4 +-- > ...-tlsx509-chardev-verify.x86_64-latest.args | 4 +-- > .../serial-tcp-tlsx509-chardev.args | 4 +-- > ...ial-tcp-tlsx509-chardev.x86_64-latest.args | 4 +-- > .../serial-tcp-tlsx509-secret-chardev.args | 4 +-- > ...-tlsx509-secret-chardev.x86_64-latest.args | 4 +-- > .../qemuxml2argvdata/serial-udp-chardev.args | 4 +-- > .../serial-udp-chardev.x86_64-latest.args | 4 +-- > .../qemuxml2argvdata/serial-unix-chardev.args | 4 +-- > .../serial-unix-chardev.x86_64-latest.args | 4 +-- > tests/qemuxml2argvdata/serial-vc-chardev.args | 2 +- > .../serial-vc-chardev.x86_64-latest.args | 2 +- > tests/qemuxml2argvdata/user-aliases.args | 4 +-- > .../virtio-9p-createmode.x86_64-latest.args | 2 +- > .../virtio-9p-multidevs.x86_64-latest.args | 2 +- > .../x86_64-pc-graphics.x86_64-latest.args | 2 +- > .../x86_64-pc-headless.x86_64-latest.args | 2 +- > .../x86_64-q35-graphics.x86_64-latest.args | 2 +- > .../x86_64-q35-headless.x86_64-latest.args | 2 +- > 52 files changed, 88 insertions(+), 73 deletions(-) > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index d822533ccb..4130df0ed9 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -10718,6 +10718,8 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def, > g_autoptr(virJSONValue) props = NULL; > g_autofree char *chardev = g_strdup_printf("char%s", serial->info.alias); > virQEMUCapsFlags caps; > + const char *typestr; > + int ret; This is a bit confusing. Usually, we use @ret for keeping return value from the function its defined in. In this specific case, @ret would hold the retval of qemuBuildSerialChrDeviceProps() and thus would have to be type of virJSONValue*. If you want to keep an intermediary retval of a function called from within we use other names, like rc, rv, tmp.

But fortunatelly, we can do without @ret and even without @typestr.

> > switch ((virDomainChrSerialTargetModel) serial->targetModel) { > case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL: > @@ -10750,11 +10752,24 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def, > return NULL; > } > > - if (virJSONValueObjectAdd(&props, > - "s:driver", virDomainChrSerialTargetModelTypeToString(serial->targetModel), > - "s:chardev", chardev, > - "s:id", serial->info.alias, > - NULL) < 0) There's no need to remove this code, especially when you're replacing it with itself. The pattern that we can use here is: if (virJSONValueObjectAdd(&props, ...) < 0) /* this is the unchanged return NULL; call */ if (serial->targetModel == ISA_SERIAL && virJSONValueObjectAdd(&props, "k:index", serial->target.port, NULL) < 0) return NULL; Michal