2010/3/30 Eric Blake <eblake@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