On Wed, 2019-02-13 at 13:29 +0100, Ján Tomko wrote: > On Wed, Feb 13, 2019 at 10:59:28AM +0100, Andrea Bolognani wrote: > > On Wed, 2019-02-13 at 10:03 +0100, Ján Tomko wrote: > > > Also, the device will be given an iobase by QEMU, we should represent > > > that in the XML and fill in that default. > > > > If we can manage to implement it in a way that's both reliable and > > backwards compatible, then I'm absolutely in favor of doing that! The > > current behavior is clearly not great. > > We cannot if any proposal for improvement is met with "it's already > ugly so we might keep doing it that way" I enthusiastically backed a proposal for improvement literally in the paragraph you're replying to :) > > <controller type='pci' model='pcie-root-port'> > > <model name='pcie-root-port'/> > > </controller> > > While having a model attribute and a model element is ugly, despite > exaclty matching QEMU's device, this is just a different way of saying > a generic device (e.g. isa-serial in my example above) There are no "generic" devices, only hypervisor-specific devices that present a similar interface to the guest but are completely separate and not interchangeable from the hypervisor's point of view. For controllers, we could have decided to use "intel-pcie-root-port" instead of "ioh3420" and then translate back and forth, but I guess at the time it was considered to be not worth doing, or perhaps the lack of redundancy resulted in the issue not being raised at all. > > Either way, my point is that while the current serial device XML is a > > bit redundant, at least it's mostly consistent > > consistent meaning it matches the QEMU devices? Yes, thus matching how controllers (and possibly other devices?) work. When I introduced the <model> element for serial devices, that's what I based the idea on, so it makes sense to me that we'll keep following the same semantics. > BTW if you look at the title of this thread, it says 'debugcon-isa', while > the QEMU device is named 'isa-debugcon'. Clearly an oversight: if you look through the patches, you'll see that there is no 'debucon-isa' anywhere in the code. > > and its implementation > > is very simple; > > Ah, the beauty of using virDomainChrSerialTargetModelTypeToString > instead of qemuDomainChrSerialTargetModelTypeToString What about the beauty of not having to implement qemuDomainChrSerialTargetModelTypeToString() in the first place? :) > > what you suggest doing would compromise both of those > > positive facts without allowing us to remove any redundancy from the > > existing scenarios, so I think it would overall represent a net > > negative. > > It allows us not to introduce more redundancy. I don't believe avoiding those four extra bytes is worth the extra code complexity and deviating from well-established semantics. -- Andrea Bolognani / Red Hat / Virtualization
