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

[Andrea Bolognani]

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