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@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;