[libvirt] [PATCH 01/10] Replace sscanf in legacy device address parsing

[Matthias Bolte]

2010/3/30 Eric Blake <eblake at redhat.com>:
> On 03/30/2010 10:20 AM, Matthias Bolte wrote:
>> +static int
>> +virDomainParseLegacyDeviceAddress(char *devaddr,
>> +                                  virDomainDevicePCIAddressPtr pci)
>> +{
>> +    char *tmp = devaddr + strspn(devaddr, " \t\r\n");
>
> Why skip leading whitespace yourself...
>
>> +
>> +    /* expected format: <domain>:<bus>:<slot> */
>> +    if (virStrToLong_ui(tmp, &tmp, 16, &pci->domain) < 0 ||
>
> when virStrToLong_ui already does the same thing for you?

Because, I missed that fact that virStrToLong_i (actually strtol)
already skips initial whitspaces like scanf does.

I need to rework the whole series, because it's done based on a wrong
assumption.

>> +        tmp == NULL || *tmp != ':')
>
> tmp cannot be NULL at this point.

Correct, I should have tested this first, before assuming something wrong.

>> +        return -1;
>> +
>> +    if (virStrToLong_ui(tmp + 1, &tmp, 16, &pci->bus) < 0 ||
>> +        tmp == NULL || *tmp != ':')
>
> Likewise.
>
>> +        return -1;
>> +
>> +    if (virStrToLong_ui(tmp + 1, &tmp, 16, &pci->slot) < 0)
>
> Do we care if there is any garbage in *tmp at this point?

At this point &tmp is passed to virStrToLong_ui, in order to ignore
possible garbage after the last number, like scanf does. If there is
garbage and we pass NULL as second parameter virStrToLong_ui returns
an error.

The general question for this series is, do we want to maintain
scanf's trailing garbage-ignoring, or not.

>> +        return -1;
>> +
>> +    return 0;
>> +}
>>
>>  int
>>  virDomainDiskDefAssignAddress(virDomainDiskDefPtr def)
>> @@ -1541,10 +1561,8 @@ virDomainDiskDefParseXML(xmlNodePtr node,
>>      }
>>
>>      if (devaddr) {
>> -        if (sscanf(devaddr, "%x:%x:%x",
>> -                   &def->info.addr.pci.domain,
>> -                   &def->info.addr.pci.bus,
>> -                   &def->info.addr.pci.slot) < 3) {
>> +        if (virDomainParseLegacyDeviceAddress(devaddr,
>> +                                              &def->info.addr.pci) < 3) {
>
> Oops - virDomainParseLegacyDeviceAddress returns 0, not 3, on success.

Damn! :)

> The other two conversions in this patch looked okay.
>
> By the way, eventually your patch series should probably enable
> sc_prohibit_atoi_atof in cfg.mk's local-checks-to-skip (if you haven't
> already been experimenting with that).  Turning that on will allow 'make
> syntax-check' to catch all uses of scanf/atoi.
>

The posted series is just the first part, the plan is to enable that
check once scanf/atoi is gone.

I posted this first part, before doing the rest in order to get
comments and reviews. And as your comments show it was a good idea to
do so :)

Matthias