Re: [libvirt] [PATCH v2] qemu: Remove duplicated code in qemuBuildSerialChrDeviceStr()
On Thu, 2017-06-22 at 16:03 -0400, Laine Stump 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(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 22 +++++++---------------
> 1 file changed, 7 insertions(+), 15 deletions(-)
Picking up old stuff...
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?
The check on serial->deviceType is pointless, because we're
only calling this function for TYPE_SERIAL. I'm pretty sure
the latter can't be false because...
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).
... otherwise we would have run into this. But I'll check.
(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))
The only other case would be TYPE_LAST, which hopefully
will never happen. I can add a check for it though.
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.
I think we can go further and unify the way things are
handled so that pSeries doesn't need to be special-cased.
I've pushed the patch as it is for the moment, based on
your ACK, and will post a follow-up shortly.
--
Andrea Bolognani / Red Hat / Virtualization