[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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;
>   };


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]