[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 06/20/2013 05:21 AM, Osier Yang wrote:
> On 20/06/13 02:28, John Ferlan wrote:
>> On 06/07/2013 01:16 PM, Osier Yang wrote:
>>> On 08/06/13 01:03, Osier Yang wrote:
>>
>>>>            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.
>>
> 
> I don't think so, indicate it requires 2 or 4 separators doesn't give the
> user what we expect clearly. That's why I use "duplicate" error messages,
> even if
> 
> +    if (!strchr(parent, '_') &&
> +        !strchr(parent, ':')) {
> 
> is false, we still have to let the user know what the format we expect for
> "parent" attribute.
> 
> 
>>>>                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...
> 
> glibc says atoi is absolete, and since it's not required to do any error
> checking, strtol is recommended.
> 
> In libvirt, we have wrapper for strtol: virStrTo*.
> 
> But I don't see it's better than using virStringPad if converting the string
> into int using virStringTo*.  We have to check the return value of virStringTo*
> anyway here, because the user could input crazy data, e.g.
> 
> 1234566789101112:01:02:02
> 
> Osier
> 

What if we separated the fields in the XML? It feels wrong to store data as a
string separated by underscores, only to have to parse it again.

Instead of
  <adapter type='scsi_host' parent='pci_0000_00_1f_2' unique_id='2'/>
We could do:
  <adapter type='scsi_host' unique_id='2'>
    <parent>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
    </parent>
  </adapter>

or:
<parent>
  <vendor>0x8086</vendor>
  <device>0x1e03</device>
</parent>

Jan


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]