3:48 a.m.
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