[libvirt] [PATCH 07/11] storage_scsi: Translate the stable address into scsi host number

[Osier Yang]

On 20/06/13 00:35, John Ferlan wrote:
> On 06/07/2013 01:03 PM, Osier Yang wrote:
>> This takes use of the two utils introduced in previous patches.
>>
>> Node device HAL backend represents PCI device like PCI_8086_2922,
>> (I.E PCI_$vendor_$product), to get the PCI address, we have to
>> traverse /sys/devices/ to find it out.
>>
>> And to get the current scsi host number assigned by the system
>> for the scsi host device. We need to traverse /sys/bus/pci/devices/
>> with the found PCI address by getScsiHostParentAddress, and the
>> specified unique_id.
>>
>> Later patch will allow to omit the "unique_id", by using the
>> scsi host which has the smallest unique_id.
>> ---
>>   src/storage/storage_backend_scsi.c | 81 +++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 79 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>> index 13c498d..a77b9ae 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -591,6 +591,66 @@ out:
>>       return retval;
>>   }
>>   
>> +/*
>> + * Node device udev backend and HAL backend represent PCI
>> + * device in different style. Udev backend represents it like
>> + * pci_0000_00_1f_2, and HAL backend represents it like
>> + * pci_8086_2922.
>> + *
>> + * Covert the value of "parent" into PCI device address string
>> + * (e.g. 0000:00:1f:2). Return the PCI address as string on
>> + * success, or -1 on failure.
>> + */
>> +static char *
>> +getScsiHostParentAddress(const char *parent)
>> +{
>> +    char **tokens = NULL;
>> +    char *vendor = NULL;
>> +    char *product = NULL;
>> +    char *ret = NULL;
>> +    int len;
>> +
>> +    if (!strchr(parent, '_')) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("'parent' of scsi_host adapter must "
>> +                         "be consistent with name of node device"));
>> +        return NULL;
>> +    }
>> +
>> +    if (!(tokens = virStringSplit(parent, "_", 0)))
>> +        return NULL;
> Given the following code, the call to Split could use 4 (or 5) for the
> max...  Not that it matters.

hm, it should be 5.

>
>> +
>> +    len = virStringListLength(tokens);
>> +
>> +    switch (len) {
>> +    case 4:
>> +        if (!(ret = virStringJoin((const char **)(&tokens[1]), ":")))
>> +            goto cleanup;
>> +        break;
>> +
>> +    case 2:
>> +        vendor = tokens[1];
>> +        product = tokens[2];
>> +        if (!(ret = virFindPCIDeviceByVPD(NULL, vendor, product))) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Unable to find PCI device with vendor '%s' "
>> +                             "product '%s'"), vendor, product);
>> +            goto cleanup;
>> +        }
>> +
>> +        break;
>> +    default:
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("'parent' of scsi_host adapter must "
>> +                         "be consistent with name of node device"));
>> +        goto cleanup;
>> +    }
>> +
>> +cleanup:
>> +    virStringFreeList(tokens);
>> +    return ret;
>> +}
>> +
>>   static int
>>   getHostNumber(const char *adapter_name,
>>                 unsigned int *result)
>> @@ -627,10 +687,27 @@ static char *
>>   getAdapterName(virStoragePoolSourceAdapter adapter)
>>   {
>>       char *name = NULL;
>> +    char *addr = NULL;
>>   
>>       if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> -        if (VIR_STRDUP(name, adapter.data.scsi_host.name) < 0)
>> -            return NULL;
>> +        if (adapter.data.scsi_host.parent) {
>> +            if (!adapter.data.scsi_host.has_unique_id) {
>> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                               _("'unique_id' is not specified for "
>> +                                 "scsi_host adapter"));
> so the optional unique_id isn't so optional after all for a couple of
> code paths.  Issuing a message here could be confusing.  Has this been
> tested within an environment where unique_id isn't specified?

It's made optional by later patches. Commit log clarifies it.

>
>
>> +                return NULL;
>> +            } else {
>> +                if (!(addr = getScsiHostParentAddress(adapter.data.scsi_host.parent)))
>> +                    return NULL;
>> +
>> +                name = virParseStableScsiHostAddress(NULL, addr,
>> +                                                     adapter.data.scsi_host.unique_id);
> You need a VIR_FREE(addr); here
>
> John
>> +            }
>> +        } else if (adapter.data.scsi_host.name) {
>> +            if (VIR_STRDUP(name, adapter.data.scsi_host.name) < 0)
>> +                return NULL;
>> +        }
>> +
>>           return name;
>>       }
>>   
>>