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

[Michal Prívozník]

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