On 03/25/2013 12:43 PM, Osier Yang wrote:
> node device driver names the HBA like "scsi_host5", but storage
> driver uses "host5", which could make the user confused. This
> changes them to be consistent. However, for back-compat reason,
> adapter name like "host5" is still supported.
>
> v1 - v2:
> * Use virStrToLong_ui instead of sscanf
> * No tests addition or changes, because this patch only affects
> the way scsi backend works for adapter, adding xml2xml tests for
> it is just meaningless.
> ---
> docs/formatstorage.html.in | 15 ++++++-----
> src/storage/storage_backend_scsi.c | 54 +++++++++++++++++++++++++++++---------
> 2 files changed, 50 insertions(+), 19 deletions(-)
>
> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
> index eff3016..5fae933 100644
> --- a/docs/formatstorage.html.in
> +++ b/docs/formatstorage.html.in
> @@ -89,13 +89,14 @@
> <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.4</span>) specifies the adapter type.
> - Valid value are "fc_host" and "scsi_host". If omitted and
> - the <code>name</code> attribute is specified, then it defaults to
> - "scsi_host". To keep backwards compatibility, the attribute
> - <code>type</code> is optional for the "scsi_host" adapter, but
> - mandatory for the "fc_host" adapter. Attributes <code>wwnn</code>
> + name (ex. "scsi_host1". NB, although a name such as "host1" is
> + still supported for backwards compatibility, it is not recommended).
> + Attribute <code>type</code> (<span class="since">1.0.4</span>)
> + specifies the adapter type. Valid value are "fc_host" and "scsi_host".
> + If omitted and the <code>name</code> attribute is specified, then it
> + defaults to "scsi_host". To keep backwards compatibility, the
> + attribute <code>type</code> is optional for the "scsi_host" adapter,
> + but mandatory for the "fc_host" adapter. Attributes <code>wwnn</code>
> (Word Wide Node Name) and <code>wwpn</code> (Word Wide Port Name)
> (<span class="since">1.0.4</span>) are used by the "fc_host" adapter
> to uniquely indentify the device in the Fibre Channel storage farbic
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index c1c163e..cc1ebe2 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -632,14 +632,49 @@ out:
> }
>
> static int
> +getHostNumber(const char *adapter_name,
> + unsigned int *result)
> +{
> + char *host = (char *)adapter_name;
> +
> + /* Specifying adapter like 'host5' is still supported for
> + * back-compat reason.
> + */
> + if (STRPREFIX(host, "scsi_host")) {
> + host += strlen("scsi_host");
> + } else if (STRPREFIX(host, "host")) {
> + host += strlen("host");
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid adapter name '%s' for scsi pool"),
s/invalid/Invalid
s/scsi/SCSI
> + adapter_name);
> + return -1;
> + }
> +
> + if (result && virStrToLong_ui(host, NULL, 10, result) == -1) {
Consider using ATTRIBUTE_NONNULL(2) on the argument instead... Although
this is a local/static function...
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid adapter name '%s' for scsi pool"),
s/invalid/Invalid
s/scsi/SCSI
> + adapter_name);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int
> virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> virStoragePoolObjPtr pool,
> bool *isActive)
> {
> char *path;
> + unsigned int host;
>
> *isActive = false;
> - if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter.data.name) < 0) {
> +
> + if (getHostNumber(pool->def->source.adapter.data.name, &host) < 0)
> + return -1;
> +
> + if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host) < 0) {
> virReportOOMError();
> return -1;
> }
> @@ -656,29 +691,24 @@ static int
> virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> virStoragePoolObjPtr pool)
> {
> - int retval = 0;
> - uint32_t host;
> + int ret = -1;
> + unsigned int host;
>
> 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 (getHostNumber(pool->def->source.adapter.data.name, &host) < 0)
> goto out;
> - }
>
> VIR_DEBUG("Scanning host%u", host);
>
> - if (virStorageBackendSCSITriggerRescan(host) < 0) {
> - retval = -1;
> + if (virStorageBackendSCSITriggerRescan(host) < 0)
> goto out;
> - }
>
> virStorageBackendSCSIFindLUs(pool, host);
>
> + ret = 0;
> out:
> - return retval;
> + return ret;
> }
>
>
>
ACK with cleanups
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list