[libvirt] [PATCHv3 03/13] conf: add new <target> subelement with chassisNr attribute to <controller>

[Martin Kletzander]

On Sat, Jul 25, 2015 at 03:58:27PM -0400, Laine Stump wrote:
>There are some configuration options to some types of pci controllers
>that are currently automatically derived from other parts of the
>controller's configuration. For example, in qemu a pci-bridge
>controller has an option that is called "chassis_nr"; up until now
>libvirt has always set chassis_nr to the index of the pci-bridge. So
>this:
>
>  <controller type='pci' model='pci-bridge' index='2'/>
>
>will always result in:
>
>  -device pci-bridge,chassis_nr=2,...
>
>on the qemu commandline. In the future we may decide there is a better
>way to derive that option, but even in that case we will need for
>existing domains to retain the same chassis_nr they were using in the
>past - that is something that is visible to the guest so it is part of
>the guest ABI and changing it would lead to problems for migrating
>guests (or just guests with very picky OSes).
>
>The <target> subelement has been added as a place to put the new
>"chassisNr" attribute that will be filled in by libvirt when it
>auto-generates the chassisNr; it will be saved in the config, then
>reused any time the domain is started:
>
>  <controller type='pci' model='pci-bridge' index='2'>
>    <model type='pci-bridge'/>
>    <target chassisNr='2'/>
>  </controller>
>
>The one oddity of all this is that if the controller configuration
>is changed (for example to change the index or the pci address
>where the controller is plugged in), the items in <target> will
>*not* be re-generated, which might lead to conflict. I can't
>really see any way around this, but fortunately if there is a
>material conflict qemu will let us know and we will pass that on
>to the user.
>---
>
>changes from V2:
>
>* check chassisNr for 0-255 range
>* multiple <target>s in a single controller is now an error
>* 1.3.0 -> 1.2.18
>
> docs/formatdomain.html.in                       | 24 +++++++++++++
> docs/schemas/domaincommon.rng                   | 10 ++++++
> src/conf/domain_conf.c                          | 45 +++++++++++++++++++++++--
> src/conf/domain_conf.h                          | 10 +++++-
> tests/qemuxml2argvdata/qemuxml2argv-q35.xml     |  1 +
> tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml |  1 +
> 6 files changed, 88 insertions(+), 3 deletions(-)
>
>diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>index 9abceee..fdf7e82 100644
>--- a/docs/formatdomain.html.in
>+++ b/docs/formatdomain.html.in
>@@ -3055,6 +3055,30 @@
>       only).</span>
>     </p>
>     <p>
>+      PCI controllers also have an optional
>+      subelement <code><target></code> with the attributes
>+      listed below. These are configurable items that 1) are visible
>+      to the guest OS so must be preserved for guest ABI
>+      compatibility, and 2) are usually left to default values or
>+      derived automatically by libvirt. In almost all cases, you
>+      should not manually add a <code><target></code> subelement
>+      to a controller, nor should you modify the values in the those
>+      that are automatically generated by
>+      libvirt. <span class="since">Since 1.2.18 (QEMU only).</span>

s/18/19/

>+    </p>
>+    <dl>
>+      <dt><code>chassisNr</code></dt>
>+      <dd>
>+        PCI controllers that have attribute model="pci-bridge", can
>+        also have a <code>chassisNr</code> attribute in
>+        the <code><target></code> subelement, which is used to
>+        control QEMU's "chassis_nr" option for the pci-bridge device
>+        (normally libvirt automatically sets this to the same value as
>+        the index attribute of the pci controller). If set, chassisNr
>+        must be between 0 and 255.
>+      </dd>
>+    </dl>
>+    <p>
>       For machine types which provide an implicit PCI bus, the pci-root
>       controller with index=0 is auto-added and required to use PCI devices.
>       pci-root has no address.
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index d58e679..fbad7e9 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -7826,6 +7830,15 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>                 }
>                 modelName = virXMLPropString(cur, "name");
>                 processedModel = true;
>+            } else if (xmlStrEqual(cur->name, BAD_CAST "target")) {
>+                if (processedTarget) {
>+                    virReportError(VIR_ERR_XML_ERROR, "%s",
>+                                   _("Multiple <target> elements in "
>+                                     "controller definition not allowed"));
>+                    goto error;
>+                }
>+                chassisNr = virXMLPropString(cur, "chassisNr");
>+                processedTarget = true;

Similarly to the previous patch you could omit the boolean here.  Also
there will be way more similarities in following patches that could be
coupled in a function.  But that's more like suggestion for future
since it wouldn't go with the rest of the file, the whole parsing
could use a re-factor :)

ACK
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150803/83855635/attachment-0001.sig>