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

[Pavel Hrdina]

On Thu, Nov 16, 2017 at 03:44:57PM +0100, Andrea Bolognani wrote:
> 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.

Right, you are folding the if-else into an existing switch.

> 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'.

Well, it wasn't that clear to me, obviously, even if I used '-w'.
Now I can see that the folding isn't possible without introducing
the new spapr type.  So ignore this comment :).

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

I believe that it was like you are describing :).

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

That's not an issue that we would not pass it directly to QEMU,
and sometimes it's even better to abstract it a little bit.

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

I would like to make it right and not to use names that will backfire
later.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171116/c3e9ec7c/attachment-0001.sig>