Re: [libvirt] [PATCH 2/8] storage: Make the adapter name be consistent with node device driver

On 02/04/2013 08:31 AM, Osier Yang wrote: > node device driver names the HBA like "scsi_host5", but storage > driver uses "host5", which could make the user confused. This > make them consistent. However, for back-compact reason, adapter > name like "host5" is still supported. > --- > docs/formatstorage.html.in | 13 +++++---- > src/storage/storage_backend_scsi.c | 43 +++++++++++++++++++++-------- > tests/storagepoolxml2xmlin/pool-scsi.xml | 2 +- > tests/storagepoolxml2xmlout/pool-scsi.xml | 2 +- > 4 files changed, 40 insertions(+), 20 deletions(-) > > diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in > index 0849618..af42fed 100644 > --- a/docs/formatstorage.html.in > +++ b/docs/formatstorage.html.in > @@ -89,12 +89,13 @@ > <dt><code>adapter</code></dt> > <dd>Provides the source for pools backed by SCSI adapters. May > 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, > - valid value is "fc_host" or "scsi_host", defaults to "scsi_host" > - if<code>name</code> is specified. To keep back-compact, > -<code>type</code> is optional for "scsi_host" adapter, but > - mandatory for "fc_host" adapter. Attribute<code>wwnn</code> and > + name (ex. "scsi_host1", name like 'host1' is not recommended to > + use, though it's still supported for back-compact reason). > + Attribute<code>type</code> (<span class="since">1.0.2</span>) > + specifies the adapter type, valid value is "fc_host" or "scsi_host", > + defaults to "scsi_host" if<code>name</code> is specified. To keep > + back-compact,<code>type</code> is optional for "scsi_host" adapter, > + but mandatory for "fc_host" adapter. Attribute<code>wwnn</code> and Still getting used to this adjust the previous patch, especially when it comes to documenting. Anyway, it seems the "new" text is ""scsi_host1", name like 'host1' is not recommended to use, though it's still supported for back-compact reason" Consider the following: "scsi_host1". NB, although a name such as "host1" is still supported for backwards compatibility, it is not recommended

> <code>wwpn</code> (<span class="since">1.0.2</span>) indicates > the (v)HBA. The optional attribute<code>parent</code> > (<span class="since">1.0.2</span>) specifies the parent device of > diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c > index c1c163e..a26bf59 100644 > --- a/src/storage/storage_backend_scsi.c > +++ b/src/storage/storage_backend_scsi.c > @@ -632,14 +632,38 @@ out: > } > > static int > +getHostNumber(const char *adapter_name) > +{ > + int host; This was a uint32_t before... So it can be negative :-) > + > + /* Specifying adpater like 'host5' is still supported for s/adpater/adapter/ > + * back-compact reason. s/back-compact reason/backwards compatibility reasons/

> + */ > + if ((sscanf(adapter_name, "scsi_host%d",&host) != 1)&& > + (sscanf(adapter_name, "host%d",&host) != 1)) { This was %u before > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to get host number from '%s'"), > + adapter_name); > + return -1; > + } > + > + return host; > +} > + > +static int > virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, > virStoragePoolObjPtr pool, > bool *isActive) > { > char *path; > + int host; Use uint32_t > > *isActive = false; > - if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter.data.name)< 0) { > + > + if ((host = getHostNumber(pool->def->source.adapter.data.name))< 0) > + return -1; > + > + if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host)< 0) { > virReportOOMError(); > return -1; > } > @@ -656,29 +680,24 @@ static int > virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, > virStoragePoolObjPtr pool) > { > - int retval = 0; > - uint32_t host; > + int ret = -1; > + int host; I think you need to keep this a uint32_t - if only because callee's expect it that way > > pool->def->allocation = pool->def->capacity = pool->def->available = 0; > > - 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.data.name); > - retval = -1; > + if ((host = getHostNumber(pool->def->source.adapter.data.name))< 0) > goto out; > - } > > VIR_DEBUG("Scanning host%u", host); host is now an 'int'? > > - if (virStorageBackendSCSITriggerRescan(host)< 0) { > - retval = -1; > + if (virStorageBackendSCSITriggerRescan(host)< 0) > goto out; > - } This call expects a uint32_t > > virStorageBackendSCSIFindLUs(pool, host); As does this one > > + ret = 0; > out: > - return retval; > + return ret; > } > > > diff --git a/tests/storagepoolxml2xmlin/pool-scsi.xml b/tests/storagepoolxml2xmlin/pool-scsi.xml > index 3650e43..091ecfc 100644 > --- a/tests/storagepoolxml2xmlin/pool-scsi.xml > +++ b/tests/storagepoolxml2xmlin/pool-scsi.xml > @@ -2,7 +2,7 @@ > <name>hba0</name> > <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> > <source> > -<adapter name="host0"/> > +<adapter name="scsi_host0"/> > </source> > <target> > <path>/dev/disk/by-path</path> > diff --git a/tests/storagepoolxml2xmlout/pool-scsi.xml b/tests/storagepoolxml2xmlout/pool-scsi.xml > index faf5342..101b61a 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 type='scsi_host' name='host0'/> > +<adapter type='scsi_host' name='scsi_host0'/> > </source> > <target> > <path>/dev/disk/by-path</path> > Considering my comments in 1/8, here we now have the need for new tests. One that accepts "host0" and one that accepts "scsi_host0". That would be true for both the "name" only and the "type"& "name" options. The point being, you've gone from one way to describe things to 4 ways: "name=host0" "name=scsi_host0" "type=scsi_host name=host0" "type=scsi_host name=scsi_host0" Unless of course, options 2& 3 above are not possible...

John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list