[libvirt] [PATCH 09/11] storage_scsi: Allow the direct PCI address for 'parent'

[John Ferlan]

On 06/07/2013 01:16 PM, Osier Yang wrote:
> On 08/06/13 01:03, Osier Yang wrote:
>> To be more flexible, except allowing to specify 'parent' with name
>> produced by node device udev/HAL backends, this supports to specify
>> 'parent' with PCI address directly (e.g.  0000:00:1f:2). The specified
>> address will be padded if it's not consistent with what sysfs exposed.
>> (e.g 0:0:2:2 will be padded to 0000:00:02:2).
>> ---
>>   src/storage/storage_backend_scsi.c | 117
>> +++++++++++++++++++++++++++++--------
>>   1 file changed, 93 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_scsi.c
>> b/src/storage/storage_backend_scsi.c
>> index a77b9ae..5635f73 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -604,51 +604,120 @@ out:
>>   static char *
>>   getScsiHostParentAddress(const char *parent)
>>   {
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>>       char **tokens = NULL;
>>       char *vendor = NULL;
>>       char *product = NULL;
>>       char *ret = NULL;
>> -    int len;
>> +    int length;
>> +    int i;
>>   -    if (!strchr(parent, '_')) {
>> +    if (!strchr(parent, '_') &&
>> +        !strchr(parent, ':')) {
>>           virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                       _("'parent' of scsi_host adapter must "
>> -                         "be consistent with name of node device"));
>> +                       _("'parent' of scsi_host adapter must be "
>> +                         "either consistent with name of mode "
>> +                         "device, or in domain:bus:slot:function "
>> +                         "format"));

"name of the node device or in"

not

"name of mode device, or in"

>>           return NULL;
>>       }
>>   -    if (!(tokens = virStringSplit(parent, "_", 0)))
>> -        return NULL;
>> +    if (strchr(parent, '_')) {
>> +        if (!(tokens = virStringSplit(parent, "_", 0)))
>> +            return NULL;
>>   -    len = virStringListLength(tokens);
>> +        length = virStringListLength(tokens);
>> +
>> +        switch (length) {
>> +        case 4:
>> +            if (!(ret = virStringJoin((const char **)(&tokens[1]),
>> ":")))
>> +                goto cleanup;
>> +            break;
>>   -    switch (len) {
>> -    case 4:
>> -        if (!(ret = virStringJoin((const char **)(&tokens[1]), ":")))
>> +        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 "
>> +                         "either consistent with name of mode "
>> +                         "device, or in domain:bus:slot:function "
>> +                         "format"));

Duplicated error message - same issues as before, plus I think you need
to consider determining which of the two places you got the error.  That
is if we see that message, then did we get an error because there wasn't
a "_" or ":" in the name or (in this case) because the address was
malformed since we expected only 2 or 4 numbers with a specific
separator but found more or less.  In this case, I would think you could
just indicate the parent %s is malformed, requires only 2 or 4 separators.

>>               goto cleanup;
>> -        break;
>> +        }
>> +    } else if (strchr(parent, ':')) {
>> +        char *padstr = NULL;
>> +
>> +        if (!(tokens = virStringSplit(parent, ":", 0)))
>> +            return NULL;
>>   -    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);
>> +        length = virStringListLength(tokens);
>> +
>> +        if (length != 4) {
>> +            virReportError(VIR_ERR_XML_ERROR,
>> +                           _("invalid PCI address for scsi_host "
>> +                             "'parent' '%s'"), parent);
>>               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;
>> +        for (i = 0; i < length; i++) {
>> +            if (strlen(tokens[i]) == 0) {
>> +                virReportError(VIR_ERR_XML_ERROR,
>> +                               _("invalid PCI address for scsi_host "
>> +                                 "'parent' '%s'"), parent);
>> +                goto cleanup;
>> +            }
>> +        }
>> +
>> +        /* sysfs always exposes the PCI address like "0000:00:1f:2",
>> +         * this do the padding if the address prodived by user is

s/prodived/provided

>> +         * not padded (e.g. 0:0:2:0).
>> +         */
>> +        if (strlen(tokens[0]) != 4) {
>> +            if (!(padstr = virStringPad(tokens[0], '0', 4, false)))
>> +                goto cleanup;
>> +
>> +            virBufferAsprintf(&buf, "%s", padstr);
>> +            VIR_FREE(padstr);
>> +        } else {
>> +            virBufferAsprintf(&buf, "%s", tokens[0]);
>> +        }
>> +
>> +        for (i = 1; i < 3; i++) {
>> +            if (strlen(tokens[i]) != 2) {
>> +                if (!(padstr = virStringPad(tokens[i], '0', 2, false)))
>> +                    goto error;
>> +                virBufferAsprintf(&buf, "%s", padstr);

I think the following syntax will avoid any sort of virStringPad() and
whatever is going on above

virBufferAsprintf(&buf, "%04x:02x:02x:%s",
                  atoi(tokens[0], atoi(tokens[1]),
                  atoi(tokens[2], tokens[3]);

Assuming of course that each field is a base16 value and atoi() is "OK"
to use here...


John

> With the attached diff squashed in:
> 
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>