Re: [libvirt] [PATCH 07/14] conf: Add VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR

On Thu, 2017-11-16 at 14:22 +0100, Pavel Hrdina wrote: > On Wed, Nov 15, 2017 at 12:50:10PM +0100, Andrea Bolognani wrote: > > We can finally introduce a specific target type for the spapr-vty > > device used by pSeries guests, which means isa-serial will no longer > > show up to confuse users. > > > > We make sure migration works in both directions by interpreting the > > isa-serial target type, or the lack of target type, appropriately > > when parsing the guest XML, and skipping the newly-introduced type > > when formatting if for migration. We also verify that spapr-vty is > > not used for non-pSeries guests and add a bunch of test cases. > > > > This commit is best viewed with 'git diff -w'. > > It would be probably good idea to split it into two patches, one > that changes all the if-else conditions to switch and second where > the actual new code is introduced. I'm not changing any if-else to switch, I'm just folding a special case that was outside of the existing all-encompassing switch back into it and getting rid of the if-else that only existed to support that special case at the same time.

I can't really think of a good way to split the change right now, plus I think what's happening is pretty clear if you use '-w'.

> > @@ -3585,6 +3585,7 @@ > > <value>isa-serial</value> > > <value>usb-serial</value> > > <value>pci-serial</value> > > + <value>spapr-vty</value> > > This name doesn't feel right. Previous names are based on the BUS that > the serial device is connected with "-serial" appended to the BUS name. > Since sPAPR is specification that defines a set of para-virtualized > devices, there is no actual BUS, but I think that in this case we can > consider spapr as a BUS name and use "spapr-serial". It would be better > than just copying the device name from QEMU. I'm not sure that's how it went: it looks to me like all the *-serial names have been adopted merely because that's what the QEMU folks had chosen for the corresponding device.

We could abstract this further but that would mean adding another layer of translation, since at the moment we're basically passing it through to QEMU and that would no longer fly.

I'm not opposed to doing that on principle, but both for pSeries and for other non-x86 architecture, as you noted in response to other commits, obvious candidates for the name don't necessarily exist in the same way they do for ISA, USB and PCI. So I wonder whether it would be worth adding more machinery when the proposed names, while maybe not pretty, do not cause any ambiguity and can be matched to the corresponding platform just as easily.