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