On 08/03/2015 05:55 AM, Martin Kletzander wrote:
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',
If it was by itself, yes. (As a matter of fact that's how I original added the "don't allow multiples" to the patch). But as you found out in patch 3, similar checking is needed for <target>, and target has multiple attributes; rather than checking for "chassis || chassisNr || port" there, I gave that one a single bool that gets set, so I came back and made this one the same to be consistent - the more similar code looks, the easier it is to follow the logic (and to potentially make a utility function or combine code in some other way in the future). Besides, I like to pretend that I'm consistent :-)
but I have no opinion on that, so ACK either way.