On 07/05/13 00:33, John Ferlan wrote:

On 05/03/2013 02:07 PM, Osier Yang wrote:

...

From: Han Cheng <hanc.fnst@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@cn.fujitsu.com> Signed-off-by: Osier Yang <jyang@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".

...

+ 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/

...

+ includeTypeInAddr ? "type='scsi' " : "", + def->source.subsys.u.scsi.bus, + def->source.subsys.u.scsi.target, + def->source.subsys.u.scsi.unit); + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected hostdev type %d"), diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 21f7ce2..1efae69 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -384,6 +384,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 }; @@ -417,6 +418,12 @@ struct _virDomainHostdevSubsys { virDevicePCIAddress addr; /* host address */ int backend; /* enum virDomainHostdevSubsysPciBackendType */ } pci; + struct { + char *adapter; + unsigned bus; + unsigned target; + unsigned unit; + } scsi; } u; };

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml new file mode 100644 index 0000000..5a263e7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <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> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 64c1c83..1ca1f7e 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -285,6 +285,8 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx");

+ DO_TEST("hostdev-scsi"); + virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt);

ACK w/ nits.

John

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 07/05/13 01:20, John Ferlan wrote:

On 05/03/2013 02:07 PM, Osier Yang wrote:

...

From: Han Cheng <hanc.fnst@cn.fujitsu.com>

This patch adds util functions for scsi hostdev.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> Signed-off-by: Osier Yang <jyang@redhat.com>

--- v3 - v4: * Use strdup instead of virAsprintf if the format string is "%s"

v2.5 - v3: * A couple "char *" to "const char *" * s/unsigned int readonly/bool readonly/ * Use STRPREIFX instead of the wrong use of STRSKIP (which can introduce bug) for virSCSIDeviceGetAdapterId. * Improve virSCSIDeviceNew

I'm not fan of introducing another list like virPCIDeviceList, which brought much trouble for us, but it doesn't hurt much to do clean up together later, since they are very similar. --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 22 +++ src/util/virscsi.c | 408 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virscsi.h | 84 ++++++++++ 5 files changed, 516 insertions(+) create mode 100644 src/util/virscsi.c create mode 100644 src/util/virscsi.h

I'll start by noting, I'm still not fully aware of all the "norms" regarding all that needs to be touched when adding a new file and all that needs to be done when adding new API's - so I could miss something that perhaps someone else will pick up on...

...

diff --git a/po/POTFILES.in b/po/POTFILES.in index bf5a864..f3ea4da 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -174,6 +174,7 @@ src/util/virportallocator.c src/util/virprocess.c src/util/virrandom.c src/util/virsexpr.c +src/util/virscsi.c src/util/virsocketaddr.c src/util/virstatslinux.c src/util/virstoragefile.c diff --git a/src/Makefile.am b/src/Makefile.am index 299b8fd..e71975a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -111,6 +111,7 @@ UTIL_SOURCES = \ util/virportallocator.c util/virportallocator.h \ util/virprocess.c util/virprocess.h \ util/virrandom.h util/virrandom.c \ + util/virscsi.c util/virscsi.h \ util/virsexpr.c util/virsexpr.h \ util/virsocketaddr.h util/virsocketaddr.c \ util/virstatslinux.c util/virstatslinux.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 98660dc..64e2816 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1687,6 +1687,28 @@ virRandomGenerateWWN; virRandomInt;

+# util/virscsi.h +virSCSIDeviceFileIterate; +virSCSIDeviceFree; +virSCSIDeviceGetAdapter; +virSCSIDeviceGetBus; +virSCSIDeviceGetDevStr; +virSCSIDeviceGetName; +virSCSIDeviceGetReadonly; +virSCSIDeviceGetTarget; +virSCSIDeviceGetUnit; +virSCSIDeviceGetUsedBy; +virSCSIDeviceListAdd; +virSCSIDeviceListCount; +virSCSIDeviceListDel; +virSCSIDeviceListFind; +virSCSIDeviceListGet; +virSCSIDeviceListNew; +virSCSIDeviceListSteal; +virSCSIDeviceNew; +virSCSIDeviceSetUsedBy; + + # util/virsexpr.h sexpr2string; sexpr_append; diff --git a/src/util/virscsi.c b/src/util/virscsi.c new file mode 100644 index 0000000..2d6bd8c --- /dev/null +++ b/src/util/virscsi.c @@ -0,0 +1,408 @@ +/* + * virscsi.c: helper APIs for managing host SCSI devices + * + * Copyright (C) 2013 Fujitsu, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Han Cheng <hanc.fnst@cn.fujitsu.com> + */ + +#include <config.h> + +#include <dirent.h> +#include <fcntl.h> +#include <inttypes.h> +#include <limits.h> +#include <stdio.h> +#include <string.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> + +#include "virscsi.h" +#include "virlog.h" +#include "viralloc.h" +#include "virutil.h" +#include "virstring.h" +#include "virerror.h" + +#define SYSFS_SCSI_DEVICES "/sys/bus/scsi/devices" + +/* For virReportOOMError() and virReportSystemError() */ +#define VIR_FROM_THIS VIR_FROM_NONE + +struct _virSCSIDevice { + unsigned int adapter; + unsigned int bus; + unsigned int target; + unsigned int unit; + + char *name; /* adapter:bus:target:unit */ + char *id; /* model:vendor */ + char *path; + const char *used_by; /* name of the domain using this dev */ + + bool readonly; +}; + +struct _virSCSIDeviceList { + virObjectLockable parent; + unsigned int count; + virSCSIDevicePtr *devs; +}; + +static virClassPtr virSCSIDeviceListClass; + +static void virSCSIDeviceListDispose(void *obj); + +static int +virSCSIOnceInit(void) +{ + if (!(virSCSIDeviceListClass = virClassNew(virClassForObjectLockable(), + "virSCSIDeviceList", + sizeof(virSCSIDeviceList), + virSCSIDeviceListDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virSCSI) + +static int +virSCSIDeviceGetAdapterId(const char *adapter, + unsigned int *adapter_id) +{ + if (STRPREFIX(adapter, "scsi_host")) { + if (virStrToLong_ui(adapter + strlen("scsi_host"), + NULL, 0, adapter_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse adapter '%s'"), adapter); + return -1; + } + } + hmm... which makes me think, does the description in patch 1/25 need to be adjusted to indicate that "scsi_host" is expected as part of the adapter name? and that adapters are thus identified by the numeric value after the prefix?

No, forcely checking it when parsing is not a good idea, just for QEMU/KVM driver, we want it has a prefix "scsi_host", it might be different for other drivers, though currently we only support qemu driver. Even checking it here is not that good, but given that this helper is only used by qemu driver now, I think it's fine, it can be changed if need in future.

...

+ return 0; +} + +char * +virSCSIDeviceGetDevStr(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit) +{ + DIR *dir = NULL; + struct dirent *entry; + char *path = NULL; + char *sg = NULL; + unsigned int adapter_id; + + if (virSCSIDeviceGetAdapterId(adapter, &adapter_id) < 0) + return NULL; + + if (virAsprintf(&path, + SYSFS_SCSI_DEVICES "/%d:%d:%d:%d/scsi_generic", + adapter_id, bus, target, unit) < 0) { + virReportOOMError(); + return NULL; + } + + if (!(dir = opendir(path))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open %s"), path); + goto cleanup; + } + + while ((entry = readdir(dir))) { + if (entry->d_name[0] == '.') + continue; + + if (!(sg = strdup(entry->d_name))) { + virReportOOMError(); + goto cleanup; + } + } + +cleanup: + closedir(dir); + VIR_FREE(path); + return sg; +} + +virSCSIDevicePtr +virSCSIDeviceNew(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit, + bool readonly) +{ + virSCSIDevicePtr dev, ret = NULL; + char *sg = NULL; + char *vendor_path = NULL; + char *model_path = NULL; + char *vendor = NULL; + char *model = NULL; + + if (VIR_ALLOC(dev) < 0) { + virReportOOMError(); + return NULL; + } + + dev->bus = bus; + dev->target = target; + dev->unit = unit; + dev->readonly = readonly; + + if (!(sg = virSCSIDeviceGetDevStr(adapter, bus, target, unit))) + goto cleanup; 'sg' is now strdup()'d memory

...

+ + if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) + goto cleanup; + + if (virAsprintf(&dev->name, "%d:%d:%d:%d", dev->adapter, + dev->bus, dev->bus, dev->unit) < 0 || + virAsprintf(&dev->path, "/dev/%s", sg) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (access(dev->path, F_OK) != 0) { + virReportSystemError(errno, + _("Device %s not found: could not access %s"), + dev->name, dev->path); Is it really 'not found'? Perhaps "SCSI device '%s': could not access '%s'"?

Agreed

...

+ goto cleanup; + } + + if (virAsprintf(&vendor_path, + SYSFS_SCSI_DEVICES "/%s/vendor", dev->name) < 0 || + virAsprintf(&model_path, + SYSFS_SCSI_DEVICES "/%s/model", dev->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virFileReadAll(vendor_path, 1024, &vendor) < 0) + goto cleanup; + + if (virFileReadAll(model_path, 1024, &model) < 0) + goto cleanup; + + virTrimSpaces(vendor, NULL); + virTrimSpaces(model, NULL); + + if (virAsprintf(&dev->id, "%s:%s", vendor, model) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret = dev; +cleanup: VIR_FREE(sg);

...

+ VIR_FREE(vendor); + VIR_FREE(model); + VIR_FREE(vendor_path); + VIR_FREE(model_path); + if (!ret) + virSCSIDeviceFree(dev); + return ret; +} + +void +virSCSIDeviceFree(virSCSIDevicePtr dev) +{ + if (!dev) + return; + + VIR_FREE(dev->id); + VIR_FREE(dev->name); + VIR_FREE(dev->path); + VIR_FREE(dev); +} + +void +virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, + const char *name) +{ + dev->used_by = name; +} + +const char * +virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev) +{ + return dev->used_by; +} + +const char * +virSCSIDeviceGetName(virSCSIDevicePtr dev) +{ + return dev->name; +} + +unsigned int +virSCSIDeviceGetAdapter(virSCSIDevicePtr dev) +{ + return dev->adapter; +} + +unsigned int +virSCSIDeviceGetBus(virSCSIDevicePtr dev) +{ + return dev->bus; +} + +unsigned int +virSCSIDeviceGetTarget(virSCSIDevicePtr dev) +{ + return dev->target; +} + +unsigned int +virSCSIDeviceGetUnit(virSCSIDevicePtr dev) +{ + return dev->unit; +} + +bool +virSCSIDeviceGetReadonly(virSCSIDevicePtr dev) +{ + return dev->readonly; +} + +int +virSCSIDeviceFileIterate(virSCSIDevicePtr dev, + virSCSIDeviceFileActor actor, + void *opaque) +{ + return (actor)(dev, dev->path, opaque); +} + +virSCSIDeviceListPtr +virSCSIDeviceListNew(void) +{ + virSCSIDeviceListPtr list; + + if (virSCSIInitialize() < 0) + return NULL; + + if (!(list = virObjectLockableNew(virSCSIDeviceListClass))) + return NULL; + + return list; +} + +static void +virSCSIDeviceListDispose(void *obj) +{ + virSCSIDeviceListPtr list = obj; + int i; + + for (i = 0; i < list->count; i++) + virSCSIDeviceFree(list->devs[i]); + + VIR_FREE(list->devs); +} + +int +virSCSIDeviceListAdd(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + if (virSCSIDeviceListFind(list, dev)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device %s already exists"), + dev->name); + return -1; + } + + if (VIR_REALLOC_N(list->devs, list->count + 1) < 0) { + virReportOOMError(); + return -1; + } + + list->devs[list->count++] = dev; + + return 0; +} + +virSCSIDevicePtr +virSCSIDeviceListGet(virSCSIDeviceListPtr list, int idx) +{ + if (idx >= list->count || idx < 0) + return NULL; + + return list->devs[idx]; +} + +int +virSCSIDeviceListCount(virSCSIDeviceListPtr list) +{ + return list->count; +} + +virSCSIDevicePtr +virSCSIDeviceListSteal(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + virSCSIDevicePtr ret = NULL; + int i; + + for (i = 0; i < list->count; i++) { + if (list->devs[i]->adapter != dev->adapter || + list->devs[i]->bus != dev->bus || + list->devs[i]->target != dev->target || + list->devs[i]->unit != dev->unit) + continue; + + ret = list->devs[i]; + + if (i != list->count--) + memmove(&list->devs[i], + &list->devs[i+1], + sizeof(*list->devs) * (list->count - i)); Just checking math and auto decrement order and making sure you get the result you want. It's the for loop end value decrement inside the loop which initially got my attention. If "i=1" and "count=2" prior to the "if", then once we get here the 3rd arg would be 0 (zero), right?

It seems you tried to "steal" what virPCIDeviceListSteal() did, but avoided or tried to include the virPCIDeviceListFindIndex() functionality... Perhaps you should make use of the virSCSIDeviceListFind() just like the PCI code.

As said in commit log, I'm thinking about having a common enough class for the list, or may be in another way, but it will be later patch, cleaning up vir{pci,usb}.[ch] together. Osier

On 13/05/13 18:42, Osier Yang wrote:

On 07/05/13 19:09, Osier Yang wrote:

...

On 07/05/13 01:20, John Ferlan wrote:

...

On 05/03/2013 02:07 PM, Osier Yang wrote:

...

From: Han Cheng <hanc.fnst@cn.fujitsu.com>

This patch adds util functions for scsi hostdev.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> Signed-off-by: Osier Yang <jyang@redhat.com>

--- v3 - v4: * Use strdup instead of virAsprintf if the format string is "%s"

v2.5 - v3: * A couple "char *" to "const char *" * s/unsigned int readonly/bool readonly/ * Use STRPREIFX instead of the wrong use of STRSKIP (which can introduce bug) for virSCSIDeviceGetAdapterId. * Improve virSCSIDeviceNew

I'm not fan of introducing another list like virPCIDeviceList, which brought much trouble for us, but it doesn't hurt much to do clean up together later, since they are very similar. --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 22 +++ src/util/virscsi.c | 408 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virscsi.h | 84 ++++++++++ 5 files changed, 516 insertions(+) create mode 100644 src/util/virscsi.c create mode 100644 src/util/virscsi.h

I'll start by noting, I'm still not fully aware of all the "norms" regarding all that needs to be touched when adding a new file and all that needs to be done when adding new API's - so I could miss something that perhaps someone else will pick up on...

...

diff --git a/po/POTFILES.in b/po/POTFILES.in index bf5a864..f3ea4da 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -174,6 +174,7 @@ src/util/virportallocator.c src/util/virprocess.c src/util/virrandom.c src/util/virsexpr.c +src/util/virscsi.c src/util/virsocketaddr.c src/util/virstatslinux.c src/util/virstoragefile.c diff --git a/src/Makefile.am b/src/Makefile.am index 299b8fd..e71975a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -111,6 +111,7 @@ UTIL_SOURCES = \ util/virportallocator.c util/virportallocator.h \ util/virprocess.c util/virprocess.h \ util/virrandom.h util/virrandom.c \ + util/virscsi.c util/virscsi.h \ util/virsexpr.c util/virsexpr.h \ util/virsocketaddr.h util/virsocketaddr.c \ util/virstatslinux.c util/virstatslinux.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 98660dc..64e2816 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1687,6 +1687,28 @@ virRandomGenerateWWN; virRandomInt; +# util/virscsi.h +virSCSIDeviceFileIterate; +virSCSIDeviceFree; +virSCSIDeviceGetAdapter; +virSCSIDeviceGetBus; +virSCSIDeviceGetDevStr; +virSCSIDeviceGetName; +virSCSIDeviceGetReadonly; +virSCSIDeviceGetTarget; +virSCSIDeviceGetUnit; +virSCSIDeviceGetUsedBy; +virSCSIDeviceListAdd; +virSCSIDeviceListCount; +virSCSIDeviceListDel; +virSCSIDeviceListFind; +virSCSIDeviceListGet; +virSCSIDeviceListNew; +virSCSIDeviceListSteal; +virSCSIDeviceNew; +virSCSIDeviceSetUsedBy; + + # util/virsexpr.h sexpr2string; sexpr_append; diff --git a/src/util/virscsi.c b/src/util/virscsi.c new file mode 100644 index 0000000..2d6bd8c --- /dev/null +++ b/src/util/virscsi.c @@ -0,0 +1,408 @@ +/* + * virscsi.c: helper APIs for managing host SCSI devices + * + * Copyright (C) 2013 Fujitsu, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Han Cheng <hanc.fnst@cn.fujitsu.com> + */ + +#include <config.h> + +#include <dirent.h> +#include <fcntl.h> +#include <inttypes.h> +#include <limits.h> +#include <stdio.h> +#include <string.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> + +#include "virscsi.h" +#include "virlog.h" +#include "viralloc.h" +#include "virutil.h" +#include "virstring.h" +#include "virerror.h" + +#define SYSFS_SCSI_DEVICES "/sys/bus/scsi/devices" + +/* For virReportOOMError() and virReportSystemError() */ +#define VIR_FROM_THIS VIR_FROM_NONE + +struct _virSCSIDevice { + unsigned int adapter; + unsigned int bus; + unsigned int target; + unsigned int unit; + + char *name; /* adapter:bus:target:unit */ + char *id; /* model:vendor */ + char *path; + const char *used_by; /* name of the domain using this dev */ + + bool readonly; +}; + +struct _virSCSIDeviceList { + virObjectLockable parent; + unsigned int count; + virSCSIDevicePtr *devs; +}; + +static virClassPtr virSCSIDeviceListClass; + +static void virSCSIDeviceListDispose(void *obj); + +static int +virSCSIOnceInit(void) +{ + if (!(virSCSIDeviceListClass = virClassNew(virClassForObjectLockable(), + "virSCSIDeviceList", + sizeof(virSCSIDeviceList), + virSCSIDeviceListDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virSCSI) + +static int +virSCSIDeviceGetAdapterId(const char *adapter, + unsigned int *adapter_id) +{ + if (STRPREFIX(adapter, "scsi_host")) { + if (virStrToLong_ui(adapter + strlen("scsi_host"), + NULL, 0, adapter_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse adapter '%s'"), adapter); + return -1; + } + } + hmm... which makes me think, does the description in patch 1/25 need to be adjusted to indicate that "scsi_host" is expected as part of the adapter name? and that adapters are thus identified by the numeric value after the prefix?

No, forcely checking it when parsing is not a good idea, just for QEMU/KVM driver, we want it has a prefix "scsi_host", it might be different for other drivers, though currently we only support qemu driver.

Even checking it here is not that good, but given that this helper is only used by qemu driver now, I think it's fine, it can be changed if need in future.

...

...

+ return 0; +} + +char * +virSCSIDeviceGetDevStr(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit) +{ + DIR *dir = NULL; + struct dirent *entry; + char *path = NULL; + char *sg = NULL; + unsigned int adapter_id; + + if (virSCSIDeviceGetAdapterId(adapter, &adapter_id) < 0) + return NULL; + + if (virAsprintf(&path, + SYSFS_SCSI_DEVICES "/%d:%d:%d:%d/scsi_generic", + adapter_id, bus, target, unit) < 0) { + virReportOOMError(); + return NULL; + } + + if (!(dir = opendir(path))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open %s"), path); + goto cleanup; + } + + while ((entry = readdir(dir))) { + if (entry->d_name[0] == '.') + continue; + + if (!(sg = strdup(entry->d_name))) { + virReportOOMError(); + goto cleanup; + } + } + +cleanup: + closedir(dir); + VIR_FREE(path); + return sg; +} + +virSCSIDevicePtr +virSCSIDeviceNew(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit, + bool readonly) +{ + virSCSIDevicePtr dev, ret = NULL; + char *sg = NULL; + char *vendor_path = NULL; + char *model_path = NULL; + char *vendor = NULL; + char *model = NULL; + + if (VIR_ALLOC(dev) < 0) { + virReportOOMError(); + return NULL; + } + + dev->bus = bus; + dev->target = target; + dev->unit = unit; + dev->readonly = readonly; + + if (!(sg = virSCSIDeviceGetDevStr(adapter, bus, target, unit))) + goto cleanup; 'sg' is now strdup()'d memory

...

+ + if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) + goto cleanup; + + if (virAsprintf(&dev->name, "%d:%d:%d:%d", dev->adapter, + dev->bus, dev->bus, dev->unit) < 0 || + virAsprintf(&dev->path, "/dev/%s", sg) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (access(dev->path, F_OK) != 0) { + virReportSystemError(errno, + _("Device %s not found: could not access %s"), + dev->name, dev->path); Is it really 'not found'? Perhaps "SCSI device '%s': could not access '%s'"?

Agreed

...

...

+ goto cleanup; + } + + if (virAsprintf(&vendor_path, + SYSFS_SCSI_DEVICES "/%s/vendor", dev->name) < 0 || + virAsprintf(&model_path, + SYSFS_SCSI_DEVICES "/%s/model", dev->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virFileReadAll(vendor_path, 1024, &vendor) < 0) + goto cleanup; + + if (virFileReadAll(model_path, 1024, &model) < 0) + goto cleanup; + + virTrimSpaces(vendor, NULL); + virTrimSpaces(model, NULL); + + if (virAsprintf(&dev->id, "%s:%s", vendor, model) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret = dev; +cleanup: VIR_FREE(sg);

...

+ VIR_FREE(vendor); + VIR_FREE(model); + VIR_FREE(vendor_path); + VIR_FREE(model_path); + if (!ret) + virSCSIDeviceFree(dev); + return ret; +} + +void +virSCSIDeviceFree(virSCSIDevicePtr dev) +{ + if (!dev) + return; + + VIR_FREE(dev->id); + VIR_FREE(dev->name); + VIR_FREE(dev->path); + VIR_FREE(dev); +} + +void +virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, + const char *name) +{ + dev->used_by = name; +} + +const char * +virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev) +{ + return dev->used_by; +} + +const char * +virSCSIDeviceGetName(virSCSIDevicePtr dev) +{ + return dev->name; +} + +unsigned int +virSCSIDeviceGetAdapter(virSCSIDevicePtr dev) +{ + return dev->adapter; +} + +unsigned int +virSCSIDeviceGetBus(virSCSIDevicePtr dev) +{ + return dev->bus; +} + +unsigned int +virSCSIDeviceGetTarget(virSCSIDevicePtr dev) +{ + return dev->target; +} + +unsigned int +virSCSIDeviceGetUnit(virSCSIDevicePtr dev) +{ + return dev->unit; +} + +bool +virSCSIDeviceGetReadonly(virSCSIDevicePtr dev) +{ + return dev->readonly; +} + +int +virSCSIDeviceFileIterate(virSCSIDevicePtr dev, + virSCSIDeviceFileActor actor, + void *opaque) +{ + return (actor)(dev, dev->path, opaque); +} + +virSCSIDeviceListPtr +virSCSIDeviceListNew(void) +{ + virSCSIDeviceListPtr list; + + if (virSCSIInitialize() < 0) + return NULL; + + if (!(list = virObjectLockableNew(virSCSIDeviceListClass))) + return NULL; + + return list; +} + +static void +virSCSIDeviceListDispose(void *obj) +{ + virSCSIDeviceListPtr list = obj; + int i; + + for (i = 0; i < list->count; i++) + virSCSIDeviceFree(list->devs[i]); + + VIR_FREE(list->devs); +} + +int +virSCSIDeviceListAdd(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + if (virSCSIDeviceListFind(list, dev)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device %s already exists"), + dev->name); + return -1; + } + + if (VIR_REALLOC_N(list->devs, list->count + 1) < 0) { + virReportOOMError(); + return -1; + } + + list->devs[list->count++] = dev; + + return 0; +} + +virSCSIDevicePtr +virSCSIDeviceListGet(virSCSIDeviceListPtr list, int idx) +{ + if (idx >= list->count || idx < 0) + return NULL; + + return list->devs[idx]; +} + +int +virSCSIDeviceListCount(virSCSIDeviceListPtr list) +{ + return list->count; +} + +virSCSIDevicePtr +virSCSIDeviceListSteal(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + virSCSIDevicePtr ret = NULL; + int i; + + for (i = 0; i < list->count; i++) { + if (list->devs[i]->adapter != dev->adapter || + list->devs[i]->bus != dev->bus || + list->devs[i]->target != dev->target || + list->devs[i]->unit != dev->unit) + continue; + + ret = list->devs[i]; + + if (i != list->count--) + memmove(&list->devs[i], + &list->devs[i+1], + sizeof(*list->devs) * (list->count - i)); Just checking math and auto decrement order and making sure you get the result you want. It's the for loop end value decrement inside the loop which initially got my attention. If "i=1" and "count=2" prior to the "if", then once we get here the 3rd arg would be 0 (zero), right?

It seems you tried to "steal" what virPCIDeviceListSteal() did, but avoided or tried to include the virPCIDeviceListFindIndex() functionality... Perhaps you should make use of the virSCSIDeviceListFind() just like the PCI code.

As said in commit log, I'm thinking about having a common enough class for the list, or may be in another way, but it will be later patch, cleaning up vir{pci,usb}.[ch] together.

Pushed with the ACK'ed nits fixed.

With 15/25 merged.

On 05/03/2013 02:07 PM, Osier Yang wrote:

From: Han Cheng <hanc.fnst@cn.fujitsu.com>

Except the scsi host device's controller is "lsilogic", mapping between the libvirt attributes and scsi-generic properties is:

libvirt qemu ----------------------------------------- controller bus ($libvirt_controller.0) bus channel target scsi-id unit lun

For scsi host device with "lsilogic" controller, the mapping is: ('target (libvirt)' must be 0, as it's not used; 'unit (libvirt) must <= 7).

libvirt qemu ---------------------------------------------------------- controller && bus bus ($libvirt_controller.$libvirt_bus) unit scsi-id

Might be nice to include this mapping in the code comments especially for those looking for this type of information some day that may not think to look in a git commit log...

It's not good to hardcode/hard-check limits of these attributes, and even worse, these limits are not documented, one has to find out by either testing or reading the qemu code, I'm looking forward to qemu expose limits like these one day). For example, exposing "max_target", "max_lun" for megasas:

static const struct SCSIBusInfo megasas_scsi_info = { .tcq = true, .max_target = MFI_MAX_LD, .max_lun = 255,

.transfer_data = megasas_xfer_complete, .get_sg_list = megasas_get_sg_list, .complete = megasas_command_complete, .cancel = megasas_command_cancel, };

Example of the qemu command line (lsilogic controller):

-drive file=/dev/sg2,if=none,id=drive-hostdev-scsi_host7-0-0-0 \ -device scsi-generic,bus=scsi0.0,scsi-id=8,\ drive=drive-hostdev-scsi_host7-0-0-0,id=hostdev-scsi_host7-0-0-0

Example of the qemu command line (virtio-scsi controller):

-drive file=/dev/sg2,if=none,id=drive-hostdev-scsi_host7-0-0-0 \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=128,lun=128,\ drive=drive-hostdev-scsi_host7-0-0-0,id=hostdev-scsi_host7-0-0-0

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> Signed-off-by: Osier Yang <jyang@redhat.com>

--- v3 - v4: * Remove checking for "bug == 0" * Split "bootindex" and "readonly" support into another patch

v2.5 - v3: * Add support for all other controllers, but not only virtio-scsi * Add checking for "bus == 0" * Add checking for "target == 0" && "unit <= 7" for scsi host device which is on "lsilogic" controller. * Integrate xml2argv test from 10/10 of v2.5 into this patch --- src/qemu/qemu_command.c | 132 ++++++++++++++++++++- src/qemu/qemu_command.h | 6 + .../qemuxml2argv-hostdev-scsi-lsi.args | 9 ++ .../qemuxml2argv-hostdev-scsi-lsi.xml | 35 ++++++ .../qemuxml2argv-hostdev-scsi-virtio-scsi.args | 9 ++ ...l => qemuxml2argv-hostdev-scsi-virtio-scsi.xml} | 0 tests/qemuxml2argvtest.c | 9 ++ tests/qemuxml2xmltest.c | 3 +- 8 files changed, 199 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-scsi.args rename tests/qemuxml2argvdata/{qemuxml2argv-hostdev-scsi.xml => qemuxml2argv-hostdev-scsi-virtio-scsi.xml} (100%)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 575dce1..df896aa 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -48,6 +48,7 @@ #include "device_conf.h" #include "virstoragefile.h" #include "virtpm.h" +#include "virscsi.h" #if defined(__linux__) # include <linux/capability.h> #endif @@ -745,7 +746,16 @@ qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev } }

- if (virAsprintf(&hostdev->info->alias, "hostdev%d", idx) < 0) { + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + if (virAsprintf(&hostdev->info->alias, "hostdev-%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) { + virReportOOMError(); + return -1; + } + } else if (virAsprintf(&hostdev->info->alias, "hostdev%d", idx) < 0) { virReportOOMError(); return -1; } @@ -4673,7 +4683,96 @@ qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev) return ret; }

+char * +qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *sg = NULL; + + if (!(sg = virSCSIDeviceGetDevStr(dev->source.subsys.u.scsi.adapter, + dev->source.subsys.u.scsi.bus, + dev->source.subsys.u.scsi.target, + dev->source.subsys.u.scsi.unit))) { + goto error; + } + + virBufferAsprintf(&buf, "file=/dev/%s,if=none", sg); + virBufferAsprintf(&buf, ",id=%s-%s", + virDomainDeviceAddressTypeToString(dev->info->type), + dev->info->alias); + + if (virBufferError(&buf)) { + virReportOOMError(); + goto error; + } +

VIR_FREE(sg); No comments on remainder. I assume the test config is good - at least it's there! ACK John

+ return virBufferContentAndReset(&buf); +error: + VIR_FREE(sg); + virBufferFreeAndReset(&buf); + return NULL; +} + +char * +qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int model = -1; + + model = virDomainDeviceFindControllerModel(def, dev->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + + if (qemuSetScsiControllerModel(def, qemuCaps, &model) < 0) + goto error; + + if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC) { + if (dev->info->addr.drive.target != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("target must be 0 for scsi host device " + "if its controller model is 'lsilogic'")); + goto error; + } + + if (dev->info->addr.drive.unit > 7) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unit must be not more than 7 for scsi host " + "device if its controller model is 'lsilogic'")); + goto error; + } + } + + virBufferAsprintf(&buf, "scsi-generic"); + + if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC) { + virBufferAsprintf(&buf, ",bus=scsi%d.%d,scsi-id=%d", + dev->info->addr.drive.controller, + dev->info->addr.drive.bus, + dev->info->addr.drive.unit); + } else { + virBufferAsprintf(&buf, ",bus=scsi%d.0,channel=%d,scsi-id=%d,lun=%d", + dev->info->addr.drive.controller, + dev->info->addr.drive.bus, + dev->info->addr.drive.target, + dev->info->addr.drive.unit); + }

+ virBufferAsprintf(&buf, ",drive=%s-%s,id=%s", + virDomainDeviceAddressTypeToString(dev->info->type), + dev->info->alias, dev->info->alias); + + if (virBufferError(&buf)) { + virReportOOMError(); + goto error; + } + + return virBufferContentAndReset(&buf); +error: + virBufferFreeAndReset(&buf); + return NULL; +}

/* This function outputs a -chardev command line option which describes only the * host side of the character device */ @@ -7927,10 +8026,11 @@ qemuBuildCommandLine(virConnectPtr conn, if (hostdev->info->bootIndex) { if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && - hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)) { + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB && + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("booting from assigned devices is only " - "supported for PCI and USB devices")); + "supported for PCI, USB and SCSI devices")); goto error; } else { if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { @@ -8039,6 +8139,32 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } } + + /* SCSI */ + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { + char *drvstr; + + virCommandAddArg(cmd, "-drive"); + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps))) + goto error; + virCommandAddArg(cmd, drvstr); + VIR_FREE(drvstr); + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev, qemuCaps))) + goto error; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SCSI passthrough is not supported by this version of qemu")); + goto error; + } + } }

/* Migration is very annoying due to wildly varying syntax & diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index a706942..9e4975d 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -138,6 +138,12 @@ char * qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev); char * qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps);

+char * qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps); +char * qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps); + char * qemuBuildHubDevStr(virDomainHubDefPtr dev, virQEMUCapsPtr qemuCaps); char * qemuBuildRedirdevDevStr(virDomainDefPtr def, virDomainRedirdevDefPtr dev, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.args new file mode 100644 index 0000000..06f7938 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.args @@ -0,0 +1,9 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc \ +-m 214 -smp 1 -nographic -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device lsi,id=scsi0,bus=pci.0,addr=0x3 -usb \ +-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive file=/dev/sg0,if=none,id=drive-hostdev-scsi_host0-0-0-0 \ +-device scsi-generic,bus=scsi0.0,scsi-id=7,drive=drive-hostdev-scsi_host0-0-0-0,id=hostdev-scsi_host0-0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.xml new file mode 100644 index 0000000..98c469c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/>72ae3d932c0119a3e0f8a9175b8e46977cae046a + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='0'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='7'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-scsi.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-scsi.args new file mode 100644 index 0000000..b92afc7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-scsi.args @@ -0,0 +1,9 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 -usb \ +-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive file=/dev/sg0,if=none,id=drive-hostdev-scsi_host0-0-0-0 \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,drive=drive-hostdev-scsi_host0-0-0-0,id=hostdev-scsi_host0-0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-scsi.xml similarity index 100% rename from tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml rename to tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-scsi.xml diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1286273..abe04b3 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -974,6 +974,15 @@ mymain(void) DO_TEST("pci-autoadd-addr", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE);

+ DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE, + QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC); + DO_TEST("hostdev-scsi-virtio-scsi", QEMU_CAPS_DRIVE, + QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC); + virObjectUnref(driver.config); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 1ca1f7e..08c3eeb 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -285,7 +285,8 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx");

- DO_TEST("hostdev-scsi"); + DO_TEST("hostdev-scsi-lsi"); + DO_TEST("hostdev-scsi-virtio-scsi");

virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt);

On 07/05/13 01:37, John Ferlan wrote:

On 05/03/2013 02:07 PM, Osier Yang wrote:

...

From: Han Cheng <hanc.fnst@cn.fujitsu.com>

Except the scsi host device's controller is "lsilogic", mapping between the libvirt attributes and scsi-generic properties is:

libvirt qemu ----------------------------------------- controller bus ($libvirt_controller.0) bus channel target scsi-id unit lun

For scsi host device with "lsilogic" controller, the mapping is: ('target (libvirt)' must be 0, as it's not used; 'unit (libvirt) must <= 7).

libvirt qemu ---------------------------------------------------------- controller && bus bus ($libvirt_controller.$libvirt_bus) unit scsi-id

Might be nice to include this mapping in the code comments especially for those looking for this type of information some day that may not think to look in a git commit log...

...

It's not good to hardcode/hard-check limits of these attributes, and even worse, these limits are not documented, one has to find out by either testing or reading the qemu code, I'm looking forward to qemu expose limits like these one day). For example, exposing "max_target", "max_lun" for megasas:

static const struct SCSIBusInfo megasas_scsi_info = { .tcq = true, .max_target = MFI_MAX_LD, .max_lun = 255,

.transfer_data = megasas_xfer_complete, .get_sg_list = megasas_get_sg_list, .complete = megasas_command_complete, .cancel = megasas_command_cancel, };

Example of the qemu command line (lsilogic controller):

-drive file=/dev/sg2,if=none,id=drive-hostdev-scsi_host7-0-0-0 \ -device scsi-generic,bus=scsi0.0,scsi-id=8,\ drive=drive-hostdev-scsi_host7-0-0-0,id=hostdev-scsi_host7-0-0-0

Example of the qemu command line (virtio-scsi controller):

-drive file=/dev/sg2,if=none,id=drive-hostdev-scsi_host7-0-0-0 \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=128,lun=128,\ drive=drive-hostdev-scsi_host7-0-0-0,id=hostdev-scsi_host7-0-0-0

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> Signed-off-by: Osier Yang <jyang@redhat.com>

--- v3 - v4: * Remove checking for "bug == 0" * Split "bootindex" and "readonly" support into another patch

v2.5 - v3: * Add support for all other controllers, but not only virtio-scsi * Add checking for "bus == 0" * Add checking for "target == 0" && "unit <= 7" for scsi host device which is on "lsilogic" controller. * Integrate xml2argv test from 10/10 of v2.5 into this patch --- src/qemu/qemu_command.c | 132 ++++++++++++++++++++- src/qemu/qemu_command.h | 6 + .../qemuxml2argv-hostdev-scsi-lsi.args | 9 ++ .../qemuxml2argv-hostdev-scsi-lsi.xml | 35 ++++++ .../qemuxml2argv-hostdev-scsi-virtio-scsi.args | 9 ++ ...l => qemuxml2argv-hostdev-scsi-virtio-scsi.xml} | 0 tests/qemuxml2argvtest.c | 9 ++ tests/qemuxml2xmltest.c | 3 +- 8 files changed, 199 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-scsi.args rename tests/qemuxml2argvdata/{qemuxml2argv-hostdev-scsi.xml => qemuxml2argv-hostdev-scsi-virtio-scsi.xml} (100%)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 575dce1..df896aa 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -48,6 +48,7 @@ #include "device_conf.h" #include "virstoragefile.h" #include "virtpm.h" +#include "virscsi.h" #if defined(__linux__) # include <linux/capability.h> #endif @@ -745,7 +746,16 @@ qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev } }

- if (virAsprintf(&hostdev->info->alias, "hostdev%d", idx) < 0) { + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + if (virAsprintf(&hostdev->info->alias, "hostdev-%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) { + virReportOOMError(); + return -1; + } + } else if (virAsprintf(&hostdev->info->alias, "hostdev%d", idx) < 0) { virReportOOMError(); return -1; } @@ -4673,7 +4683,96 @@ qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev) return ret; }

+char * +qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *sg = NULL; + + if (!(sg = virSCSIDeviceGetDevStr(dev->source.subsys.u.scsi.adapter, + dev->source.subsys.u.scsi.bus, + dev->source.subsys.u.scsi.target, + dev->source.subsys.u.scsi.unit))) { + goto error; + } + + virBufferAsprintf(&buf, "file=/dev/%s,if=none", sg); + virBufferAsprintf(&buf, ",id=%s-%s", + virDomainDeviceAddressTypeToString(dev->info->type), + dev->info->alias); + + if (virBufferError(&buf)) { + virReportOOMError(); + goto error; + } + VIR_FREE(sg);

No comments on remainder. I assume the test config is good - at least it's there!

Fixed, and pushed.

On 05/03/2013 02:07 PM, Osier Yang wrote:

Since it's generic enough to be used by other types in future, I put it in <hostdev> as sub-element, though now it's only used by scsi host device. --- docs/formatdomain.html.in | 4 +++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 6 ++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 4 +++ .../qemuxml2argv-hostdev-scsi-readonly.args | 9 ++++++ .../qemuxml2argv-hostdev-scsi-readonly.xml | 36 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 70 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 488673f..9cd79e5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2406,6 +2406,10 @@ could be changed in the future with no impact to domains that don't specify anything. </dd> + <dt><code>readonly</code></dt> + <dd>Indicates that the device is readonly, only supported by SCSI host + device now. <span class="since">Since 1.0.6</span> + </dd>

I just realized your example in 1/25 has the 'readonly' element listed in the new "<pre> ... </pre> section, but it's not described. I think you probably need to split that line out of there and then put it into this patch. Does anything need to be said here that the underlying QEMU needs to support the readonly too? That is some version or capability? Since there is a check later on to actually pass/add readonly to the command line, it'd surely raise a question some day if someone defined it as readonly, but then come to find out the domain didn't honor that...

</dl>

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b8d0d60..4fdacab 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3075,6 +3075,11 @@ <optional> <ref name="address"/> </optional> + <optional> + <element name="readonly"> + <empty/> + </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9e6b65b..31a8c46 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8724,6 +8724,9 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, _("SCSI host devices must have address specified")); goto error; } + + if (virXPathBoolean("boolean(./readonly)", ctxt)) + def->readonly = true; break; } } @@ -15342,6 +15345,9 @@ virDomainHostdevDefFormat(virBufferPtr buf, return -1; break; } + + if (def->readonly) + virBufferAddLit(buf, "<readonly/>\n"); virBufferAdjustIndent(buf, -6);

if (virDomainDeviceInfoFormat(buf, def->info, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1efae69..5471cd3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -461,6 +461,7 @@ struct _virDomainHostdevDef { int startupPolicy; /* enum virDomainStartupPolicy */ bool managed; bool missing; + bool readonly; union { virDomainHostdevSubsys subsys; virDomainHostdevCaps caps; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index df896aa..2b141d1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4702,6 +4702,10 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, virDomainDeviceAddressTypeToString(dev->info->type), dev->info->alias);

+ if (dev->readonly && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) + virBufferAsprintf(&buf, ",readonly=on"); +

See comment above. If this is defined as readonly, but the underlying system doesn't support it, then should we fail? It'd be bad news to have a domain expect something readonly, but still load and potentially write on something it wasn't supposed to. If there's a precedent already that allows a defined readonly element to be used read/write, then I guess it's a weak ACK, but otherwise, I think it might be best to fail. John

if (virBufferError(&buf)) { virReportOOMError(); goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args new file mode 100644 index 0000000..ea2f7af --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args @@ -0,0 +1,9 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 -usb \ +-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive file=/dev/sg0,if=none,id=drive-hostdev-scsi_host0-0-0-0,readonly=on \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,drive=drive-hostdev-scsi_host0-0-0-0,id=hostdev-scsi_host0-0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml new file mode 100644 index 0000000..359bb95 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='0'/> + </source> + <readonly/> + <address type='drive' controller='0' bus='0' target='4' unit='8'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index abe04b3..4d7f5e2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -982,6 +982,10 @@ mymain(void) QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); + DO_TEST("hostdev-scsi-readonly", QEMU_CAPS_DRIVE, + QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, + QEMU_CAPS_DRIVE_READONLY, QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC);

virObjectUnref(driver.config); virObjectUnref(driver.caps); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 08c3eeb..492ac60 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -287,6 +287,7 @@ mymain(void)

DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); + DO_TEST("hostdev-scsi-readonly");

virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt);

On 07/05/13 18:58, Osier Yang wrote:

On 07/05/13 02:01, John Ferlan wrote:

...

...

Since it's generic enough to be used by other types in future, I put it in <hostdev> as sub-element, though now it's only used by scsi host device. --- docs/formatdomain.html.in | 4 +++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 6 ++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 4 +++ .../qemuxml2argv-hostdev-scsi-readonly.args | 9 ++++++ .../qemuxml2argv-hostdev-scsi-readonly.xml | 36 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 70 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 488673f..9cd79e5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2406,6 +2406,10 @@ could be changed in the future with no impact to domains that don't specify anything. </dd> + <dt><code>readonly</code></dt> + <dd>Indicates that the device is readonly, only supported by SCSI host + device now. <span class="since">Since 1.0.6</span> + </dd> I just realized your example in 1/25 has the 'readonly' element listed in the new "<pre> ... </pre> section, but it's not described. I think you probably need to split that line out of there and then put it into

On 05/03/2013 02:07 PM, Osier Yang wrote: this patch.

Yeah, I missed it, should remove it in 1/25.

...

Does anything need to be said here that the underlying QEMU needs to support the readonly too? That is some version or capability? Since there is a check later on to actually pass/add readonly to the command line, it'd surely raise a question some day if someone defined it as readonly, but then come to find out the domain didn't honor that...

Agreed. QEMU/KVM only.

...

...

</dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b8d0d60..4fdacab 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3075,6 +3075,11 @@ <optional> <ref name="address"/> </optional> + <optional> + <element name="readonly"> + <empty/> + </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9e6b65b..31a8c46 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8724,6 +8724,9 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, _("SCSI host devices must have address specified")); goto error; } + + if (virXPathBoolean("boolean(./readonly)", ctxt)) + def->readonly = true; break; } } @@ -15342,6 +15345,9 @@ virDomainHostdevDefFormat(virBufferPtr buf, return -1; break; } + + if (def->readonly) + virBufferAddLit(buf, "<readonly/>\n"); virBufferAdjustIndent(buf, -6); if (virDomainDeviceInfoFormat(buf, def->info, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1efae69..5471cd3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -461,6 +461,7 @@ struct _virDomainHostdevDef { int startupPolicy; /* enum virDomainStartupPolicy */ bool managed; bool missing; + bool readonly; union { virDomainHostdevSubsys subsys; virDomainHostdevCaps caps; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index df896aa..2b141d1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4702,6 +4702,10 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, virDomainDeviceAddressTypeToString(dev->info->type), dev->info->alias); + if (dev->readonly && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) + virBufferAsprintf(&buf, ",readonly=on"); +

See comment above. If this is defined as readonly, but the underlying system doesn't support it, then should we fail? It'd be bad news to have a domain expect something readonly, but still load and potentially write on something it wasn't supposed to.

If there's a precedent already that allows a defined readonly element to be used read/write, then I guess it's a weak ACK, but otherwise, I think it might be best to fail.

We don't have a standdard yet, some attributes are just ignored if the underlying hypervisor doesn't support it, some will fail. Unfortunately, we can't change it simply to error out, as it will introduce "regression" from user's p.o.v, though from out p.o.v, it's improvement.

I will let it fail when pushing.

Pushed with the nits fixed.

On 07/05/13 18:57, John Ferlan wrote:

On 05/03/2013 02:07 PM, Osier Yang wrote:

...

From: Han Cheng <hanc.fnst@cn.fujitsu.com>

Although virtio-scsi supports SCSI PR, the device on host may do not support it. To avoid losing data, we only allow one scsi hostdev to be passthroughed to one live guest, Just like what we do for PCI and USB devices. What is "PR"? (Persistent Reservations?)

Yes, I should make it more clear.

s/To avoid losing data....USB devices/

"Just like PCI and USB pass through devices, only one live guest is allowed per SCSI host pass through device."

Okay.

...

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_driver.c | 3 + src/qemu/qemu_hostdev.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hostdev.h | 10 +++ src/qemu/qemu_process.c | 3 + 5 files changed, 231 insertions(+)

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 77d3d2f..30a2a22 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -36,6 +36,7 @@ # include "security/security_manager.h" # include "virpci.h" # include "virusb.h" +# include "virscsi.h" # include "cpu_conf.h" # include "driver.h" # include "virportallocator.h" @@ -203,6 +204,7 @@ struct _virQEMUDriver { virPCIDeviceListPtr activePciHostdevs; virPCIDeviceListPtr inactivePciHostdevs; virUSBDeviceListPtr activeUsbHostdevs; + virSCSIDeviceListPtr activeScsiHostdevs;

/* Immutable pointer. Unsafe APIs. XXX */ virHashTablePtr sharedDisks; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d5d7de3..b631a1f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -675,6 +675,9 @@ qemuStateInitialize(bool privileged, if ((qemu_driver->inactivePciHostdevs = virPCIDeviceListNew()) == NULL) goto error;

+ if (!(qemu_driver->activeScsiHostdevs = virSCSIDeviceListNew())) + goto error; + A nit, but keeping things with the same look and feel in code is sometimes easier rather than something that looks different, but accomplishes the same thing. So rather than !(), use the existing () == NULL)...

Personally I like "!", but I don't have a good reason to keep it, and keep consistence sounds good.

Furthermore a bunch of those if's could probably be put together, but that's outside of this set of changes... eg, if (cond1 || cond2 || cond3 || etc) goto error;

Another personal habit I think, though I prefer to put them together too.

...

if (!(qemu_driver->sharedDisks = virHashCreate(30, qemuSharedDiskEntryFree))) goto error;

diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index e4af036..d5f94d5 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -29,6 +29,7 @@ #include "viralloc.h" #include "virpci.h" #include "virusb.h" +#include "virscsi.h" #include "virnetdev.h"

#define VIR_FROM_THIS VIR_FROM_QEMU @@ -226,6 +227,47 @@ cleanup: return ret; }

+int +qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + virDomainHostdevDefPtr hostdev = NULL; + int i; + int ret = -1; + + if (!def->nhostdevs) + return 0; + + virObjectLock(driver->activeScsiHostdevs); + for (i = 0; i < def->nhostdevs; i++) { + virSCSIDevicePtr scsi = NULL; + hostdev = def->hostdevs[i]; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + continue; + + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + hostdev->readonly))) + goto cleanup;

I see that the qemuUpdateActiveUsbHostdevs() had a VIR_WARN before the goto cleanup - something you may want to consider here as well.

The VIR_WARN is useless, as virSCSIDeviceNew has errors.

...

+ + virSCSIDeviceSetUsedBy(scsi, def->name); + + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) { + virSCSIDeviceFree(scsi); + goto cleanup; + } + } + ret = 0; + +cleanup: + virObjectUnlock(driver->activeScsiHostdevs); + return ret; +} + static int qemuDomainHostdevPciSysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) { @@ -827,6 +869,107 @@ cleanup: return ret; }

+int +qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + int i, j, count; + virSCSIDeviceListPtr list; + virSCSIDevicePtr tmp; + + /* To prevent situation where SCSI device is assigned to two domains + * we need to keep a list of currently assigned SCSI devices. + * This is done in several loops which cannot be joined into one big + * loop. See qemuPrepareHostdevPCIDevices() + */ + if (!(list = virSCSIDeviceListNew())) + goto cleanup; + + /* Loop 1: build temporary list */ And I see from PrepareHostDevices that nhostdevs is guaranteed to be non zero.... good...

...

+ for (i = 0 ; i < nhostdevs ; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + virSCSIDevicePtr scsi; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + continue; + + if (hostdev->managed) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("SCSI host device doesn't support managed mode")); + goto cleanup; + } + + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + hostdev->readonly))) + goto cleanup; + + if (scsi && virSCSIDeviceListAdd(list, scsi) < 0) { + virSCSIDeviceFree(scsi); + goto cleanup; + } + } + + /* Loop 2: Mark devices in temporary list as used by @name + * and add them to driver list. However, if something goes + * wrong, perform rollback. + */ + virObjectLock(driver->activeScsiHostdevs); + count = virSCSIDeviceListCount(list); + + for (i = 0; i < count; i++) { + virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i); + if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) { + const char *other_name = virSCSIDeviceGetUsedBy(tmp); + + if (other_name) + virReportError(VIR_ERR_OPERATION_INVALID, + _("SCSI device %s is in use by domain %s"), + virSCSIDeviceGetName(tmp), other_name); + else + virReportError(VIR_ERR_OPERATION_INVALID, + _("SCSI device %s is already in use"), + virSCSIDeviceGetName(tmp)); + goto error; + } + + virSCSIDeviceSetUsedBy(scsi, name); + VIR_DEBUG("Adding %s to activeScsiHostdevs", virSCSIDeviceGetName(scsi)); + + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) + goto error; + } + + virObjectUnlock(driver->activeScsiHostdevs); + + /* Loop 3: Temporary list was successfully merged with + * driver list, so steal all items to avoid freeing them + * when freeing temporary list. + */ + while (virSCSIDeviceListCount(list) > 0) { + tmp = virSCSIDeviceListGet(list, 0); + virSCSIDeviceListSteal(list, tmp); + } + + virObjectUnref(list); + return 0; + +error: + for (j = 0; j < i; j++) { + tmp = virSCSIDeviceListGet(list, i); + virSCSIDeviceListSteal(driver->activeScsiHostdevs, tmp); + } So what happens to the devices that were taken from 'list', had a 'name' associated with them on the active list? Don't they need to have the name now disassociated? And be removed from there?

It's not *taken* from list, I.E. list still keep the pointers, so they are just stealed (let's say detached) from activeList, and virObjectUnref will takes care of freeing them. That's why I don't like the vir{pci,usb}.[ch], it causes a lot of mess.

...

+ virObjectUnlock(driver->activeScsiHostdevs); +cleanup: If we get here from loop1, since the 'list' is potentially full of 'scsi' devices via virSCSIDeviceListAdd(), should there be a loop to remove those or does this Unref do that for you? Or is that what loop3 is supposed to do?

virObjectUnref will do it, virSCSIDeviceListDispose is involked. Osier

On 05/03/2013 02:07 PM, Osier Yang wrote:

It's better to put the usb related codes into qemuDomainAttachHostUsbDevice instead of qemuDomainAttachHostDevice.

And in the old qemuDomainAttachHostDevice, just stealing the "usb" from driver->activeUsbHostdevs leaks the memory.

Seems this is a bug fix that potentially could be separated from this patch stream... Right?

--- src/qemu/qemu_hotplug.c | 77 +++++++++++++++++++++---------------------------- 1 file changed, 33 insertions(+), 44 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a4f48b0..422d336 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1136,20 +1136,38 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - int ret; qemuDomainObjPrivatePtr priv = vm->privateData; + virUSBDeviceList *list = NULL; + virUSBDevicePtr usb = NULL; char *devstr = NULL; + bool added = false; + int ret = -1; + + if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) + return -1; + + if (!(list = virUSBDeviceListNew())) + goto cleanup; + + if (virUSBDeviceListAdd(list, usb) < 0) + goto cleanup;

How is cleanup affected if the next call fails? That is we have something on 'list', but not on the hostdev, so wouldn't the device need to be taken from there... Perhaps it's just Unref of list takes care of that - something I'm not yet clear on.

+ + if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0) + goto cleanup; + + added = true; + virUSBDeviceListSteal(list, usb);

if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) - goto error; + goto cleanup; if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev, priv->qemuCaps))) - goto error; + goto cleanup; }

if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) { virReportOOMError(); - goto error; + goto cleanup; }

qemuDomainObjEnterMonitor(driver, vm); @@ -1162,26 +1180,24 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm); virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); if (ret < 0) - goto error; + goto cleanup;

vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;

+ ret = 0; +cleanup: + if (added) + virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); + virUSBDeviceFree(usb); + virObjectUnref(list); VIR_FREE(devstr); - - return 0; - -error: - VIR_FREE(devstr); - return -1; + return ret; }

int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - virUSBDeviceList *list; - virUSBDevicePtr usb = NULL; - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev mode '%s' not supported"), @@ -1189,33 +1205,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, return -1; }

- if (!(list = virUSBDeviceListNew())) - goto cleanup; - - if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { - if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) - goto cleanup; - - if (virUSBDeviceListAdd(list, usb) < 0) { - virUSBDeviceFree(usb); - usb = NULL; - goto cleanup; - } - - if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0) { - usb = NULL; - goto cleanup; - } - - virUSBDeviceListSteal(list, usb); - } -

Moving the above hunk causes no "ordering" issues with the following two calls, right? Since you're placing USB devices on the hostdev list in the hunk and that won't happen until afterwards now thus the CGroup and HostdevLabel adjustments below will possibly have different results, right? John

if (qemuSetupHostdevCGroup(vm, hostdev) < 0) - goto cleanup; + return -1;

if (virSecurityManagerSetHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) - goto teardown_cgroup; + goto cleanup;

switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: @@ -1237,7 +1232,6 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, goto error; }

- virObjectUnref(list); return 0;

error: @@ -1245,14 +1239,9 @@ error: vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail");

-teardown_cgroup: +cleanup: if (qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); - -cleanup: - virObjectUnref(list); - if (usb) - virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); return -1; }

On 07/05/13 20:42, John Ferlan wrote:

...

From: Han Cheng <hanc.fnst@cn.fujitsu.com>

This adds both attachment and detachment support for scsi host device.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> Signed-off-by: Osier Yang <jyang@redhat> --- src/qemu/qemu_hotplug.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 136 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 422d336..e6bc3a2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1194,6 +1194,74 @@ cleanup: return ret; }

+static int +qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + char *devstr = NULL; + char *drvstr = NULL; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE) || + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) || + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SCSI passthrough is not supported by this version of qemu")); + return -1; + } + + if (qemuPrepareHostdevSCSIDevices(driver, vm->def->name, + &hostdev, 1)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to prepare scsi hostdev: %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); + return -1; + } + + if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, 0) < 0) + goto cleanup; + + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps))) + goto cleanup; + + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) + goto cleanup; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + qemuDomainObjEnterMonitor(driver, vm); + if ((ret = qemuMonitorAddDrive(priv->mon, drvstr)) == 0) { + if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) { + VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", + drvstr, devstr); + qemuMonitorDriveDel(priv->mon, drvstr); + } + } + qemuDomainObjExitMonitor(driver, vm); + + virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); It may be better to more closely follow code flow of other modules as I

On 05/03/2013 02:07 PM, Osier Yang wrote: think you could be missing a nuance of a failure mode by trying to be more generic. Just check all callers of AddDrive and AddDevice...

...

+ if (ret < 0) + goto cleanup; + + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; + + ret = 0; +cleanup: + if (ret < 0) + qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &hostdev, 1); + VIR_FREE(drvstr); + VIR_FREE(devstr); + return ret; +} + int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -1225,6 +1293,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, goto error; break;

+ case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (qemuDomainAttachHostScsiDevice(driver, vm, + hostdev) < 0) + goto error; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev subsys type '%s' not supported"), @@ -2441,11 +2515,59 @@ qemuDomainDetachHostUsbDevice(virQEMUDriverPtr driver, return ret; }

-static -int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainHostdevDefPtr detach, - int idx) +static int +qemuDomainDetachHostScsiDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *drvstr = NULL; + char *devstr = NULL; + int ret = -1; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias")); + return -1; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached with this QEMU version")); + return -1; + } + + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(detach, priv->qemuCaps))) + goto cleanup; + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, detach, priv->qemuCaps))) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + if ((ret = qemuMonitorDelDevice(priv->mon, detach->info->alias)) == 0) { + if ((ret = qemuMonitorDriveDel(priv->mon, drvstr)) < 0) { + VIR_WARN("qemuMonitorDriveDel failed on %s (%s)", + detach->info->alias, drvstr); + qemuMonitorAddDevice(priv->mon, devstr); Coverity complains right here about no checking of return status:

(9) Event check_return: Calling function "qemuMonitorAddDevice(qemuMonitorPtr, char const *)" without checking return value (as is done elsewhere 8 out of 9 times).

As with attach, the flow of this code is a bit different than other places where DelDevice() and DriveDel() are called - I would think you may want to follow those other models more closely...

I think you meant keeping the original error? here is the diff:

On 14/05/13 00:08, John Ferlan wrote:

On 05/13/2013 11:48 AM, Osier Yang wrote:

...

On 07/05/13 20:42, John Ferlan wrote:

...

...

From: Han Cheng <hanc.fnst@cn.fujitsu.com>

This adds both attachment and detachment support for scsi host device.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> Signed-off-by: Osier Yang <jyang@redhat> --- src/qemu/qemu_hotplug.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 136 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 422d336..e6bc3a2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1194,6 +1194,74 @@ cleanup: return ret; } +static int +qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + char *devstr = NULL; + char *drvstr = NULL; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE) || + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) || + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SCSI passthrough is not supported by this version of qemu")); + return -1; + } + + if (qemuPrepareHostdevSCSIDevices(driver, vm->def->name, + &hostdev, 1)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to prepare scsi hostdev: %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); + return -1; + } + + if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, 0) < 0) + goto cleanup; + + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps))) + goto cleanup; + + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) + goto cleanup; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + qemuDomainObjEnterMonitor(driver, vm); + if ((ret = qemuMonitorAddDrive(priv->mon, drvstr)) == 0) { + if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) { + VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", + drvstr, devstr); + qemuMonitorDriveDel(priv->mon, drvstr); + } + } + qemuDomainObjExitMonitor(driver, vm); + + virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); It may be better to more closely follow code flow of other modules as I

On 05/03/2013 02:07 PM, Osier Yang wrote: think you could be missing a nuance of a failure mode by trying to be more generic. Just check all callers of AddDrive and AddDevice...

...

+ if (ret < 0) + goto cleanup; + + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; + + ret = 0; +cleanup: + if (ret < 0) + qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &hostdev, 1); + VIR_FREE(drvstr); + VIR_FREE(devstr); + return ret; +} + int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -1225,6 +1293,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (qemuDomainAttachHostScsiDevice(driver, vm, + hostdev) < 0) + goto error; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev subsys type '%s' not supported"), @@ -2441,11 +2515,59 @@ qemuDomainDetachHostUsbDevice(virQEMUDriverPtr driver, return ret; } -static -int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainHostdevDefPtr detach, - int idx) +static int +qemuDomainDetachHostScsiDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *drvstr = NULL; + char *devstr = NULL; + int ret = -1; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias")); + return -1; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached with this QEMU version")); + return -1; + } + + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(detach, priv->qemuCaps))) + goto cleanup; + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, detach, priv->qemuCaps))) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + if ((ret = qemuMonitorDelDevice(priv->mon, detach->info->alias)) == 0) { + if ((ret = qemuMonitorDriveDel(priv->mon, drvstr)) < 0) { + VIR_WARN("qemuMonitorDriveDel failed on %s (%s)", + detach->info->alias, drvstr); + qemuMonitorAddDevice(priv->mon, devstr); Coverity complains right here about no checking of return status:

(9) Event check_return: Calling function "qemuMonitorAddDevice(qemuMonitorPtr, char const *)" without checking return value (as is done elsewhere 8 out of 9 times).

As with attach, the flow of this code is a bit different than other places where DelDevice() and DriveDel() are called - I would think you may want to follow those other models more closely...

I think you meant keeping the original error? here is the diff:

Right, seems better now and the visual inspection regarding the coverity complaint is cleaned up...

ACK

Thanks, pushed with the diff.

On 05/03/2013 02:07 PM, Osier Yang wrote:

With unknown good reasons, the attribute "bus" of scsi device address is always set to 0, same for attribute "target". (See virDomainDiskDefAssignAddress).

Though we might need to change the algrithom to honor "bus"

s/algrithom/algorithm

and "target" too, it's another story. The address generator

s/it's another story/that's a different issue

for scsi host device in this patch just follows the unknown good reasons, only considering the "controller" and "unit". It walks through all scsi controllers and their units, to see if the address $controller:0:0:$unit can be used, if found one, it sits on it, otherwise, it creates a new controller (actually the controller is created implicitly by someone else), and sits on $new_controller:0:0:0 instead.

Implicit creation

--- Since it needs to add the controllers for "drive" type address implicitly, I add the generator in domain_conf.c instead of the specific the driver, e.g. qemu. --- src/conf/domain_conf.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 139 insertions(+), 5 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 31a8c46..cff2b46 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8658,8 +8658,140 @@ error: return NULL; }

+/* Check if a drive type address $controller:0:0:$unit is already + * taken by a disk or not. + */ +static bool +virDomainDriveAddressIsUsedByDisk(virDomainDefPtr def, + enum virDomainDiskBus type, + unsigned int controller, + unsigned int unit) +{ + virDomainDiskDefPtr disk; + int i; + + for (i = 0; i < def->ndisks; i++) { + disk = def->disks[i]; + + if (disk->bus != type || + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) + continue; + + if (disk->info.addr.drive.controller == controller && + disk->info.addr.drive.unit == unit && + disk->info.addr.drive.bus == 0 && + disk->info.addr.drive.target == 0) + return true; + } + + return false; +} + +/* Check if a drive type address $controller:0:0:$unit is already + * taken by a host device or not. + */ +static bool +virDomainDriveAddressIsUsedByHostdev(virDomainDefPtr def, + enum virDomainHostdevSubsysType type, + unsigned int controller, + unsigned int unit) +{ + virDomainHostdevDefPtr hostdev; + int i; + + for (i = 0; i < def->nhostdevs; i++) { + hostdev = def->hostdevs[i]; + + if (hostdev->source.subsys.type != type) + continue; + + if (hostdev->info->addr.drive.controller == controller && + hostdev->info->addr.drive.unit == unit && + hostdev->info->addr.drive.bus == 0 && + hostdev->info->addr.drive.target == 0) + return true; + } + + return false; +} + +/* Find out the next usable "unit" of a specific controller */ +static int +virDomainControllerSCSINextUnit(virDomainDefPtr def, + unsigned int max_unit, + unsigned int controller) +{ + int i; + + for (i = 0; i < max_unit; i++) { + /* The controller itself is on unit 7 */ + if (max_unit == 16 && i == 7)

I would expect 16 and 7 to be constants The 'max_unit' check is irrelevant if max_unit=7, then we're not getting here anyway...

+ continue; + + if (!virDomainDriveAddressIsUsedByDisk(def, + VIR_DOMAIN_DISK_BUS_SCSI, controller, i) && + !virDomainDriveAddressIsUsedByHostdev(def, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, controller, i)) + return i; + } + + return -1; +} + +static int +virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, + virDomainDefPtr def, + virDomainHostdevDefPtr hostdev)

This ends up being Scsi specific right? E.G. AssignSCSIAddress instead of generic AssignAddress (or wherever the "best" place is to put SCSI)

+{ + unsigned int max_unit; + int next_unit; + unsigned nscsi_controllers;

s/;/ = 0;

+ bool found; + int i; + + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + return -1; + + /* See comments in virDomainDiskDefAssignAddress */ + if (xmlopt->config.hasWideScsiBus) + max_unit = 16; + else + max_unit = 7;

Guess I would have figured these would be constants somewhere... Not that I expect the values to change... Units are numbered 0 -> 15 and 0 -> 6 then?

+ + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + continue; + + nscsi_controllers++;

If we're "implicitly" creating a new controller, then is it's number based on the total number of controllers or the next scsi controller?

+ next_unit = virDomainControllerSCSINextUnit(def, max_unit, + def->controllers[i]->idx); + if (next_unit >= 0) { + found = true; + break; + } + } + + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + + if (found) { + hostdev->info->addr.drive.controller = def->controllers[i]->idx; + hostdev->info->addr.drive.bus = 0; + hostdev->info->addr.drive.target = 0; + hostdev->info->addr.drive.unit = next_unit; + } else { + hostdev->info->addr.drive.controller = nscsi_controllers + 1;

To be fair, I'm not familiar (yet) with how things are laid out, but it would seem to me that this might not be returning what you want/expect. I'm missing a nuance here - the 'controller' field is generally based off the def->controllers[i]->idx value, except when we're "implicitly creating" one. In this case it's just a 'random' value based on some count of existing nscsi_controllers. If there are zero existing controllers, then we start at 1 If there is one existing/full controller, then we start at 2 How does the 'idx' value get populated? Is it possible that this setting conflicts with a different and non scsi controller? Is there a max controller number we shouldn't go past? John

+ hostdev->info->addr.drive.bus = 0; + hostdev->info->addr.drive.target = 0; + hostdev->info->addr.drive.unit = 0; + } + + return 0; +} + static virDomainHostdevDefPtr -virDomainHostdevDefParseXML(const xmlNodePtr node, +virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, + virDomainDefPtr vmdef, + const xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, unsigned int flags) @@ -8719,7 +8851,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, } break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: - if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + virDomainHostdevAssignAddress(xmlopt, vmdef, def) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("SCSI host devices must have address specified")); goto error; @@ -9128,8 +9261,8 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; } else if (xmlStrEqual(node->name, BAD_CAST "hostdev")) { dev->type = VIR_DOMAIN_DEVICE_HOSTDEV; - if (!(dev->data.hostdev = virDomainHostdevDefParseXML(node, ctxt, NULL, - flags))) + if (!(dev->data.hostdev = virDomainHostdevDefParseXML(xmlopt, def, node, + ctxt, NULL, flags))) goto error; } else if (xmlStrEqual(node->name, BAD_CAST "controller")) { dev->type = VIR_DOMAIN_DEVICE_CONTROLLER; @@ -11477,7 +11610,8 @@ virDomainDefParseXML(xmlDocPtr xml, for (i = 0 ; i < n ; i++) { virDomainHostdevDefPtr hostdev;

- hostdev = virDomainHostdevDefParseXML(nodes[i], ctxt, bootHash, flags); + hostdev = virDomainHostdevDefParseXML(xmlopt, def, nodes[i], + ctxt, bootHash, flags); if (!hostdev) goto error;

On 07/05/13 21:45, John Ferlan wrote:

On 05/03/2013 02:07 PM, Osier Yang wrote:

...

...

Unlike disk device, the scsi-generic always writethrough the data, so no need to introduce a "cache" tag, and set "cache=off". --- docs/formatdomain.html.in | 6 ++++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 4 +++ src/conf/domain_conf.h | 1 + .../qemuxml2argv-hostdev-scsi-shareable.xml | 36 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 53 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-shareable.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9cd79e5..6437d6d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2410,6 +2410,12 @@ <dd>Indicates that the device is readonly, only supported by SCSI host device now. <span class="since">Since 1.0.6</span> </dd> + <dt><code>shareable</code></dt> + <dd>If present, this indicates the device is expected to be shared + between domains (assuming the hypervisor and OS support this). + Only supported by SCSI host device now. s/ now/

Missed that above too I think in the previous review.

ACK on the rest. Thanks, pushed with the change.

On 05/03/2013 02:07 PM, Osier Yang wrote:

E.g.

% sg_map /dev/sg0 /dev/sda /dev/sg1 /dev/sr0

What the helper gets for /dev/sg0 is /dev/sda, it will be used by later patch. --- src/libvirt_private.syms | 1 + src/util/virscsi.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virscsi.h | 4 ++++ 3 files changed, 52 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fa59c14..8a7105c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1692,6 +1692,7 @@ virSCSIDeviceFileIterate; virSCSIDeviceFree; virSCSIDeviceGetAdapter; virSCSIDeviceGetBus; +virSCSIDeviceGetDevName; virSCSIDeviceGetName; virSCSIDeviceGetReadonly; virSCSIDeviceGetSgName; diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 7d3d40e..f337c75 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -143,6 +143,53 @@ cleanup: return sg; }

+/* Returns device name (e.g. "sdc") on success, or NULL + * on failure. + */ +char * +virSCSIDeviceGetDevName(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit) +{ + DIR *dir = NULL; + struct dirent *entry; + char *path = NULL; + char *name = NULL; + unsigned int adapter_id; + + if (virSCSIDeviceGetAdapterId(adapter, &adapter_id) < 0) + return NULL; + + if (virAsprintf(&path, + SYSFS_SCSI_DEVICES "/%d:%d:%d:%d/block", + adapter_id, bus, target, unit) < 0) { + virReportOOMError(); + return NULL; + } + + if (!(dir = opendir(path))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open %s"), path); + goto cleanup; + } + + while ((entry = readdir(dir))) { + if (entry->d_name[0] == '.') + continue; + + if (!(name = strdup(entry->d_name))) { + virReportOOMError(); + goto cleanup; + }

If 'name' then what - we go back to the top of the loop and if there's more entries then we'll strdup() another name.... Although I have to wonder if there can be more than non hidden entry in the list... Makes me wonder about my earlier reviews which didn't think about the loop exit (that is virSCSIDeviceGetSgName()) ACK with the proper exit strategy. John

+ } + +cleanup: + closedir(dir); + VIR_FREE(path); + return name; +} + virSCSIDevicePtr virSCSIDeviceNew(const char *adapter, unsigned int bus, diff --git a/src/util/virscsi.h b/src/util/virscsi.h index 8268cdf..cce5df4 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -37,6 +37,10 @@ char *virSCSIDeviceGetSgName(const char *adapter, unsigned int bus, unsigned int target, unsigned int unit); +char *virSCSIDeviceGetDevName(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit);

virSCSIDevicePtr virSCSIDeviceNew(const char *adapter, unsigned int bus,

On 07/05/13 22:51, Osier Yang wrote:

On 07/05/13 22:00, John Ferlan wrote:

...

On 05/03/2013 02:07 PM, Osier Yang wrote:

...

E.g.

% sg_map /dev/sg0 /dev/sda /dev/sg1 /dev/sr0

What the helper gets for /dev/sg0 is /dev/sda, it will be used by later patch. --- src/libvirt_private.syms | 1 + src/util/virscsi.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virscsi.h | 4 ++++ 3 files changed, 52 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fa59c14..8a7105c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1692,6 +1692,7 @@ virSCSIDeviceFileIterate; virSCSIDeviceFree; virSCSIDeviceGetAdapter; virSCSIDeviceGetBus; +virSCSIDeviceGetDevName; virSCSIDeviceGetName; virSCSIDeviceGetReadonly; virSCSIDeviceGetSgName; diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 7d3d40e..f337c75 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -143,6 +143,53 @@ cleanup: return sg; } +/* Returns device name (e.g. "sdc") on success, or NULL + * on failure. + */ +char * +virSCSIDeviceGetDevName(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit) +{ + DIR *dir = NULL; + struct dirent *entry; + char *path = NULL; + char *name = NULL; + unsigned int adapter_id; + + if (virSCSIDeviceGetAdapterId(adapter, &adapter_id) < 0) + return NULL; + + if (virAsprintf(&path, + SYSFS_SCSI_DEVICES "/%d:%d:%d:%d/block", + adapter_id, bus, target, unit) < 0) { + virReportOOMError(); + return NULL; + } + + if (!(dir = opendir(path))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open %s"), path); + goto cleanup; + } + + while ((entry = readdir(dir))) { + if (entry->d_name[0] == '.') + continue; + + if (!(name = strdup(entry->d_name))) { + virReportOOMError(); + goto cleanup; + } If 'name' then what - we go back to the top of the loop and if there's more entries then we'll strdup() another name.... Although I have to wonder if there can be more than non hidden entry in the list... Makes me wonder about my earlier reviews which didn't think about the loop exit (that is virSCSIDeviceGetSgName())

It's the only one entry (except the hidden entries), same for virSCSIDeviceGetSgName. But adding the "break" is better, to avoid it loops 1 more. Will change when pushing.

Actually adding the break looks a bit strange, and I think we won't care it tries one more condition checking.. So I pushed this as-is.

On 05/03/2013 02:07 PM, Osier Yang wrote:

This changes the helpers qemu{Add,Remove}SharedDisk into qemu{Add,Remove}SharedDevice, as most of the code in the helpers can be reused for scsi host device.

To track the shared scsi host device, first it finds out the device path (e.g. /dev/s[dr]*) which is mapped to the sg device, and use device ID of the found device path (/dev/s[dr]*) as the hash key. This is because of the device ID is not unique between between /dev/s[dr]* and /dev/sg*, e.g.

% sg_map /dev/sg0 /dev/sda /dev/sg1 /dev/sr0

% ls -l /dev/sda brw-rw----. 1 root disk 8, 0 May 2 19:26 /dev/sda

%ls -l /dev/sg0 crw-rw----. 1 root disk 21, 0 May 2 19:26 /dev/sg0 --- src/qemu/qemu_conf.c | 143 ++++++++++++++++++++++++++++++++++++------------ src/qemu/qemu_conf.h | 20 +++---- src/qemu/qemu_driver.c | 16 +++--- src/qemu/qemu_process.c | 19 +++++-- 4 files changed, 140 insertions(+), 58 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 244795d..ebcd176 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1084,47 +1084,80 @@ cleanup: return NULL; }

-/* qemuAddSharedDisk: +/* qemuAddSharedDevice: * @driver: Pointer to qemu driver struct - * @disk: The disk def + * @dev: The device def * @name: The domain name * * Increase ref count and add the domain name into the list which - * records all the domains that use the shared disk if the entry + * records all the domains that use the shared device if the entry * already exists, otherwise add a new entry. */ int -qemuAddSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk, - const char *name) +qemuAddSharedDevice(virQEMUDriverPtr driver, + virDomainDeviceDefPtr dev, + const char *name) { qemuSharedDeviceEntry *entry = NULL; qemuSharedDeviceEntry *new_entry = NULL; + virDomainDiskDefPtr disk = NULL; + virDomainHostdevDefPtr hostdev = NULL; + char *dev_name = NULL; + char *dev_path = NULL; char *key = NULL; int ret = -1;

- /* Currently the only conflicts we have to care about - * for the shared disk is "sgio" setting, which is only - * valid for block disk. + /* Currently the only conflicts we have to care about for + * the shared disk and shared host device is "sgio" setting, + * which is only valid for block disk and scsi host device. */ - if (!disk->shared || - !disk->src || - (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && - !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && - disk->srcpool && - disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + disk = dev->data.disk; + + if (disk->shared || + !disk->src || + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) + return 0; + } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { + hostdev = dev->data.hostdev; + + if (!hostdev->shareable || + (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI))

And this is the controller that's being OR'd so I think this is right, but making sure :-) Over time HPVM was asked to allow a whole controller to be shareable because the guest would control/manage the luns...

+ return 0; + } else { return 0; + }

qemuDriverLock(driver); - if (qemuCheckSharedDisk(driver->sharedDevices, disk) < 0) - goto cleanup; + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + if (qemuCheckSharedDisk(driver->sharedDevices, disk) < 0) + goto cleanup;

- if (!(key = qemuGetSharedDeviceKey(disk->src))) - goto cleanup; + if (!(key = qemuGetSharedDeviceKey(disk->src))) + goto cleanup; + } else { + if (!(dev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit))) + goto cleanup; + + if (virAsprintf(&dev_path, "/dev/%s", dev_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!(key = qemuGetSharedDeviceKey(dev_path))) + goto cleanup; + }

if ((entry = virHashLookup(driver->sharedDevices, key))) { - /* Nothing to do if the shared disk is already recorded - * in the table. + /* Nothing to do if the shared scsi host device is already + * recorded in the table. */ if (qemuSharedDeviceEntryDomainExists(entry, name, NULL)) { ret = 0; @@ -1163,41 +1196,77 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, ret = 0; cleanup: qemuDriverUnlock(driver); + VIR_FREE(dev_name); + VIR_FREE(dev_path); VIR_FREE(key); return ret; }

-/* qemuRemoveSharedDisk: +/* qemuRemoveSharedDevice: * @driver: Pointer to qemu driver struct - * @disk: The disk def + * @device: The device def * @name: The domain name * * Decrease ref count and remove the domain name from the list which - * records all the domains that use the shared disk if ref is not 1, - * otherwise remove the entry. + * records all the domains that use the shared device if ref is not + * 1, otherwise remove the entry. */ int -qemuRemoveSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk, - const char *name) +qemuRemoveSharedDevice(virQEMUDriverPtr driver, + virDomainDeviceDefPtr dev, + const char *name) { qemuSharedDeviceEntryPtr entry = NULL; qemuSharedDeviceEntryPtr new_entry = NULL; + virDomainDiskDefPtr disk = NULL; + virDomainHostdevDefPtr hostdev = NULL; char *key = NULL; + char *dev_name = NULL; + char *dev_path = NULL; int ret = -1; int idx;

- if (!disk->shared || - !disk->src || - (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && - !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && - disk->srcpool && - disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + disk = dev->data.disk; + + if (!disk->shared || + !disk->src || + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) + return 0; + } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { + hostdev = dev->data.hostdev; + + if (!hostdev->shareable || + (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)) + return 0;

Same in reverse... ACK on the remainder John

+ } else { return 0; + }

qemuDriverLock(driver); - if (!(key = qemuGetSharedDeviceKey(disk->src))) - goto cleanup; + + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + if (!(key = qemuGetSharedDeviceKey(disk->src))) + goto cleanup; + } else { + if (!(dev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit))) + goto cleanup; + + if (virAsprintf(&dev_path, "/dev/%s", dev_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!(key = qemuGetSharedDeviceKey(dev_path))) + goto cleanup; + }

if (!(entry = virHashLookup(driver->sharedDevices, key))) goto cleanup; @@ -1233,6 +1302,8 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver, ret = 0; cleanup: qemuDriverUnlock(driver); + VIR_FREE(dev_name); + VIR_FREE(dev_path); VIR_FREE(key); return ret; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d4a54a0..c004f7f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -283,22 +283,22 @@ bool qemuSharedDeviceEntryDomainExists(qemuSharedDeviceEntryPtr entry, int *index) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

-int qemuAddSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk, - const char *name) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); - -int qemuRemoveSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk, - const char *name) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); - char * qemuGetSharedDeviceKey(const char *disk_path) ATTRIBUTE_NONNULL(1);

void qemuSharedDeviceEntryFree(void *payload, const void *name) ATTRIBUTE_NONNULL(1);

+int qemuAddSharedDevice(virQEMUDriverPtr driver, + virDomainDeviceDefPtr dev, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +int qemuRemoveSharedDevice(virQEMUDriverPtr driver, + virDomainDeviceDefPtr dev, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + int qemuDriverAllocateID(virQEMUDriverPtr driver); virDomainXMLOptionPtr virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver);

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4c2ab7b..991b68b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5648,7 +5648,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, if (qemuTranslateDiskSourcePool(conn, disk) < 0) goto end;

- if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) + if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) goto end;

if (qemuSetUnprivSGIO(disk) < 0) @@ -5694,8 +5694,8 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, * if the operation is either ejecting or updating. */ if (ret == 0) - ignore_value(qemuRemoveSharedDisk(driver, dev_copy->data.disk, - vm->def->name)); + ignore_value(qemuRemoveSharedDevice(driver, dev_copy, + vm->def->name)); break; case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: @@ -5732,7 +5732,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,

end: if (ret != 0) - ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); + ignore_value(qemuRemoveSharedDevice(driver, dev, vm->def->name)); virObjectUnref(caps); virDomainDeviceDefFree(dev_copy); return ret; @@ -5848,7 +5848,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, }

if (ret == 0) - ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); + ignore_value(qemuRemoveSharedDevice(driver, dev, vm->def->name));

return ret; } @@ -5955,7 +5955,7 @@ qemuDomainChangeDiskMediaLive(virConnectPtr conn, dev->data.disk = tmp;

/* Add the new disk src into shared disk hash table */ - if (qemuAddSharedDisk(driver, dev->data.disk, vm->def->name) < 0) + if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) goto end;

ret = qemuDomainChangeEjectableMedia(driver, vm, disk, orig_disk, force); @@ -5968,8 +5968,8 @@ qemuDomainChangeDiskMediaLive(virConnectPtr conn, */ if (ret == 0) { dev->data.disk = NULL; - ignore_value(qemuRemoveSharedDisk(driver, dev_copy->data.disk, - vm->def->name)); + ignore_value(qemuRemoveSharedDevice(driver, dev_copy, + vm->def->name)); } break; default: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 70eadb6..117c669 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2982,10 +2982,14 @@ qemuProcessReconnect(void *opaque) * qemu_driver->sharedDevices. */ for (i = 0; i < obj->def->ndisks; i++) { + virDomainDeviceDef dev; + if (qemuTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) goto error; - if (qemuAddSharedDisk(driver, obj->def->disks[i], - obj->def->name) < 0) + + dev.type = VIR_DOMAIN_DEVICE_DISK; + dev.data.disk = obj->def->disks[i]; + if (qemuAddSharedDevice(driver, &dev, obj->def->name) < 0) goto error; }

@@ -3663,6 +3667,7 @@ int qemuProcessStart(virConnectPtr conn,

/* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */ for (i = 0; i < vm->def->ndisks; i++) { + virDomainDeviceDef dev; virDomainDiskDefPtr disk = vm->def->disks[i];

if (vm->def->disks[i]->rawio == 1) @@ -3673,7 +3678,9 @@ int qemuProcessStart(virConnectPtr conn, _("Raw I/O is not supported on this platform")); #endif

- if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) + dev.type = VIR_DOMAIN_DEVICE_DISK; + dev.data.disk = disk; + if (qemuAddSharedDevice(driver, &dev, vm->def->name) < 0) goto cleanup;

if (qemuSetUnprivSGIO(disk) < 0) @@ -4084,8 +4091,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, virSecurityManagerReleaseLabel(driver->securityManager, vm->def);

for (i = 0; i < vm->def->ndisks; i++) { + virDomainDeviceDef dev; virDomainDiskDefPtr disk = vm->def->disks[i]; - ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); + + dev.type = VIR_DOMAIN_DEVICE_DISK; + dev.data.disk = disk; + ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name)); }

/* Clear out dynamically assigned labels */

On 08/05/13 03:17, John Ferlan wrote:

On 05/03/2013 02:07 PM, Osier Yang wrote:

...

"sgio" is only valid for scsi host device. --- docs/formatdomain.html.in | 7 ++++- docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 34 ++++++++++++++++++--- src/conf/domain_conf.h | 1 + .../qemuxml2argv-hostdev-scsi-sgio.xml | 35 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-sgio.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6437d6d..89d11e7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2319,7 +2319,12 @@ and <code>virNodeDeviceReAttach</code> (or <code>virsh nodedev-reattach</code>) after hot-unplug or stopping the guest. For SCSI device, user is responsible to make sure the device - is not used by host.</dd> + is not used by host. + The optional <code>sgio</code> (<span class="since">since 1.0.6</span>) + attribute indicates whether the kernel will filter unprivileged + SG_IO commands for the disk, valid settings are "filtered" or + "unfiltered". Defaults to "filtered". + </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 diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ca79e67..0a59c7a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3179,6 +3179,14 @@ <attribute name="type"> <value>scsi</value> </attribute> + <optional> + <attribute name="sgio"> + <choice> + <value>filtered</value> + <value>unfiltered</value> + </choice> + </attribute> + </optional> <element name="source"> <interleave> <ref name="sourceinfoadapter"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9823b9c..d604e5b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3784,6 +3784,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, { xmlNodePtr sourcenode; char *managed = NULL; + char *sgio = NULL; char *backendStr = NULL; int backend; int ret = -1; @@ -3798,6 +3799,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, def->managed = true; }

+ sgio = virXMLPropString(node, "sgio"); + /* @type is passed in from the caller rather than read from the * xml document, because it is specified in different places for * different kinds of defs - it is an attribute of @@ -3834,6 +3837,22 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, goto error; }

+ if (sgio) { + if (def->source.subsys.type != + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("sgio is only supported for scsi host device")); + goto error; + } + + if ((def->source.subsys.u.scsi.sgio = + virDomainDeviceSGIOTypeFromString(sgio)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown sgio mode '%s'"), sgio); + goto error; + } + } + switch (def->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (virDomainHostdevSubsysPciDefParseXML(sourcenode, def, flags) < 0) @@ -3872,6 +3891,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, ret = 0; error: VIR_FREE(managed); + VIR_FREE(sgio); VIR_FREE(backendStr); return ret; } @@ -15464,11 +15484,17 @@ virDomainHostdevDefFormat(virBufferPtr buf,

virBufferAsprintf(buf, " <hostdev mode='%s' type='%s'", mode, type); - if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - virBufferAsprintf(buf, " managed='%s'>\n", + if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + virBufferAsprintf(buf, " managed='%s'", def->managed ? "yes" : "no"); - else - virBufferAddLit(buf, ">\n"); + + if (def->source.subsys.type == + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + def->source.subsys.u.scsi.sgio) + virBufferAsprintf(buf, " sgio='%s'", + virDomainDeviceSGIOTypeToString(def->source.subsys.u.scsi.sgio)); + } + virBufferAddLit(buf, ">\n");

virBufferAdjustIndent(buf, 6); switch (def->mode) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 59cf6a7..8d67e67 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -423,6 +423,7 @@ struct _virDomainHostdevSubsys { unsigned bus; unsigned target; unsigned unit; + int sgio; /* enum virDomainDeviceSGIO */ } scsi; } u; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-sgio.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-sgio.xml new file mode 100644 index 0000000..21404ee --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-sgio.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='scsi' managed='yes' sgio='unfiltered'> + <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> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index d37a657..04be8dd 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -289,6 +289,7 @@ mymain(void) DO_TEST("hostdev-scsi-virtio-scsi"); DO_TEST("hostdev-scsi-readonly"); DO_TEST("hostdev-scsi-shareable"); + DO_TEST("hostdev-scsi-sgio"); NIT: should this be "scsi-unfiltered-sgio" since the default is filtered and you're testing the unfiltered mode... Probably changes the xml file name too.

It's not a deal breaker... ACK otherwise.

I'd like keep it, one can also define "filtered" mode in the same XML for a different device. Pushed.

On 08/05/13 08:05, John Ferlan wrote:

On 05/03/2013 02:07 PM, Osier Yang wrote:

...

Just like previous patches, this changes qemuCheckSharedDisk into qemuCheckSharedDevice, which takes a virDomainDeviceDefPtr argument instead. --- src/qemu/qemu_conf.c | 86 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 25 deletions(-)

Ahhh finally - never thought I'd get to the last one :-) Taken longer than I wanted!

...

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index cf1c7ee..f8264f6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -943,29 +943,55 @@ qemuGetSharedDeviceKey(const char *device_path) return key; }

-/* Check if a shared disk's setting conflicts with the conf +/* Check if a shared device's setting conflicts with the conf * used by other domain(s). Currently only checks the sgio * setting. Note that this should only be called for disk with - * block source. + * block source if the device type is disk. * * Returns 0 if no conflicts, otherwise returns -1. */ static int -qemuCheckSharedDisk(virHashTablePtr sharedDevices, - virDomainDiskDefPtr disk) +qemuCheckSharedDevice(virHashTablePtr sharedDevices, + virDomainDeviceDefPtr dev) { + virDomainDiskDefPtr disk = NULL; + virDomainHostdevDefPtr hostdev = NULL; char *sysfs_path = NULL; char *key = NULL; + char *hostdev_name = NULL; + char *hostdev_path = NULL; + char *device_path = NULL; int val; int ret = 0;

- /* The only conflicts between shared disk we care about now - * is sgio setting, which is only valid for device='lun'. - */ - if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) - return 0; Coverity note #1:

(2) Event cond_false: Condition "dev->type == VIR_DOMAIN_DEVICE_DISK", taking false branch

...

+ if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + disk = dev->data.disk; + + /* The only conflicts between shared disk we care about now + * is sgio setting, which is only valid for device='lun'. + */ + if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) + return 0; + + device_path = disk->src; Coverity note #2:

(3) Event cond_false: Condition "dev->type == VIR_DOMAIN_DEVICE_HOSTDEV", taking false branch

...

+ } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { + hostdev = dev->data.hostdev; + + if (!(hostdev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit))) + goto cleanup; + + if (virAsprintf(&hostdev_path, "/dev/%s", hostdev_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + device_path = hostdev_path; + } Coverity Note #3

In the "else" condition (not here) - that means "device_path = NULL" which is going to be a problem shortly....

Should we return 0 as an "else" condition?

...

- if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL))) { + if (!(sysfs_path = virGetUnprivSGIOSysfsPath(device_path, NULL))) { ret = -1; goto cleanup; } @@ -976,7 +1002,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices, if (!virFileExists(sysfs_path)) goto cleanup;

Coverity complains:

(8) Event var_deref_model: Passing null pointer "device_path" to function "qemuGetSharedDeviceKey(char const *)", which dereferences it. (The dereference is assumed on the basis of the 'nonnull' parameter attribute.) Also see events: [assign_zero]

Fix that and it's an ACK

To not introduce coverity errors anymore, I setup the coverity env on my local box, and the errors in this patch are disapeared after add the else branch: } else { return 0; } So pushed. Thanks. Osier