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@xxxxxxxxxx>
> ---
> 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;
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
