Re: [libvirt] [PATCHv2 05/17] conf: add new <model> subelement with type attribute to <controller>

On 07/24/2015 07:28 AM, Martin Kletzander wrote: > On Thu, Jul 23, 2015 at 03:18:04PM +0200, Ján Tomko wrote: >> On Fri, Jul 17, 2015 at 02:43:32PM -0400, Laine Stump wrote: >>> This new subelement is used in PCI controllers: the toplevel >>> *attribute* "model" of a controller denotes what kind of PCI >>> controller is being described, e.g. a "dmi-to-pci-bridge", >>> "pci-bridge", or "pci-root". But in the future there will be different >>> implementations of some of those types of PCI controllers, which >>> behave similarly from libvirt's point of view (and so should have the >>> same model), but use a different device in qemu (and present >>> themselves as a different piece of hardware in the guest). In an ideal >>> world we (i.e. "I") would have thought of that back when the pci >>> controllers were added, and used some sort of type/class/model >>> notation (where class was used in the way we are now using model, and >>> model was used for the actual manufacturer's model number of a >>> particular family of PCI controller), but that opportunity is long >>> past, so as an alternative, this patch allows selecting a particular >>> implementation of a pci controller with the "type" attribute of the >>> <model> subelement, e.g.: >>> >>> <controller type='pci' model='dmi-to-pci-bridge' index='1'> >>> <model type='i82801b11-bridge'/> >>> </controller> >>> >> >> I'd say 'type' would be more fitting for the generic class of the device >> (dmi-to-pci-bridge), not the exact device model (i82801b11-bridge) so >> <model name='i82801b11-bridge'/> looks nicer to me, but I'm not going to >> suggest it again. >> > > Well, you just did. But I must say I like "model name" better too. > When I pitched the "model type" I did that to show the creation of the > new element, not the spelling itself. Using "model type" is based on the existing "model type" for <interface> and <video>. I also think "model name" is more fitting, but it breaks with precedent. Should we knowingly be inconsistent in order to make the new one better? I'm undecided.

> >>> In this case, "dmi-to-pci-bridge" is the kind of controller (one that >>> has a single PCIe port upstream, and 32 standard PCI ports downstream, >>> which are not hotpluggable), and the qemu device to be used to >>> implement this kind of controller is named "i82801b11-bridge". >>> >>> Implementing the above now will allow us in the future to add a new >>> kind of dmi-to-pci-bridge that doesn't use qemu's i82801b11-bridge >>> device, but instead uses something else (which doesn't yet exist, but >>> qemu people have been discussing it), all without breaking existing >>> configs. >>> >>> (note that for the existing "pci-bridge" type of PCI controller, both >>> the model attribute and <model> type are 'pci-bridge'. This is just a >>> coincidence, since it turns out that in this case the device name in >>> qemu really is a generic 'pci-bridge' rather than being the name of >>> some real-world chip) >>> --- >>> new in V2 (previously was a part of the patch to add pcie-root-port) >>> >>> docs/formatdomain.html.in | 12 ++++++++++++ >>> docs/schemas/domaincommon.rng | 13 +++++++++++++ >>> src/conf/domain_conf.c | 23 >>> +++++++++++++++++++++-- >>> src/conf/domain_conf.h | 8 ++++++++ >>> tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 ++++++-- >>> tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 ++++++-- >>> 6 files changed, 66 insertions(+), 6 deletions(-) >> >>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h >>> index 50750c1..09fe3c0 100644 >>> --- a/src/conf/domain_conf.h >>> +++ b/src/conf/domain_conf.h >>> @@ -797,6 +797,14 @@ typedef virDomainPCIControllerOpts >>> *virDomainPCIControllerOptsPtr; >>> struct _virDomainPCIControllerOpts { >>> bool pcihole64; >>> unsigned long pcihole64size; >>> + >>> + /* the type is in the "model" subelement, e.g.: >>> + * <controller type='pci' model='pcie-root-port'> >>> + * <model type='ioh3420''/> >>> + * ... >>> + * similar to the model of <interface> devices. >>> + */ >>> + char *type; /* the exact name of the device in hypervisor */ >> >> This would be nicer as an enum - we don't allow arbitrary strings there >> anyway. >> > > The only place where we allow arbitrary strings is the interface model > and due to that we're having some problems. Not allowing strings is > good, storing the enum value is even better. Again, I used a string because that's the way it is in existing code (interface), but will change it to an enum for efficiency's sake. (I'm curious what problem you're refering to though - is it the fact that qemuBuildNicDevStr() doesn't qualify the string before using it in the commandline? That is taken care of in the case of this code.) -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list