[libvirt] [PATCH 01/25] conf: Generic XMLs for scsi hostdev
Osier Yang
jyang at redhat.com
Mon May 13 10:28:08 UTC 2013
On 07/05/13 18:38, Osier Yang wrote:
> On 07/05/13 00:33, John Ferlan wrote:
>> On 05/03/2013 02:07 PM, Osier Yang wrote:
>>> From: Han Cheng <hanc.fnst at cn.fujitsu.com>
>>>
>>> An example of the scsi hostdev XML:
>>>
>>> <hostdev mode='subsystem' type='scsi'>
>>> <source>
>>> <adapter name='scsi_host0'/>
>>> <address bus='0' target='0' unit='0'/>
>>> </source>
>>> <address type='drive' controller='0' bus='0' target='4'
>>> unit='8'/>
>>> </hostdev>
>>>
>>> Controller is implicitly added for scsi hostdev, though the scsi
>>> controller's model defaults to "lsilogic", which might be not what
>>> the user wants (same problem exists for virtio-scsi disk). It's
>>> the existing problem, will be addressed later.
>>>
>>> The device address must be specified manually. Later patch will let
>>> libvirt generate it automatically.
>>>
>>> This only introduces the generic XMLs for scsi hostdev, later patches
>>> will add other elements, e.g. <readonly>, <shareable>.
>>>
>>> Signed-off-by: Han Cheng <hanc.fnst at cn.fujitsu.com>
>>> Signed-off-by: Osier Yang <jyang at redhat.com>
>>>
>>> ---
>>> v3 - v4:
>>> * Split 1/10 in v3 into two patches, <readonly> will be in a later
>>> patch
>>> * Error messages are improved.
>>>
>>> v2.5 - v3:
>>> * Remove the rigid algrithom to generate the address, the address of
>>> scsi host device must be specified manually now, later patch will
>>> add the generator.
>>> * Merge the xml2xml test from 10/10 into this patch
>>> * s/1.0.5/1.0.6/
>>> * Improve the XML parsing, fixes on virReportError statements, typos
>>> * hostdev->readonly is changed from bit field into boolean
>>> ---
>>> docs/formatdomain.html.in | 34 ++++-
>>> docs/schemas/domaincommon.rng | 26 ++++
>>> src/conf/domain_audit.c | 10 ++
>>> src/conf/domain_conf.c | 161
>>> ++++++++++++++++++++-
>>> src/conf/domain_conf.h | 7 +
>>> .../qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml | 35 +++++
>>> tests/qemuxml2xmltest.c | 2 +
>>> 7 files changed, 266 insertions(+), 9 deletions(-)
>>> create mode 100644
>>> tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml
>>>
>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>> index 8d4edfb..488673f 100644
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -2249,13 +2249,13 @@
>>> <h4><a name="elementsHostDev">Host device assignment</a></h4>
>>> - <h5><a href="elementsHostDevSubsys">USB / PCI devices</a></h5>
>>> + <h5><a href="elementsHostDevSubsys">USB / PCI / SCSI
>>> devices</a></h5>
>>> <p>
>>> - USB and PCI devices attached to the host can be passed through
>>> + USB, PCI and SCSI devices attached to the host can be passed
>>> through
>>> to the guest using the <code>hostdev</code> element.
>>> - <span class="since">since after 0.4.4 for USB and 0.6.0 for PCI
>>> - (KVM only)</span>:
>>> + <span class="since">since after 0.4.4 for USB, 0.6.0 for
>>> PCI(KVM only)
>>> + and 1.0.6 for SCSI(KVM only)</span>:
>>> </p>
>>> <pre>
>>> @@ -2286,12 +2286,29 @@
>>> </devices>
>>> ...</pre>
>>> + <p>or:</p>
>>> +
>>> +<pre>
>>> + ...
>>> + <devices>
>>> + <hostdev mode='subsystem' type='scsi'>
>>> + <source>
>>> + <adapter name='scsi_host0'/>
>>> + <address type='scsi' bus='0' target='0' unit='0'/>
>>> + </source>
>>> + <readonly/>
>>> + <address type='drive' controller='0' bus='0' target='0'
>>> unit='0'/>
>>> + </hostdev>
>>> + </devices>
>>> + ...</pre>
>>> +
>>> <dl>
>>> <dt><code>hostdev</code></dt>
>>> <dd>The <code>hostdev</code> element is the main container
>>> for describing
>>> host devices. For usb device passthrough <code>mode</code>
>>> is always
>>> - "subsystem" and <code>type</code> is "usb" for a USB device
>>> and "pci"
>>> - for a PCI device. When <code>managed</code> is "yes" for a PCI
>>> + "subsystem" and <code>type</code> is "usb" for a USB
>>> device, "pci"
>>> + for a PCI device and "scsi" for a SCSI device. When
>>> + <code>managed</code> is "yes" for a PCI
>>> device, it is detached from the host before being passed
>>> on to
>>> the guest, and reattached to the host after the guest exits.
>>> If <code>managed</code> is omitted or "no", and for USB
>>> @@ -2301,13 +2318,16 @@
>>> hot-plugging the device,
>>> and <code>virNodeDeviceReAttach</code> (or <code>virsh
>>> nodedev-reattach</code>) after hot-unplug or stopping the
>>> - guest.</dd>
>>> + guest. For SCSI device, user is responsible to make sure
>>> the device
>>> + is not used by host.</dd>
>>> <dt><code>source</code></dt>
>>> <dd>The source element describes the device as seen from the
>>> host.
>>> The USB device can either be addressed by vendor / product
>>> id using the
>>> <code>vendor</code> and <code>product</code> elements or by
>>> the device's
>>> address on the hosts using the <code>address</code> element.
>>> PCI devices
>>> on the other hand can only be described by their
>>> <code>address</code>.
>>> + SCSI devices can be described by <code>adapter</code> and
>>> + <code>address</code>.
>> s/can be/are/
>> s/by/by both the/
>> s/./ elements./
>>
>> E.G. "SCSI devices are described by both the adapter and address
>> elements."
>
> I'd like keep the <code>...</code>
>
>>
>> I would say that a couple of changes could be made... Your choice
>> whether to make them or not. The paragraph just reads funny as if
>> someone has only "added" to it without regard to how the existing
>> elements are described.
>>
>> "USB devices can either be described by vendor/product id using the
>> vendor or product elements or by the hosts' USB device address using the
>> address element."
>>
>> "PCI devices are described by their address element." ?Is this the
>> "hosts' PCI address? or just one fabricated for the guest?
>
> It should be the "host address". Yeah, I feel a bit confused when seeing
> them, could be a later patch.
>
>>
>>
>>> <span class="since">Since 1.0.0</span>, the
>>> <code>source</code> element
>>> of USB devices may contain <code>startupPolicy</code>
>>> attribute which can
>>> diff --git a/docs/schemas/domaincommon.rng
>>> b/docs/schemas/domaincommon.rng
>>> index 10596dc..6a4b77a 100644
>>> --- a/docs/schemas/domaincommon.rng
>>> +++ b/docs/schemas/domaincommon.rng
>>> @@ -3094,6 +3094,7 @@
>>> <choice>
>>> <ref name="hostdevsubsyspci"/>
>>> <ref name="hostdevsubsysusb"/>
>>> + <ref name="hostdevsubsysscsi"/>
>>> </choice>
>>> </define>
>>> @@ -3162,6 +3163,20 @@
>>> </element>
>>> </define>
>>> + <define name="hostdevsubsysscsi">
>>> + <attribute name="type">
>>> + <value>scsi</value>
>>> + </attribute>
>>> + <element name="source">
>>> + <interleave>
>>> + <ref name="sourceinfoadapter"/>
>>> + <element name="address">
>>> + <ref name="scsiaddress"/>
>>> + </element>
>>> + </interleave>
>>> + </element>
>>> + </define>
>>> +
>>> <define name="hostdevcapsstorage">
>>> <attribute name="type">
>>> <value>storage</value>
>>> @@ -3217,6 +3232,17 @@
>>> </attribute>
>>> </element>
>>> </define>
>>> + <define name="scsiaddress">
>>> + <attribute name="bus">
>>> + <ref name="driveBus"/>
>>> + </attribute>
>>> + <attribute name="target">
>>> + <ref name="driveTarget"/>
>>> + </attribute>
>>> + <attribute name="unit">
>>> + <ref name="driveUnit"/>
>>> + </attribute>
>>> + </define>
>>> <define name="usbportaddress">
>>> <attribute name="bus">
>>> <ref name="usbAddr"/>
>>> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
>>> index 40910d6..a6cefb2 100644
>>> --- a/src/conf/domain_audit.c
>>> +++ b/src/conf/domain_audit.c
>>> @@ -399,6 +399,16 @@ virDomainAuditHostdev(virDomainObjPtr vm,
>>> virDomainHostdevDefPtr hostdev,
>>> goto cleanup;
>>> }
>>> break;
>>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
>>> + if (virAsprintf(&address, "%s:%d:%d:%d",
>>> + hostdev->source.subsys.u.scsi.adapter,
>>> + hostdev->source.subsys.u.scsi.bus,
>>> + hostdev->source.subsys.u.scsi.target,
>>> + hostdev->source.subsys.u.scsi.unit) < 0) {
>>> + VIR_WARN("OOM while encoding audit message");
>>> + goto cleanup;
>>> + }
>>> + break;
>>> default:
>>> VIR_WARN("Unexpected hostdev type while encoding audit
>>> message: %d",
>>> hostdev->source.subsys.type);
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index fe97c02..9e6b65b 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -585,7 +585,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode,
>>> VIR_DOMAIN_HOSTDEV_MODE_LAST,
>>> VIR_ENUM_IMPL(virDomainHostdevSubsys,
>>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST,
>>> "usb",
>>> - "pci")
>>> + "pci",
>>> + "scsi")
>>> VIR_ENUM_IMPL(virDomainHostdevSubsysPciBackend,
>>> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST,
>>> @@ -1634,7 +1635,8 @@ void
>>> virDomainHostdevDefClear(virDomainHostdevDefPtr def)
>>> if (def->parent.type == VIR_DOMAIN_DEVICE_NONE)
>>> virDomainDeviceInfoFree(def->info);
>>> - if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) {
>>> + switch (def->mode) {
>>> + case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES:
>>> switch (def->source.caps.type) {
>>> case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE:
>>> VIR_FREE(def->source.caps.u.storage.block);
>>> @@ -1646,6 +1648,11 @@ void
>>> virDomainHostdevDefClear(virDomainHostdevDefPtr def)
>>> VIR_FREE(def->source.caps.u.net.iface);
>>> break;
>>> }
>>> + break;
>>> + case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
>>> + if (def->source.subsys.type ==
>>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
>>> + VIR_FREE(def->source.subsys.u.scsi.adapter);
>>> + break;
>>> }
>>> }
>>> @@ -3681,6 +3688,94 @@ out:
>>> }
>>> static int
>>> +virDomainHostdevSubsysScsiDefParseXML(const xmlNodePtr node,
>>> + virDomainHostdevDefPtr def)
>>> +{
>>> + int ret = -1;
>>> + bool got_address = false, got_adapter = false;
>>> + xmlNodePtr cur;
>>> + char *bus = NULL, *target = NULL, *unit = NULL;
>>> +
>>> + cur = node->children;
>>> + while (cur != NULL) {
>>> + if (cur->type == XML_ELEMENT_NODE) {
>>> + if (xmlStrEqual(cur->name, BAD_CAST "address")) {
>>> + if (got_address) {
>>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>>> + _("more than one source
>>> addresses are "
>>> + "specified for scsi hostdev"));
>> "more than one source address is"
>
> Surely I won't want to talk about English with you. :/
>
>>
>> Should scsi be SCSI when referencing the architecture rather than the
>> attribute name? Or should scsi be single quoted, eg 'scsi'? Not sure
>> if there is a "norm", but it should be consistent with other uses (usb
>> and pci).
>
> It should be "scsi", which is consistent with what we use in the XML.
> And here it's "scsi hostdev", both are the terms we use in XML.
>
>>
>>
>>> + goto cleanup;
>>> + }
>>> +
>>> + if (!(bus = virXMLPropString(cur, "bus")) ||
>>> + !(target = virXMLPropString(cur, "target")) ||
>>> + !(unit = virXMLPropString(cur, "unit"))) {
>>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>>> + _("'bus', 'target', and 'unit'
>>> must be specified "
>>> + "for scsi hostdev source
>>> address"));
>>> + goto cleanup;
>>> + }
>>> +
>>> + if (virStrToLong_ui(bus, NULL, 0,
>>> &def->source.subsys.u.scsi.bus) < 0) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("cannot parse bus '%s'"), bus);
>>> + goto cleanup;
>>> + }
>>> +
>>> + if (virStrToLong_ui(target, NULL, 0,
>>> &def->source.subsys.u.scsi.target) < 0) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("cannot parse target '%s'"),
>>> target);
>>> + goto cleanup;
>>> + }
>>> +
>>> + if (virStrToLong_ui(unit, NULL, 0,
>>> &def->source.subsys.u.scsi.unit) < 0) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("cannot parse unit '%s'"), unit);
>>> + goto cleanup;
>>> + }
>>> +
>>> + got_address = true;
>>> + } else if (xmlStrEqual(cur->name, BAD_CAST "adapter")) {
>>> + if (got_adapter) {
>>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>>> + _("more than one adapters are
>>> specified "
>>> + "for scsi hostdev source"));
>> "more than one adapter is specified"
>>
>>> + goto cleanup;
>>> + }
>>> + if (!(def->source.subsys.u.scsi.adapter =
>>> + virXMLPropString(cur, "name"))) {
>>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>>> + _("'adapter' must be specified
>>> for scsi hostdev source"));
>>> + goto cleanup;
>>> + }
>>> +
>>> + got_adapter = true;
>>> + } else {
>>> + virReportError(VIR_ERR_XML_ERROR,
>>> + _("unsuported element '%s' of scsi
>>> hostdev source"),
>> s/unsuported/unsupported
>>
>>> + cur->name);
>>> + goto cleanup;
>>> + }
>>> + }
>>> + cur = cur->next;
>>> + }
>>> +
>>> + if (!got_address || !got_adapter) {
>>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>>> + _("'adapter' and 'address' must be specified
>>> for scsi "
>>> + "hostdev source"));
>>> + goto cleanup;
>>> + }
>>> +
>>> + ret = 0;
>>> +cleanup:
>>> + VIR_FREE(bus);
>>> + VIR_FREE(target);
>>> + VIR_FREE(unit);
>>> + return ret;
>>> +}
>>> +
>>> +static int
>>> virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>>> xmlXPathContextPtr ctxt,
>>> const char *type,
>>> @@ -3761,6 +3856,12 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr
>>> node,
>>> if (virDomainHostdevSubsysUsbDefParseXML(sourcenode, def)
>>> < 0)
>>> goto error;
>>> break;
>>> +
>>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
>>> + if (virDomainHostdevSubsysScsiDefParseXML(sourcenode, def)
>>> < 0)
>>> + goto error;
>>> + break;
>>> +
>>> default:
>>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> _("address type='%s' not supported in
>>> hostdev interfaces"),
>>> @@ -8617,6 +8718,13 @@ virDomainHostdevDefParseXML(const xmlNodePtr
>>> node,
>>> goto error;
>>> }
>>> break;
>>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
>>> + if (def->info->type ==
>>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
>>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>>> + _("SCSI host devices must have
>>> address specified"));
>> See here it's capitalized - SCSI... Just be consistent in use.
>
> will use "scsi hostdev".
Several lines above, it uses "PCI host devices", so I keep this, we can
change them together later if necessary.
>
>>
>>> + goto error;
>>> + }
>>> + break;
>>> }
>>> }
>>> @@ -9144,6 +9252,17 @@
>>> virDomainHostdevMatchSubsysPCI(virDomainHostdevDefPtr a,
>>> return 0;
>>> }
>>> +static int
>>> +virDomainHostdevMatchSubsysSCSI(virDomainHostdevDefPtr a,
>>> + virDomainHostdevDefPtr b)
>>> +{
>>> + if (STREQ(a->source.subsys.u.scsi.adapter,
>>> b->source.subsys.u.scsi.adapter) &&
>>> + a->source.subsys.u.scsi.bus == b->source.subsys.u.scsi.bus &&
>>> + a->source.subsys.u.scsi.target ==
>>> b->source.subsys.u.scsi.target &&
>>> + a->source.subsys.u.scsi.unit == b->source.subsys.u.scsi.unit)
>>> + return 1;
>>> + return 0;
>>> +}
>>> static int
>>> virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
>>> @@ -9157,6 +9276,8 @@
>>> virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
>>> return virDomainHostdevMatchSubsysPCI(a, b);
>>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
>>> return virDomainHostdevMatchSubsysUSB(a, b);
>>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
>>> + return virDomainHostdevMatchSubsysSCSI(a, b);
>>> }
>>> return 0;
>>> }
>>> @@ -12971,6 +13092,30 @@
>>> virDomainDefMaybeAddSmartcardController(virDomainDefPtr def)
>>> return 0;
>>> }
>>> +static int
>>> +virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>>> +{
>>> + /* Look for any hostdev scsi dev */
>>> + int i;
>>> + int maxController = -1;
>>> + virDomainHostdevDefPtr hostdev;
>>> +
>>> + for (i = 0; i < def->nhostdevs; i++) {
>>> + hostdev = def->hostdevs[i];
>>> + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
>>> + hostdev->source.subsys.type ==
>>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
>>> + (int)hostdev->info->addr.drive.controller >
>>> maxController) {
>>> + maxController = hostdev->info->addr.drive.controller;
>>> + }
>>> + }
>>> +
>>> + for (i = 0; i <= maxController; i++) {
>>> + if (virDomainDefMaybeAddController(def,
>>> VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0)
>>> + return -1;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> /*
>>> * Based on the declared <address/> info for any devices,
>>> @@ -13007,6 +13152,9 @@
>>> virDomainDefAddImplicitControllers(virDomainDefPtr def)
>>> if (virDomainDefMaybeAddSmartcardController(def) < 0)
>>> return -1;
>>> + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0)
>>> + return -1;
>>> +
>>> return 0;
>>> }
>>> @@ -13912,6 +14060,15 @@
>>> virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>>> virBufferAddLit(buf, "</origstates>\n");
>>> }
>>> break;
>>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
>>> + virBufferAsprintf(buf, "<adapter name='%s'/>\n",
>>> + def->source.subsys.u.scsi.adapter);
>>> + virBufferAsprintf(buf, "<address %sbus='%d' target='%d'
>>> unit='%d'/>\n",
>> s/sbus/bus/
It's "sbus", to output the address "type" only if "includeTypeInAddr" is
true.
Changed the other nits and pushed.
More information about the libvir-list
mailing list