[libvirt] [PATCH 1/8] New XML attributes for storage pool source adapter

Osier Yang jyang at redhat.com
Wed Feb 6 12:35:14 UTC 2013


On 2013年02月06日 03:48, John Ferlan wrote:
> On 02/04/2013 08:31 AM, Osier Yang wrote:
>> This introduces 4 new attributes for storage pool source adapter.
>> E.g.
>>
>> <adapter type='fc_host' parent='scsi_host5' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/>
>>
>> Attribute 'type' can be either 'scsi_host' or 'fc_host', and defaults
>> to 'scsi_host' if attribute 'name' is specified. I.e. It's optional
>> for 'scsi_host' adapter, for back-compact reason. However, it's mandatory
>> for 'fc_host' adapter and any new future adapter types. Attribute 'parent'
>> is required for vHBA, but optional for HBA.
>>
>> * docs/formatstorage.html.in:
>>    - Add documents for the 4 new attrs
>> * docs/schemas/storagepool.rng:
>>    - Add RNG schema
>> * src/conf/storage_conf.c:
>>    - Parse and format the new XMLs
>> * src/conf/storage_conf.h:
>>    - New struct virStoragePoolSourceAdapter, replace "char *adapter" with it;
>>    - New enum virStoragePoolSourceAdapterType
>> * src/libvirt_private.syms:
>>    - Export TypeToString and TypeFromString
>> * src/phyp/phyp_driver.c:
>>    - Replace "adapter" with "adapter.data.name", which is member of the union
>>      of the new struct virStoragePoolSourceAdapter now
>> * src/storage/storage_backend_scsi.c:
>>    - Like above
>> * tests/storagepoolxml2xmlin/pool-scsi1.xml:
>>    - New test for 'fc_host' adapter
>> * tests/storagepoolxml2xmlout/pool-scsi.xml:
>>    - Change the expected output, as the 'type' defaults to 'scsi_host' if 'name"
>>      specified now
>> * tests/storagepoolxml2xmlout/pool-scsi1.xml:
>>    - New test
>> * tests/storagepoolxml2xmltest.c:
>>    - Include the test
>> ---
>>   docs/formatstorage.html.in                 |   13 +++-
>>   docs/schemas/storagepool.rng               |   33 +++++++-
>>   src/conf/storage_conf.c                    |  123 +++++++++++++++++++++++++--
>>   src/conf/storage_conf.h                    |   23 +++++-
>>   src/libvirt_private.syms                   |    2 +
>>   src/phyp/phyp_driver.c                     |    8 +-
>>   src/storage/storage_backend_scsi.c         |    6 +-
>>   tests/storagepoolxml2xmlin/pool-scsi1.xml  |   15 ++++
>>   tests/storagepoolxml2xmlout/pool-scsi.xml  |    2 +-
>>   tests/storagepoolxml2xmlout/pool-scsi1.xml |   18 ++++
>>   tests/storagepoolxml2xmltest.c             |    1 +
>>   11 files changed, 220 insertions(+), 24 deletions(-)
>>   create mode 100644 tests/storagepoolxml2xmlin/pool-scsi1.xml
>>   create mode 100644 tests/storagepoolxml2xmlout/pool-scsi1.xml
>>
>> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
>> index 9f93db8..0849618 100644
>> --- a/docs/formatstorage.html.in
>> +++ b/docs/formatstorage.html.in
>> @@ -88,8 +88,17 @@
>>           <span class="since">Since 0.4.1</span></dd>
>>         <dt><code>adapter</code></dt>
>>         <dd>Provides the source for pools backed by SCSI adapters. May
>> -        only occur once. Contains a single attribute<code>name</code>
>> -        which is the SCSI adapter name (ex. "host1").
>> +        only occur once. Attribute<code>name</code>  is the SCSI adapter
>> +        name (ex. "host1"). Attribute<code>type</code>
>> +        (<span class="since">1.0.2</span>) specifies the adapter type,
>
> s/type,/type./

Okay,

>
>> +        valid value is "fc_host" or "scsi_host", defaults to "scsi_host"
>> +        if<code>name</code>  is specified. To keep back-compact,
>
> Consider the following instead:
>
> Valid values are "fc_host" (vHBA) and "scsi_host"(HBA). If omitted and
> the<code>name</code>  attribute is specified, then it defaults to
> "scsi_host".

Sounds good.

>
> NOTE: Based on further description, I'm assuming "fc_host" is "vHBA" and
> "scsi_host" is "HBA".  Rather than introduce (v)HBA below, I used this
> opportunity to define them here...

But one still can use "scsi_host" for vHBA. So I don't think
"fc_host (vHBA)" and "scsi_host (HBA)" is good.

>

>
> s/back-compact/backwards compatibility the/

Okay, but /backwards compatibility, the/

>
>> +<code>type</code>  is optional for "scsi_host" adapter, but
>
> /s/is optional for "scsi_host"/attribute is optional for the "scsi_host"/

I'd like:

attribute <code>type</code> is optional for the "scsi_host".

>
>> +        mandatory for "fc_host" adapter.  Attribute<code>wwnn</code>  and
>
> s/for/for the/
>

Okay.

>> +<code>wwpn</code>  (<span class="since">1.0.2</span>) indicates
>
>
> s/Attribute/Attributes/
> s/indicates/indicate/
>

Okay.

> Consider perhaps: "Attributes wwnn (world wide node name) and wwpn

Additions on your proposal:

<code>wwnn</code>,  <code>wwpn</code>

> (world wide port name) are used by the "fc_host" (vHBA) adapter to
> uniquely identify the device in the Fibre Channel storage fabric.
>
> Thus "the (v)HBA" below becomes unnecessary.
>
>> +        the (v)HBA. The optional attribute<code>parent</code>
>
> s/optional//
>
>> +        (<span class="since">1.0.2</span>) specifies the parent device of
>> +        the (v)HBA. It's optional for HBA, but required for vHBA.
>
> s/of the v(HBA)/
>
> The text is here is confusing because you say it's required for vHBA
> here, but declare it optional later. It's completely ignored for
> "scsi_host", but can be present for "fc_host". This is where you
> introduced vHBA without defining it so it has led to my confusion. If
> someone mistakenly supplied parent with scsi_host, then the
> virStoragePoolSourceFormat() will not write out the parent.
>

It's sorted out in later patch. Because "find one parent for the vHBA"
is implemented in later patch, not in this one. The reason for making
you confused is one can use "wwnn && wwpn" to indicate a HBA, which
doesn't need the parent. Hmm I tend to agree with you that it's easier 
to just say it's optional here. Because it doesn't error out when
parsing, and also the rng schema defines it as optional.

>
>>           <span class="since">Since 0.6.2</span></dd>
>>         <dt><code>host</code></dt>
>>         <dd>Provides the source for pools backed by storage from a
>> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
>> index 165e276..a3204ec 100644
>> --- a/docs/schemas/storagepool.rng
>> +++ b/docs/schemas/storagepool.rng
>> @@ -275,9 +275,36 @@
>>
>>     <define name='sourceinfoadapter'>
>>       <element name='adapter'>
>> -<attribute name='name'>
>> -<text/>
>> -</attribute>
>> +<choice>
>> +<group>
>> +<!-- To keep back-compact, 'type' is not mandatory for
>
> s/compact/compat/

Okay.

>
>> +           scsi_host adapter -->
>> +<optional>
>> +<attribute name='type'>
>> +<value>scsi_host</value>
>> +</attribute>
>> +</optional>
>> +<attribute name='name'>
>> +<text/>
>> +</attribute>
>> +</group>
>> +<group>
>> +<attribute name='type'>
>> +<value>fc_host</value>
>> +</attribute>
>> +<optional>
>> +<attribute name='parent'>
>> +<text/>
>> +</attribute>
>> +</optional>
>
> The text description implies otherwise, but the code certainly declares
> this optional.

As said above, this will be not problem anymore once I just
say it's optional.

>
>> +<attribute name='wwnn'>
>> +<ref name='wwn'/>
>> +</attribute>
>> +<attribute name='wwpn'>
>> +<ref name='wwn'/>
>> +</attribute>
>> +</group>
>> +</choice>
>>         <empty/>
>>       </element>
>>     </define>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 7a39998..ddb2945 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -90,6 +90,10 @@ VIR_ENUM_IMPL(virStoragePartedFsType,
>>                 "ext2", "ext2",
>>                 "extended")
>>
>> +VIR_ENUM_IMPL(virStoragePoolSourceAdapterType,
>> +              VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST,
>> +              "default", "scsi_host", "fc_host")
>> +
>>   typedef const char *(*virStorageVolFormatToString)(int format);
>>   typedef int (*virStorageVolFormatFromString)(const char *format);
>>
>> @@ -304,6 +308,18 @@ virStorageVolDefFree(virStorageVolDefPtr def) {
>>       VIR_FREE(def);
>>   }
>>
>> +static void
>> +virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapter adapter)
>> +{
>> +    if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> +        VIR_FREE(adapter.data.fchost.wwnn);
>> +        VIR_FREE(adapter.data.fchost.wwpn);
>
> ??
> VIR_FREE(adapter.data.fchost.parent)

Good catch.

>
>> +    } else if (adapter.type ==
>> +               VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>> +        VIR_FREE(adapter.data.name);
>> +    }
>> +}
>> +
>>   void
>>   virStoragePoolSourceClear(virStoragePoolSourcePtr source)
>>   {
>> @@ -324,7 +340,7 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source)
>>       VIR_FREE(source->devices);
>>       VIR_FREE(source->dir);
>>       VIR_FREE(source->name);
>> -    VIR_FREE(source->adapter);
>> +    virStoragePoolSourceAdapterClear(source->adapter);
>>       VIR_FREE(source->initiator.iqn);
>>       VIR_FREE(source->vendor);
>>       VIR_FREE(source->product);
>> @@ -489,6 +505,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
>>       virStoragePoolOptionsPtr options;
>>       char *name = NULL;
>>       char *port = NULL;
>> +    char *adapter_type = NULL;
>>       int n;
>>
>>       relnode = ctxt->node;
>> @@ -580,7 +597,45 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
>>       }
>>
>>       source->dir = virXPathString("string(./dir/@path)", ctxt);
>> -    source->adapter = virXPathString("string(./adapter/@name)", ctxt);
>> +
>> +    if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) {
>> +        if ((source->adapter.type =
>> +             virStoragePoolSourceAdapterTypeTypeFromString(adapter_type))<= 0) {
>> +            virReportError(VIR_ERR_XML_ERROR,
>> +                           _("Unknown pool adapter type '%s'"),
>> +                           adapter_type);
>> +            goto cleanup;
>> +        }
>> +
>> +        if (source->adapter.type ==
>> +            VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> +            source->adapter.data.fchost.parent =
>> +                virXPathString("string(./adapter/@parent)", ctxt);
>> +            source->adapter.data.fchost.wwnn =
>> +                virXPathString("string(./adapter/@wwnn)", ctxt);
>> +            source->adapter.data.fchost.wwpn =
>> +                virXPathString("string(./adapter/@wwpn)", ctxt);
>> +        } else if (source->adapter.type ==
>> +                   VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>> +            source->adapter.data.name =
>> +                virXPathString("string(./adapter/@name)", ctxt);
>> +        }
>> +    } else {
>> +        if (virXPathString("string(./adapter/@wwnn)", ctxt) ||
>> +            virXPathString("string(./adapter/@wwpn)", ctxt)) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("'type' for 'fc_host' adapter must be specified"));
>
> Message is confusing, consider "Use of 'wwnn' and 'wwpn' attributes not
> valid for 'scsi_host" adapter." or "Use of 'wwnn' and 'wwpn' attributes
> requires the 'fc_host' adapter 'type'".

I like the later one.

>
>> +            goto cleanup;
>> +        }
> I believe you will leak here if either of these is true.  Thus you'll
> need some sort of
>      wwnn = virXPath...
>      wwpn = virXPath...
>      if (wwnn || wwpn) {
>          VIR_FREE(wwnn);
>          VIR_FREE(wwpn);

Right.

>> +
>> +        /* To keep back-compact, 'type' is not required to specify
>
> s/back-compact/backwards compatibility/

Okay.

>
>> +         * for scsi_host adapter.
>> +         */
>> +        if ((source->adapter.data.name =
>> +             virXPathString("string(./adapter/@name)", ctxt)))
>> +            source->adapter.type =
>> +                VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
>> +    }
>>
>>       authType = virXPathString("string(./auth/@type)", ctxt);
>>       if (authType == NULL) {
>> @@ -618,6 +673,7 @@ cleanup:
>>       VIR_FREE(port);
>>       VIR_FREE(authType);
>>       VIR_FREE(nodeset);
>> +    VIR_FREE(adapter_type);
>>       return ret;
>>   }
>>
>> @@ -819,11 +875,33 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) {
>>       }
>>
>>       if (options->flags&  VIR_STORAGE_POOL_SOURCE_ADAPTER) {
>> -        if (!ret->source.adapter) {
>> -            virReportError(VIR_ERR_XML_ERROR,
>> -                           "%s", _("missing storage pool source adapter name"));
>> +        if (!ret->source.adapter.type) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("missing storage pool source adapter"));
>>               goto cleanup;
>>           }
>> +
>> +        if (ret->source.adapter.type ==
>> +            VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>
> The fchost.parent is optional here - following the code, but not the docs.
>
>> +            if (!ret->source.adapter.data.fchost.wwnn ||
>> +                !ret->source.adapter.data.fchost.wwpn) {
>> +                virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                               _("'wwnn' and 'wwpn' must be specified for adapter "
>> +                                 "type 'fchost'"));
>> +                goto cleanup;
>> +            }
>> +
>> +            if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) ||
>> +                !virValidateWWN(ret->source.adapter.data.fchost.wwpn))
>> +                goto cleanup;
>> +        } else if (ret->source.adapter.type ==
>> +                   VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>> +            if (!ret->source.adapter.data.name) {
>> +                virReportError(VIR_ERR_XML_ERROR,
>> +                               "%s", _("missing storage pool source adapter name"));
>> +                goto cleanup;
>> +            }
>> +        }
>>       }
>>
>>       /* If DEVICE is the only source type, then its required */
>> @@ -953,9 +1031,23 @@ virStoragePoolSourceFormat(virBufferPtr buf,
>>       if ((options->flags&  VIR_STORAGE_POOL_SOURCE_DIR)&&
>>           src->dir)
>>           virBufferAsprintf(buf,"<dir path='%s'/>\n", src->dir);
>> -    if ((options->flags&  VIR_STORAGE_POOL_SOURCE_ADAPTER)&&
>> -        src->adapter)
>> -        virBufferAsprintf(buf,"<adapter name='%s'/>\n", src->adapter);
>> +    if ((options->flags&  VIR_STORAGE_POOL_SOURCE_ADAPTER)) {
>> +        if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST ||
>> +            src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)
>> +            virBufferAsprintf(buf, "<adapter type='%s'",
>> +                              virStoragePoolSourceAdapterTypeTypeToString(src->adapter.type));
>> +
>> +        if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> +            virBufferEscapeString(buf, " parent='%s'",
>> +                                  src->adapter.data.fchost.parent);
>
> Since parent is possible NULL you won't write anything, right?

Yes. It's the magic of virBufferEscapeString.

>
>> +            virBufferAsprintf(buf," wwnn='%s' wwpn='%s'/>\n",
>> +                              src->adapter.data.fchost.wwnn,
>> +                              src->adapter.data.fchost.wwpn);
>> +        } else if (src->adapter.type ==
>> +                 VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>> +            virBufferAsprintf(buf," name='%s'/>\n", src->adapter.data.name);
>> +        }
>> +    }
>>       if ((options->flags&  VIR_STORAGE_POOL_SOURCE_NAME)&&
>>           src->name)
>>           virBufferAsprintf(buf,"<name>%s</name>\n", src->name);
>> @@ -1856,8 +1948,19 @@ int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
>>                   matchpool = pool;
>>               break;
>>           case VIR_STORAGE_POOL_SCSI:
>> -            if (STREQ(pool->def->source.adapter, def->source.adapter))
>> -                matchpool = pool;
>> +            if (pool->def->source.adapter.type ==
>> +                VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> +                if (STREQ(pool->def->source.adapter.data.fchost.wwnn,
>> +                          def->source.adapter.data.fchost.wwnn)&&
>> +                    STREQ(pool->def->source.adapter.data.fchost.wwpn,
>> +                          def->source.adapter.data.fchost.wwpn))
>> +                    matchpool = pool;
>> +            } else if (pool->def->source.adapter.type ==
>> +                       VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST){
>> +                if (STREQ(pool->def->source.adapter.data.name,
>> +                          def->source.adapter.data.name))
>> +                    matchpool = pool;
>> +            }
>>               break;
>>           case VIR_STORAGE_POOL_ISCSI:
>>           {
>> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
>> index ad16eca..60b8034 100644
>> --- a/src/conf/storage_conf.h
>> +++ b/src/conf/storage_conf.h
>> @@ -233,7 +233,28 @@ struct _virStoragePoolSourceDevice {
>>       } geometry;
>>   };
>>
>> +enum virStoragePoolSourceAdapterType {
>> +    VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_DEFAULT = 0,
>> +    VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST,
>> +    VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST,
>>
>> +    VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST,
>> +};
>> +VIR_ENUM_DECL(virStoragePoolSourceAdapterType)
>> +
>> +typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter;
>> +struct _virStoragePoolSourceAdapter {
>> +    int type; /* enum virStoragePoolSourceAdapterType */
>> +
>> +    union {
>> +        char *name;
>> +        struct {
>> +            char *parent;
>> +            char *wwnn;
>> +            char *wwpn;
>> +        } fchost;
>> +    } data;
>> +};
>>
>>   typedef struct _virStoragePoolSource virStoragePoolSource;
>>   typedef virStoragePoolSource *virStoragePoolSourcePtr;
>> @@ -250,7 +271,7 @@ struct _virStoragePoolSource {
>>       char *dir;
>>
>>       /* Or an adapter */
>> -    char *adapter;
>> +    virStoragePoolSourceAdapter adapter;
>>
>>       /* Or a name */
>>       char *name;
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 4a84b2b..c0c6b91 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -777,6 +777,8 @@ virStoragePoolObjLock;
>>   virStoragePoolObjRemove;
>>   virStoragePoolObjSaveDef;
>>   virStoragePoolObjUnlock;
>> +virStoragePoolSourceAdapterTypeTypeFromString;
>> +virStoragePoolSourceAdapterTypeTypeToString;
>>   virStoragePoolSourceClear;
>>   virStoragePoolSourceFindDuplicate;
>>   virStoragePoolSourceFindDuplicateDevices;
>> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
>> index 74f04ff..a12a430 100644
>> --- a/src/phyp/phyp_driver.c
>> +++ b/src/phyp/phyp_driver.c
>> @@ -2058,7 +2058,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool,
>>       spdef->source.ndevice = 1;
>>
>>       /*XXX source adapter not working properly, should show hdiskX */
>> -    if ((spdef->source.adapter =
>> +    if ((spdef->source.adapter.data.name =
>
> So this code only works for scsi_host then?  Since data.name is not
> valid for fc_host.

Yes, later patch adds checking before this.

>
>>            phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) {
>>           VIR_ERROR(_("Unable to determine storage pools's source adapter."));
>>           goto err;
>> @@ -2280,7 +2280,7 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags)
>>
>>       pool.source.ndevice = 1;
>>
>> -    if ((pool.source.adapter =
>> +    if ((pool.source.adapter.data.name =
>
> Same comment

Since we will support to create pool has adapter of 'fc_host' type.
It can only be "scsi_host".

>
>>            phypGetStoragePoolDevice(sp->conn, sp->name)) == NULL) {
>>           VIR_ERROR(_("Unable to determine storage sps's source adapter."));
>>           goto cleanup;
>> @@ -2520,7 +2520,7 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def)
>>                             managed_system, vios_id);
>>
>>       virBufferAsprintf(&buf, "mksp -f %schild %s", def->name,
>> -                      source.adapter);
>> +                      source.adapter.data.name);
>
> and again

Likewise.

>
>>
>>       if (system_type == HMC)
>>           virBufferAddChar(&buf, '\'');
>> @@ -2761,7 +2761,7 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags)
>>       def.source.ndevice = 1;
>>
>>       /*XXX source adapter not working properly, should show hdiskX */
>> -    if ((def.source.adapter =
>> +    if ((def.source.adapter.data.name =
>
> ditto

Likewise.

>
>>            phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) {
>>           VIR_ERROR(_("Unable to determine storage pools's source adapter."));
>>           goto err;
>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>> index 90bbf59..c1c163e 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -639,7 +639,7 @@ virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>>       char *path;
>>
>>       *isActive = false;
>> -    if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter)<  0) {
>> +    if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter.data.name)<  0) {
>>           virReportOOMError();
>>           return -1;
>>       }
>> @@ -661,9 +661,9 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>>
>>       pool->def->allocation = pool->def->capacity = pool->def->available = 0;
>>
>> -    if (sscanf(pool->def->source.adapter, "host%u",&host) != 1) {
>> +    if (sscanf(pool->def->source.adapter.data.name, "host%u",&host) != 1) {
>>           VIR_DEBUG("Failed to get host number from '%s'",
>> -                    pool->def->source.adapter);
>> +                    pool->def->source.adapter.data.name);
>>           retval = -1;
>>           goto out;
>>       }
> I'll make the broader assumption that something with _scsi in the
> filename means it only works for scsi.

Yeah, it's sorted out in later patch, the reason for I simply do the
replacement is this patch is just for paring and formating.

>
>> diff --git a/tests/storagepoolxml2xmlin/pool-scsi1.xml b/tests/storagepoolxml2xmlin/pool-scsi1.xml
>> new file mode 100644
>> index 0000000..1e9826d
>> --- /dev/null
>> +++ b/tests/storagepoolxml2xmlin/pool-scsi1.xml
>> @@ -0,0 +1,15 @@
>> +<pool type='scsi'>
>> +<name>hba0</name>
>> +<uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
>> +<source>
>> +<adapter type='fc_host' parent='scsi_host5' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/>
>> +</source>
>> +<target>
>> +<path>/dev/disk/by-path</path>
>> +<permissions>
>> +<mode>0700</mode>
>> +<owner>0</owner>
>> +<group>0</group>
>> +</permissions>
>> +</target>
>> +</pool>
>> diff --git a/tests/storagepoolxml2xmlout/pool-scsi.xml b/tests/storagepoolxml2xmlout/pool-scsi.xml
>> index 321dc2b..faf5342 100644
>> --- a/tests/storagepoolxml2xmlout/pool-scsi.xml
>> +++ b/tests/storagepoolxml2xmlout/pool-scsi.xml
>> @@ -5,7 +5,7 @@
>>     <allocation unit='bytes'>0</allocation>
>>     <available unit='bytes'>0</available>
>>     <source>
>> -<adapter name='host0'/>
>> +<adapter type='scsi_host' name='host0'/>
>>     </source>
>>     <target>
>>       <path>/dev/disk/by-path</path>
>
> I think you shouldn't change this one - it becomes the backwards
> compatibility check.  Then create a "pool-scsi-type-scsi.xml" using the
> new syntax (or something close/similar to indicate the "scsi_host"
> syntax is in use).

Agreed. The new name is nice.

>
>> diff --git a/tests/storagepoolxml2xmlout/pool-scsi1.xml b/tests/storagepoolxml2xmlout/pool-scsi1.xml
>> new file mode 100644
>> index 0000000..fb1079f
>> --- /dev/null
>> +++ b/tests/storagepoolxml2xmlout/pool-scsi1.xml
>> @@ -0,0 +1,18 @@
>> +<pool type='scsi'>
>> +<name>hba0</name>
>> +<uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
>> +<capacity unit='bytes'>0</capacity>
>> +<allocation unit='bytes'>0</allocation>
>> +<available unit='bytes'>0</available>
>> +<source>
>> +<adapter type='fc_host' parent='scsi_host5' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/>
>> +</source>
>> +<target>
>> +<path>/dev/disk/by-path</path>
>> +<permissions>
>> +<mode>0700</mode>
>> +<owner>0</owner>
>> +<group>0</group>
>> +</permissions>
>> +</target>
>> +</pool>
>
> Change the name from "pool-scsi1.xml" ->  "pool-scsi-type-fc.xml"
>
> I think that'll be clearer
>
>> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c
>> index 8cac978..c04eb69 100644
>> --- a/tests/storagepoolxml2xmltest.c
>> +++ b/tests/storagepoolxml2xmltest.c
>> @@ -90,6 +90,7 @@ mymain(void)
>>       DO_TEST("pool-iscsi-auth");
>>       DO_TEST("pool-netfs");
>>       DO_TEST("pool-scsi");
>> +    DO_TEST("pool-scsi1");
>>       DO_TEST("pool-mpath");
>>       DO_TEST("pool-iscsi-multiiqn");
>>       DO_TEST("pool-iscsi-vendor-product");
>>
>
> Then change/add the names here too.
>
>
> John
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list




More information about the libvir-list mailing list