[libvirt] [PATCH v2] qemu: Remove duplicated code in qemuBuildSerialChrDeviceStr()

[Laine Stump]

On 06/22/2017 06:08 AM, Andrea Bolognani wrote:
> The call to qemuBuildDeviceAddressStr() happens no matter
> what, so we can move it to the outer possible scope inside
> the function.
> 
> We can also move the call to virBufferAsprintf() after all
> the checks have been performed, where it makes more sense.
> 
> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> ---
>  src/qemu/qemu_command.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c53ab97..5118541 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10292,14 +10292,8 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
>              serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
>              virBufferAsprintf(&cmd, "spapr-vty,chardev=char%s",
>                                serial->info.alias);
> -            if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0)
> -                goto error;
>          }
>      } else {
> -        virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
> -                          virDomainChrSerialTargetTypeToString(serial->targetType),
> -                          serial->info.alias, serial->info.alias);
> -
>          switch (serial->targetType) {
>          case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
>              if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_SERIAL)) {
> @@ -10314,9 +10308,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
>                                 _("usb-serial requires address of usb type"));
>                  goto error;
>              }
> -
> -            if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0)
> -                goto error;
>              break;
>  
>          case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
> @@ -10326,9 +10317,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
>                                 _("isa-serial requires address of isa type"));
>                  goto error;
>              }
> -
> -            if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0)
> -                goto error;
>              break;
>  
>          case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
> @@ -10344,13 +10332,17 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
>                                 _("pci-serial requires address of pci type"));
>                  goto error;
>              }
> -
> -            if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0)
> -                goto error;
>              break;
>          }
> +
> +        virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
> +                          virDomainChrSerialTargetTypeToString(serial->targetType),
> +                          serial->info.alias, serial->info.alias);
>      }
>  
> +    if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0)
> +        goto error;
> +

I haven't looked at the caller of qemuBuildSerialChrDeviceStr() with a
fine-toothed comb, but this patch does change the code a bit. In
particular, previously if qemuDomainIsPSeries(def) was true, but either
one of these was false:

     serial->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL
     serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO

then it would return an empty string in *deviceStr. But now it will
instead return an address string, without the
"spapr-vty,chardev=charBLAH" at the beginning. So can we guarantee that
the above two conditions are always true for PSeries?

I guess in both the "before" and "after" cases, the result would be a
failure (with before, we would end up with a "-device" with nothing
following it, and now we would have "-device" with an address string but
none of the preceding stuff describing the address).

(Similarly, before this patch, if serial->targetType wasn't one of
VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE(USB|ISA|PCI) then *deviceStr would
contain "(null),chardev=charBLAH,id=BLAH" with no address, but now it
will have an address as well as the nonsensical preamble. (yes, I'm
being ridiculous in this case :-P))

In the end, ACK to your patch  since you haven't made the behavior any
worse (and have eliminated a bunch of duplicated code, but I do think
that either the checks on deviceType and info.type should be removed as
pointless, or that they should trigger a failure directly rather than
just creating a bad commandline.

>      if (virBufferCheckError(&cmd) < 0)
>          goto error;
>  
>