Re: [libvirt] [PATCHv3 01/13] conf: add new <model> subelement with name attribute to <controller>

On Sat, Jul 25, 2015 at 03:58:25PM -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 "name" attribute of the > <model> subelement, e.g.: > > <controller type='pci' model='dmi-to-pci-bridge' index='1'> > <model name='i82801b11-bridge'/> > </controller> > > 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> name 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) > --- > change from V2: > > * attribute is now called "name" instead of "type" > * different possible model names are stored internally as an enum > value rather than a string. > * more than one occurence of <model> in a single controller is now > considered an error > * docs say "1.2.18" instead of "1.3.0" > > docs/formatdomain.html.in | 13 +++++++ > docs/schemas/domaincommon.rng | 13 +++++++ > src/conf/domain_conf.c | 46 > +++++++++++++++++++++++-- > src/conf/domain_conf.h | 16 +++++++++ > src/libvirt_private.syms | 2 ++ > tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 +++-- > tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 +++-- > 7 files changed, 100 insertions(+), 6 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 6b557d1..d58e679 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -7809,6 +7817,15 @@ virDomainControllerDefParseXML(xmlNodePtr node, > queues = virXMLPropString(cur, "queues"); > cmd_per_lun = virXMLPropString(cur, "cmd_per_lun"); > max_sectors = virXMLPropString(cur, "max_sectors"); > + } else if (xmlStrEqual(cur->name, BAD_CAST "model")) { > + if (processedModel) { Unnecessary variable, you could just use 'modelName',

but I have no opinion on that, so ACK either way.