[libvirt] [PATCH 09/11] storage_scsi: Allow the direct PCI address for 'parent'
Osier Yang
jyang at redhat.com
Thu Jun 20 08:48:36 UTC 2013
On 20/06/13 15:04, Ján Tomko wrote:
> 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>
>
As the cover letter says, it uses the name what nodedev represents. I
woudn't
want to store the data in that strange way if nodedev doesn't reresent
device
like that.
Osier
More information about the libvir-list
mailing list