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

[Laine Stump]

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.