Re: [libvirt] [PATCH RFC 2/5] conf: Introduce scsi hostdev

On 2013年03月04日 14:01, Han Cheng wrote:

Adding scsi hostdev, it should like:

<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> --- docs/schemas/domaincommon.rng | 33 +++++++++ src/conf/domain_audit.c | 10 +++ src/conf/domain_conf.c | 149 +++++++++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 7 ++ 4 files changed, 194 insertions(+), 5 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fbb4001..ebd0e42 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2931,6 +2931,7 @@ <choice> <ref name="hostdevsubsyspci"/> <ref name="hostdevsubsysusb"/> +<ref name="hostdevsubsysscsi"/> </choice> </define>

@@ -2983,6 +2984,22 @@ </element> </define>

+<define name="hostdevsubsysscsi"> +<attribute name="type"> +<value>scsi</value> +</attribute> +<element name="source"> +<element name="adapter"> +<attribute name="name"> +<ref name="scsiAdapter"/> +</attribute> +</element> +<element name="address"> +<ref name="scsiaddress"/> +</element> +</element> +</define> + <define name="hostdevcapsstorage"> <attribute name="type"> <value>storage</value> @@ -3027,6 +3044,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"/> @@ -3893,4 +3921,9 @@ </element> <empty/> </define> +<define name="scsiAdapter"> +<data type="string"> +<param name="pattern">scsi_host[0-9]{1,2}</param>

No need to have a duplicate definition. It can reuse what storage pool uses.

+</data> +</define> </grammar> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index c00bd11..e6d44b1 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -281,6 +281,16 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, goto cleanup; } break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (virAsprintf(&address, "%.3d:%.3d:%.3d:%.3d", + 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 5e385e4..2be9129 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -557,7 +557,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST,

VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST, "usb", - "pci") + "pci", + "scsi")

VIR_ENUM_IMPL(virDomainHostdevCaps, VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST, "storage", @@ -2928,6 +2929,96 @@ virDomainParseLegacyDeviceAddress(char *devaddr, }

static int +virDomainHostdevSubsysScsiDefParseXML(const xmlNodePtr node, + virDomainHostdevDefPtr def) +{ + int ret = -1; + xmlNodePtr cur;

If you define those variables here: char *bus, *target, *unit;

+ + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "address")) { + char *bus, *target, *unit; + + bus=virXMLPropString(cur, "bus"); + if (bus) {

These codes can be simplified as: if ((bus = virXMLPropString(cur, "bus")) < 0) { virReportError(...); goto out; } if (virStrToLong_ui(bus, NULL, 0, &def->source.subsys.u.scsi.bus) < 0) { virReportError(...); goto out; } With freeing the strings in "out". [1]

+ ret = virStrToLong_ui(bus, NULL, 0, +&def->source.subsys.u.scsi.bus); + VIR_FREE(bus); + if (ret< 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse bus %s"), bus); + goto out; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("scsi address needs bus id"));

Generally we uses strings like in this case: "bus must be specified for scsi hostdev address"

+ goto out; + } + target=virXMLPropString(cur, "target"); + if (target) { + ret = virStrToLong_ui(target, NULL, 0, +&def->source.subsys.u.scsi.target); + VIR_FREE(target); + if (ret< 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse target %s"), target); + goto out; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("scsi address needs target id")); + goto out; + } + unit=virXMLPropString(cur, "unit"); + if (unit) { + ret = virStrToLong_ui(unit, NULL, 0, +&def->source.subsys.u.scsi.unit); + VIR_FREE(unit); + if (ret< 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse unit %s"), unit); + goto out; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("scsi address needs unit id")); + goto out; + } + } else if (xmlStrEqual(cur->name, BAD_CAST "adapter")) { + char *adapter; + adapter = virXMLPropString(cur, "name"); + if (adapter&& STRSKIP(adapter, "scsi_host")) { + ret = virStrToLong_ui(adapter + strlen("scsi_host"), NULL, 0, +&def->source.subsys.u.scsi.adapter);

Why not storing the adapter name as a string instead?

+ VIR_FREE(adapter); + if (ret< 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse adapter %s"), adapter); + goto out; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("scsi needs adapter")); + goto out; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown scsi source type '%s'"), + cur->name); + goto out; + } + } + cur = cur->next; + } +

How about "adapter" is specified, but "source address" is not specified. Or the vice versa.

+ ret = 0; +out:

[1] VIR_FREE(bus); VIR_FREE(target); VIR_FREE(unit);

+ return ret; +} + +static int virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, virDomainHostdevDefPtr def) { @@ -3222,6 +3313,10 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, if (virDomainHostdevSubsysUsbDefParseXML(sourcenode, def)< 0) goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (virDomainHostdevSubsysScsiDefParseXML(sourcenode, def)< 0) + goto error; + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("address type='%s' not supported in hostdev interfaces"), @@ -12146,6 +12241,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)< 0) + return -1; + } + + return 0; +}

/* * Based on the declared<address/> info for any devices, @@ -12182,6 +12301,9 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def) if (virDomainDefMaybeAddSmartcardController(def)< 0) return -1;

+ if (virDomainDefMaybeAddHostdevSCSIcontroller(def)< 0) + return -1; + return 0; }

@@ -12997,6 +13119,15 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAdjustIndent(buf, 2); switch (def->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + virBufferAsprintf(buf, "<adapter name='scsi_host%d'/>\n",

This is hard code. Assuming that a scsi host device can have different name with "scsi_host" on platform other than Linux. So again, IMHO we should just storing the adapter name as string.

+ def->source.subsys.u.scsi.adapter); + virBufferAsprintf(buf, "<address %sbus='%d' target='%d' unit='%d'/>\n", + includeTypeInAddr ? "type='scsi' " : "", + def->source.subsys.u.scsi.bus, + def->source.subsys.u.scsi.target, + def->source.subsys.u.scsi.unit); + break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: if (def->source.subsys.u.usb.vendor) { virBufferAsprintf(buf, "<vendor id='0x%.4x'/>\n", @@ -14250,10 +14381,18 @@ virDomainHostdevDefFormat(virBufferPtr buf, } virBufferAdjustIndent(buf, -6);

- if (virDomainDeviceInfoFormat(buf, def->info, - flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT - | VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM)< 0) - return -1; + if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS&& + def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + if (virDomainDeviceInfoFormat(buf, def->info, + flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT + | VIR_DOMAIN_XML_INTERNAL_ALLOW_READONLY)< 0)

As commented in 1/5. This is no need as long as you add the "readonly" as an external XML tag.

+ return -1; + } else { + if (virDomainDeviceInfoFormat(buf, def->info, + flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT + | VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM)< 0) + return -1; + }

virBufferAddLit(buf, "</hostdev>\n");

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 39c5849..c04cb23 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -366,6 +366,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 }; @@ -385,6 +386,12 @@ struct _virDomainHostdevSubsys { unsigned vendor; unsigned product; } usb; + struct { + unsigned adapter;

char *adapter;

+ unsigned bus; + unsigned target; + unsigned unit; + } scsi; virDevicePCIAddress pci; /* host address */ } u; };