3:08 p.m.
On 07/24/2015 12:07 PM, Martin Kletzander wrote:
On Fri, Jul 17, 2015 at 02:43:37PM -0400, Laine Stump wrote:
> This controller can be connected (at domain startup time only - not
> hotpluggable) only to a port on the pcie root complex ("pcie-root" in
> libvirt config), hence the new connect type
> VIR_PCI_CONNECT_TYPE_PCIE_ROOT. It provides a hotpluggable port that
> will accept any PCI or PCIe device.
>
> New attributes must be added to the controller <target> subelement for
> this - chassis and port are guest-visible option values that will be
> set by libvirt with values derived from the controller's index and pci
> address information.
> ---
> docs/formatdomain.html.in | 35
> ++++++++++++++++++--
> docs/schemas/domaincommon.rng | 15 ++++++++-
> src/conf/domain_addr.c | 10 ++++--
> src/conf/domain_addr.h | 5 ++-
> src/conf/domain_conf.c | 37
> ++++++++++++++++++++--
> src/conf/domain_conf.h | 7 +++-
> src/qemu/qemu_command.c | 1 +
> .../qemuxml2argv-pcie-root-port.xml | 36
> +++++++++++++++++++++
> tests/qemuxml2xmltest.c | 1 +
> 9 files changed, 138 insertions(+), 9 deletions(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index ae0d66a..dcbca75 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3070,6 +3071,25 @@
> (normally libvirt automatically sets this to the same value as
> the index attribute of the pci controller).
> </dd>
> + <dt><code>chassis</code></dt>
> + <dd>
> + pcie-root-port controllers can also have
> + a <code>chassis</code> attribute in
> + the <code><model></code> subelement, which is used
to
> + control QEMU's "chassis" option for the device (normally
> + libvirt automatically sets this to the same value as the index
> + attribute of the pci controller).
Talking about QEMU-only options it feels like we're not abstracting
anything although trying to be an abstraction layer. But I don't have
any better explanation, so it's fine, I guess :)
Also I wouldn't mention how the number is calculated, just so you
don't need to mention that it might change.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 17526d4..e02c861 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7811,6 +7820,20 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> chassisNr);
> goto error;
> }
> + if (chassis && virStrToLong_i(chassis, NULL, 0,
> + &def->opts.pciopts.chassis) <
> 0) {
You probably want to use _ui variants not just here, but throughout
the patches.
To do that would require a temporary variable (since chassis is an int
so that it can hold the "-1" value meaning "unspecified". Since the
limits are much lower than what can fit in an int, I'd rather continue
to use *_i and check both ends of the range after conversion (which I'm
adding in to all of them now)
> @@ -18914,6 +18941,12 @@ virDomainControllerDefFormat(virBufferPtr buf,
> if (def->opts.pciopts.chassisNr != -1)
> virBufferAsprintf(buf, " chassisNr='%d'",
> def->opts.pciopts.chassisNr);
> + if (def->opts.pciopts.chassis != -1)
> + virBufferAsprintf(buf, " chassis='%d'",
> + def->opts.pciopts.chassis);
> + if (def->opts.pciopts.port != -1)
> + virBufferAsprintf(buf, " port='0x%x'",
> + def->opts.pciopts.port);
Just out of curiosity, why did you choose to format chassis as decimal
and and port as hexadecimal?
I forget the exact reason, but it had something to do with matching
other existing things. I think in the case of port it is coming from
slot and slot is always printed in hex, but chassis was coming from
index, and index is always printed in decimal. Makes it easier to
visually verify that they match, but no better reason than that :-)
Negative tests wouldn't hurt ;)
The amount of negative tests that could be added is endless. Some
negative tests are impossible though - anything that is outside the
schema in the RNG can never get past the domainschematest even if it's
supposed to fail.