[libvirt] [PATCH v2 03/10] conf: Introduce scsi hostdev
Osier Yang
jyang at redhat.com
Wed Apr 3 08:32:30 UTC 2013
On 03/04/13 16:29, Osier Yang wrote:
> On 01/04/13 20:00, Han Cheng wrote:
>> Adding scsi hostdev, it should like:
>
> s/Adding/Add, s/should/looks/,
>
>>
>> <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>
>>
>> Signed-off-by: Han Cheng <hanc.fnst at cn.fujitsu.com>
>> ---
>> docs/formatdomain.html.in | 34 ++++++--
>> docs/schemas/domaincommon.rng | 24 +++++
>> src/conf/domain_audit.c | 10 ++
>> src/conf/domain_conf.c | 192
>> ++++++++++++++++++++++++++++++++++++++++-
>> src/conf/domain_conf.h | 7 ++
>> 5 files changed, 258 insertions(+), 9 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index a6bacfa..be3630e 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -2172,13 +2172,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 0.13.0 for SCSI(KVM only)</span>:
>
> s/0.13.0/1.0.5/, confused with qemu's version?
>
>> </p>
>> <pre>
>> @@ -2209,12 +2209,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='scsi' 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
>> @@ -2224,13 +2241,15 @@
>> 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 do not
>> + use this device on host</dd>
>
> s/do not use this device on host/the device is not used by either host
> or any guest./
>
>> <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 is described by <code>adapter<code> and
>> <code>address<code>.
>> <span class="since">Since 1.0.0</span>, the
>> <code>source</code> element
>> of USB devices may contain <code>startupPolicy</code>
>> attribute which can
>> @@ -2268,6 +2287,7 @@
>> <a href="#elementsOSBIOS">BIOS bootloader</a> section.
>> <span class="since">Since 0.8.8</span> for PCI devices,
>> <span class="since">Since 1.0.1</span> for USB devices.
>> + <span class="since">Since 1.0.0</span> for SCSI devices.
>
> s/1.0.0/1.0.5/
>
>> <dt><code>rom</code></dt>
>> <dd>The <code>rom</code> element is used to change how a PCI
>> device's ROM is presented to the guest. The optional
>> <code>bar</code>
>> diff --git a/docs/schemas/domaincommon.rng
>> b/docs/schemas/domaincommon.rng
>> index 364abf1..209d4f5 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -2945,6 +2945,7 @@
>> <choice>
>> <ref name="hostdevsubsyspci"/>
>> <ref name="hostdevsubsysusb"/>
>> + <ref name="hostdevsubsysscsi"/>
>> </choice>
>> </define>
>> @@ -2997,6 +2998,18 @@
>> </element>
>> </define>
>> + <define name="hostdevsubsysscsi">
>> + <attribute name="type">
>> + <value>scsi</value>
>> + </attribute>
>> + <element name="source">
>> + <ref name="sourceinfoadapter"/>
>> + <element name="address">
>> + <ref name="scsiaddress"/>
>> + </element>
>> + </element>
>> + </define>
>
> I was confused. The attribute "readonly" should be in
> "hostdevsubsysscsi",
> as it's only about the "source".
>
>> +
>> <define name="hostdevcapsstorage">
>> <attribute name="type">
>> <value>storage</value>
>> @@ -3041,6 +3054,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 a776058..2fb5989 100644
>> --- a/src/conf/domain_audit.c
>> +++ b/src/conf/domain_audit.c
>> @@ -398,6 +398,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 d9d6b9f..c98ca48 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -573,7 +573,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(virDomainHostdevCaps,
>> VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST,
>> "storage",
>> @@ -1558,7 +1559,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);
>> @@ -1567,6 +1569,11 @@ void
>> virDomainHostdevDefClear(virDomainHostdevDefPtr def)
>> VIR_FREE(def->source.caps.u.misc.chardev);
>> 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;
>> }
>> }
>> @@ -3080,6 +3087,104 @@ virDomainParseLegacyDeviceAddress(char
>> *devaddr,
>> }
>> static int
>> +virDomainHostdevSubsysScsiDefParseXML(const xmlNodePtr node,
>> + virDomainHostdevDefPtr def)
>> +{
>> + int ret = -1, got_address = 0, got_adapter = 0;
>
> 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,
>> + _("more than one addresses are
>> specified"));
>> + goto out;
>> + }
>> + if (!(bus = virXMLPropString(cur, "bus"))) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("bus must be specified for scsi
>> hostdev address"));
>> + goto out;
>> + }
>> + if (virStrToLong_ui(bus, NULL, 0,
>> &def->source.subsys.u.scsi.bus) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("cannot parse bus %s"), bus);
>> + goto out;
>> + }
>> + if (!(target = virXMLPropString(cur, "target"))) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("target must be specified for
>> scsi hostdev address"));
>> + goto out;
>> + }
>> + if (virStrToLong_ui(target, NULL, 0,
>> &def->source.subsys.u.scsi.target) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("cannot parse target %s"),
>> target);
>> + goto out;
>> + }
>> + if (!(unit = virXMLPropString(cur, "unit"))) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("unit must be specified for
>> scsi hostdev address"));
>> + goto out;
>> + }
>> + if (virStrToLong_ui(unit, NULL, 0,
>> &def->source.subsys.u.scsi.unit) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("cannot parse unit %s"), unit);
>> + goto out;
>> + }
>> + VIR_FREE(bus);
>> + VIR_FREE(target);
>> + VIR_FREE(unit);
>
> No need to free these 3 strings here. They are free'ed in "out:"
> anyway. And since
> you have "git_address". They can be only parsed once. So no worry of
> the leaking
> here.
>
>> + got_address = 1;
>
> And
>
> got_adress = true;
>
>> + } else if (xmlStrEqual(cur->name, BAD_CAST "adapter")) {
>> + if (got_adapter) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("more than one adapters are
>> specified"));
>> + goto out;
>> + }
>> + if (!(def->source.subsys.u.scsi.adapter =
>> + virXMLPropString(cur, "name"))) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("adapter must be specified for
>> scsi hostdev address"));
>> + goto out;
>> + }
>> + got_adapter = 1;
>
> got_adapter = true;
>
>> + } else {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("unknown scsi source type '%s'"),
>
> Improper error.
>
>> + cur->name);
>> + goto out;
>> + }
>> + }
>> + cur = cur->next;
>> + }
>> +
>> + if (!got_address && !got_adapter) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + "%s", _("missing adapter and address"));
>> + goto out;
>> + }
>> + if (!got_address && got_adapter) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + "%s", _("missing address"));
>> + goto out;
>> + }
>> + if (got_address && !got_adapter) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + "%s", _("missing adapter"));
>> + goto out;
>> + }
>
> Above checking can be simplied as:
>
> if (!got_address || !got_dapter) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("'adapter' and 'address' must be
> specified"));
> goto out;
> }
>
>> +
>> + ret = 0;
>> +out:
>
> s/out/cleanup/, this applies across the function. Some old code still
> use the "out", but new code should be consistent as much as possible.
>
>> + VIR_FREE(bus);
>> + VIR_FREE(target);
>> + VIR_FREE(unit);
>> + return ret;
>> +}
>> +
>> +static int
>> virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node,
>> virDomainHostdevDefPtr def)
>> {
>> @@ -3374,6 +3479,10 @@ 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"),
>> @@ -8036,6 +8145,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node,
>> }
>> if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
>> + xmlNodePtr cur;
>> + unsigned int readonly = 0;
>> switch (def->source.subsys.type) {
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>> if (def->info->type !=
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>> @@ -8044,6 +8155,20 @@
>> (const xmlNodePtr node,
>> _("PCI host devices must use 'pci'
>> address type"));
>> goto error;
>> }
>> +
>> + break;
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
>> + cur = node->children;
>> + while (cur != NULL) {
>> + if (cur->type == XML_ELEMENT_NODE) {
>> + if (readonly == 0 &&
>> + xmlStrEqual(cur->name, BAD_CAST "readonly"))
>> + readonly = 1;
>> + }
>> + cur = cur->next;
>> + }
>> + def->readonly = readonly;
>
> Agreed with Hu Tao, that "readonly" should be of boolean type.
>
>> +
>> break;
>> }
>> }
>> @@ -8567,6 +8692,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,
>> @@ -8580,6 +8716,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;
>> }
>> @@ -10773,6 +10911,16 @@ virDomainDefParseXML(virCapsPtr caps,
>> goto error;
>> }
>> + if (hostdev->source.subsys.type ==
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
>> + hostdev->info->type ==
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
>> + /* reverse first 16 unit for disk usage */
>> + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
>> + hostdev->info->addr.drive.controller = 0;
>> + hostdev->info->addr.drive.bus = 0;
>> + hostdev->info->addr.drive.target = 0;
>
> Why this defdaults to 0? Can you explain it either in the commit log or
> by comments?
>
>> + hostdev->info->addr.drive.unit = 16 + i;
>
> And why the "16".
>
>> + }
>> +
>> def->hostdevs[def->nhostdevs++] = hostdev;
>> }
>> VIR_FREE(nodes);
>> @@ -12340,6 +12488,30 @@
>> virDomainDefMaybeAddSmartcardController(virDomainDefPtr def)
>> return 0;
>> }
>> +static int
>> +virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>
> Why do we need this?
Ignore this, just typed too quickly. Besides, the tests for the new
XMLs should be
merged into this patch. It's also for you to find out if the XML works
well, other than
later.
>
>> +{
>> + /* Look for any hostdev scsi dev */
>> + int i;
>> + int maxController = -1;
>> + virDomainHostdevDefPtr hostdev;
>> +
>> + for (i = 0; i < def->nhostdevs; i++) {
>> + hostdev = *(def->hostdevs + i);
>
> Generally we use:
> 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) < 0)
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> /*
>> * Based on the declared <address/> info for any devices,
>> @@ -12376,6 +12548,9 @@
>> virDomainDefAddImplicitControllers(virDomainDefPtr def)
>> if (virDomainDefMaybeAddSmartcardController(def) < 0)
>> return -1;
>> + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0)
>> + return -1;
>> +
>> return 0;
>> }
>> @@ -13194,6 +13369,15 @@
>> virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> virBufferAdjustIndent(buf, 2);
>> switch (def->source.subsys.type)
>> {
>> + 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",
>> + ? "type='scsi' " : "",
>> + def->source.subsys.u.scsi.bus,
>> + def->source.subsys.u.scsi.target,
>> + def->source.subsys.u.scsi.unit);
>> + break;
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
>> if (def->source.subsys.u.usb.vendor) {
>> virBufferAsprintf(buf, "<vendor id='0x%.4x'/>\n",
>> @@ -14446,6 +14630,9 @@ virDomainHostdevDefFormat(virBufferPtr buf,
>> case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
>> if (virDomainHostdevDefFormatSubsys(buf, def, flags, false)
>> < 0)
>> return -1;
>> + if (def->source.subsys.type ==
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
>> + def->readonly)
>> + virBufferAsprintf(buf, "<readonly/>\n");
>> break;
>> case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES:
>> if (virDomainHostdevDefFormatCaps(buf, def) < 0)
>> @@ -14454,6 +14641,7 @@ virDomainHostdevDefFormat(virBufferPtr buf,
>> }
>> virBufferAdjustIndent(buf, -6);
>> +
>> if (virDomainDeviceInfoFormat(buf, def->info,
>> flags |
>> VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT
>> |
>> VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM) < 0)
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index f8e3973..2d238eb 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -380,6 +380,7 @@ enum virDomainHostdevMode {
>> enum virDomainHostdevSubsysType {
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB,
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI,
>> + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI,
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST
>> };
>> @@ -399,6 +400,12 @@ struct _virDomainHostdevSubsys {
>> unsigned vendor;
>> unsigned product;
>> } usb;
>> + struct {
>> + char *adapter;
>> + unsigned bus;
>> + unsigned target;
>> + unsigned unit;
>> + } scsi;
>> virDevicePCIAddress pci; /* host address */
>> } u;
>> };
>
More information about the libvir-list
mailing list