[libvirt] [PATCH 00/10] Begin to replace scanf and atoi
This set of patches is a first step towards removing the scanf and atoi usage in libvirt. I began with the simple cases and post this now to get some feedback before I start to convert the more difficult ones. 01/10 Replace sscanf in legacy device address parsing 02/10 Replace sscanf in nwfilter rule parsing 03/10 Refactor major.minor.micro version parsing into a function 04/10 cgroup: Replace sscanf with virStrToLong_ll 05/10 vbox: Replace atoi with virStrToLong_i 06/10 xen: Use virParseMacAddr instead of sscanf 07/10 openvz: Use strtok_r instead of sscanf for VPS UUID parsing 08/10 xenapi: Use virStrToLong_i instead of sscanf for CPU map parsing 09/10 xen: Use virStrToLong_i instead of sscanf for XenD port parsing 10/10 Replace sscanf in PCI device address parsing src/conf/domain_conf.c | 38 +++++++++++++++++++--------- src/conf/nwfilter_conf.c | 10 +++--- src/esx/esx_driver.c | 31 ++++------------------ src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 10 +------ src/openvz/openvz_conf.c | 27 ++++++++++++------- src/uml/uml_conf.h | 2 +- src/uml/uml_driver.c | 17 ++++++------ src/util/cgroup.c | 3 +- src/util/pci.c | 59 ++++++++++++++++++++++++++++++++++--------- src/util/util.c | 37 +++++++++++++++++++++++++++ src/util/util.h | 1 + src/vbox/vbox_tmpl.c | 18 +++++-------- src/xen/xend_internal.c | 20 ++------------ src/xen/xm_internal.c | 19 +++---------- src/xenapi/xenapi_driver.c | 21 +++++++-------- src/xenapi/xenapi_utils.c | 3 +- 17 files changed, 179 insertions(+), 138 deletions(-) Matthias
--- src/conf/domain_conf.c | 38 ++++++++++++++++++++++++++------------ 1 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66aa53e..9a517ca 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1290,6 +1290,26 @@ cleanup: return ret; } +static int +virDomainParseLegacyDeviceAddress(char *devaddr, + virDomainDevicePCIAddressPtr pci) +{ + char *tmp = devaddr + strspn(devaddr, " \t\r\n"); + + /* expected format: <domain>:<bus>:<slot> */ + if (virStrToLong_ui(tmp, &tmp, 16, &pci->domain) < 0 || + tmp == NULL || *tmp != ':') + return -1; + + if (virStrToLong_ui(tmp + 1, &tmp, 16, &pci->bus) < 0 || + tmp == NULL || *tmp != ':') + return -1; + + if (virStrToLong_ui(tmp + 1, &tmp, 16, &pci->slot) < 0) + 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) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse devaddr parameter '%s'"), devaddr); @@ -1894,10 +1912,8 @@ virDomainNetDefParseXML(virCapsPtr caps, } 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) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse devaddr parameter '%s'"), devaddr); @@ -3215,10 +3231,8 @@ virDomainHostdevSubsysPciDefParseXML(const xmlNodePtr node, /* Legacy back-compat. Don't add any more attributes here */ char *devaddr = virXMLPropString(cur, "devaddr"); if (devaddr && - sscanf(devaddr, "%x:%x:%x", - &def->info.addr.pci.domain, - &def->info.addr.pci.bus, - &def->info.addr.pci.slot) < 3) { + virDomainParseLegacyDeviceAddress(devaddr, + &def->info.addr.pci) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse devaddr parameter '%s'"), devaddr); -- 1.6.3.3
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?
+ tmp == NULL || *tmp != ':')
tmp cannot be NULL at this point.
+ 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?
+ 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. 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. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
--- src/conf/nwfilter_conf.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 668918d..24b79a7 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1226,7 +1226,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_UINT8: storage_ptr = &item->u.u8; - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 0xff) { if (!validator) *(uint8_t *)storage_ptr = int_val; @@ -1240,7 +1240,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_UINT16: storage_ptr = &item->u.u16; - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 0xffff) { if (!validator) *(uint16_t *)storage_ptr = int_val; @@ -1264,7 +1264,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_IPMASK: storage_ptr = &item->u.u8; if (!virNWIPv4AddressParser(prop, &ipaddr)) { - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 32) { if (!validator) *(uint8_t *)storage_ptr = @@ -1319,7 +1319,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_IPV6MASK: storage_ptr = &item->u.u8; if (!virNWIPv6AddressParser(prop, &ipaddr)) { - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 128) { if (!validator) *(uint8_t *)storage_ptr = @@ -1607,7 +1607,7 @@ virNWFilterRuleParse(virConnectPtr conn, ret->priority = MAX_RULE_PRIORITY / 2; if (prio) { - if (sscanf(prio, "%d", (int *)&priority) == 1) { + if (virStrToLong_i(prio, NULL, 10, (int *)&priority) >= 0) { if ((int)priority >= 0 && priority <= MAX_RULE_PRIORITY) ret->priority = priority; } -- 1.6.3.3
Matthias Bolte wrote:
src/conf/nwfilter_conf.c | 10 +++++-----
Hi Matthias, It's great that you're removing all of these sscanf uses. I suppose the plan includes eventually enabling the syntax-check that prohibits them altogether.
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c @@ -1226,7 +1226,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) {
Not sure it's worth worrying about, but bear in mind that this patch does induce a semantic change: sscanf is more permissive, and returns "1" even if there's garbage in the "prop" string after a valid integer, while virStrToLong_i (with NULL param #2) will reject that same bogus input string. I think of this as a feature, but it probably deserves a note in the commit log, so if some libvirt client starts seeing mysterious new failures due to their previously-accepted bogus inputs, they might find this set of commits. Other than that, this patch looks fine.
2010/3/30 Jim Meyering <jim@meyering.net>:
Matthias Bolte wrote:
src/conf/nwfilter_conf.c | 10 +++++-----
Hi Matthias,
It's great that you're removing all of these sscanf uses. I suppose the plan includes eventually enabling the syntax-check that prohibits them altogether.
Yes, once scanf and atoi are gone, that rule should be activated.
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c @@ -1226,7 +1226,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) {
Not sure it's worth worrying about, but bear in mind that this patch does induce a semantic change: sscanf is more permissive, and returns "1" even if there's garbage in the "prop" string after a valid integer, while virStrToLong_i (with NULL param #2) will reject that same bogus input string.
Yes, I'm aware of that. In some cases it is necessary to pass a non-NULL value as second parameter to virStrToLong_i in order to have the same behavior as scanf. For example the new virParseVersionNumber function does this in order to ignore the suffix on the VirtualBox version number 3.1.4_OSE.
I think of this as a feature, but it probably deserves a note in the commit log, so if some libvirt client starts seeing mysterious new failures due to their previously-accepted bogus inputs, they might find this set of commits.
True this needs to be mentioned in the commit message. I think for some scanfs ignoring trailing stuff in the string should just be ignore and in other places trailing stuff should be considered as error.
Other than that, this patch looks fine.
Matthias
libvir-list-bounces@redhat.com wrote on 03/30/2010 12:20:26 PM:
--- src/conf/nwfilter_conf.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 668918d..24b79a7 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1226,7 +1226,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED,
case DATATYPE_UINT8: storage_ptr = &item->u.u8; - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 0xff) { if (!validator) *(uint8_t *)storage_ptr =
int_val;
@@ -1240,7 +1240,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED,
case DATATYPE_UINT16: storage_ptr = &item->u.u16; - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 0xffff) { if (!validator) *(uint16_t *)storage_ptr = int_val; @@ -1264,7 +1264,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_IPMASK: storage_ptr = &item->u.u8; if (!virNWIPv4AddressParser(prop, &ipaddr)) { - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 32) { if (!validator) *(uint8_t *)storage_ptr = @@ -1319,7 +1319,7 @@ virNWFilterRuleDetailsParse(virConnectPtr conn ATTRIBUTE_UNUSED, case DATATYPE_IPV6MASK: storage_ptr = &item->u.u8; if (!virNWIPv6AddressParser(prop, &ipaddr)) { - if (sscanf(prop, "%d", &int_val) == 1) { + if (virStrToLong_i(prop, NULL, 10, &int_val) >= 0) { if (int_val >= 0 && int_val <= 128) { if (!validator) *(uint8_t *)storage_ptr = @@ -1607,7 +1607,7 @@ virNWFilterRuleParse(virConnectPtr conn, ret->priority = MAX_RULE_PRIORITY / 2;
if (prio) { - if (sscanf(prio, "%d", (int *)&priority) == 1) { + if (virStrToLong_i(prio, NULL, 10, (int *)&priority) >= 0) { if ((int)priority >= 0 && priority <= MAX_RULE_PRIORITY) ret->priority = priority; } --
Looks good to me. Stefan
1.6.3.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
virParseVersionString uses virStrToLong_ui instead of sscanf. This also fixes a bug in the UML driver, that always returned 0 as version number. --- src/esx/esx_driver.c | 31 ++++++------------------------- src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 10 ++-------- src/openvz/openvz_conf.c | 16 ++++++++++------ src/uml/uml_conf.h | 2 +- src/uml/uml_driver.c | 17 +++++++++-------- src/util/util.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/util.h | 1 + src/vbox/vbox_tmpl.c | 14 ++++---------- src/xenapi/xenapi_driver.c | 21 ++++++++++----------- 10 files changed, 81 insertions(+), 69 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index bbe8a51..f745f9a 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -684,36 +684,17 @@ static int esxGetVersion(virConnectPtr conn, unsigned long *version) { esxPrivate *priv = conn->privateData; - char *temp; - unsigned int major, minor, release; - temp = (char *)priv->host->service->about->version; - - /* Expecting 'major.minor.release' format */ - if (virStrToLong_ui(temp, &temp, 10, &major) < 0 || temp == NULL || - *temp != '.') { - goto failure; - } - - if (virStrToLong_ui(temp + 1, &temp, 10, &minor) < 0 || temp == NULL || - *temp != '.') { - goto failure; - } + if (virParseVersionString(priv->host->service->about->version, + version) < 0) { + ESX_ERROR(VIR_ERR_INTERNAL_ERROR, + "Could not parse version number from '%s'", + priv->host->service->about->version); - if (virStrToLong_ui(temp + 1, NULL, 10, &release) < 0) { - goto failure; + return -1; } - *version = 1000000 * major + 1000 * minor + release; - return 0; - - failure: - ESX_ERROR(VIR_ERR_INTERNAL_ERROR, - "Expecting version to match 'major.minor.release', but got '%s'", - priv->host->service->about->version); - - return -1; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cc943f8..edb23c2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -648,6 +648,7 @@ virFilePid; virFileReadPid; virFileLinkPointsTo; virParseNumber; +virParseVersionString; virPipeReadUntilEOF; virAsprintf; virRun; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7ebc7ae..9caefa1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1951,20 +1951,14 @@ lxcActive(void) { static int lxcVersion(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long *version) { struct utsname ver; - int maj; - int min; - int rev; uname(&ver); - if (sscanf(ver.release, "%i.%i.%i", &maj, &min, &rev) != 3) { - lxcError(VIR_ERR_INTERNAL_ERROR, - _("Unknown release: %s"), ver.release); + if (virParseVersionString(ver.release, version) < 0) { + lxcError(VIR_ERR_INTERNAL_ERROR, _("Unknown release: %s"), ver.release); return -1; } - *version = (maj * 1000 * 1000) + (min * 1000) + rev; - return 0; } diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 3713a45..a580ff0 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -78,8 +78,8 @@ openvzExtractVersionInfo(const char *cmd, int *retversion) pid_t child; int newstdout = -1; int ret = -1, status; - unsigned int major, minor, micro; - unsigned int version; + unsigned long version; + char *tmp; if (retversion) *retversion = 0; @@ -93,12 +93,16 @@ openvzExtractVersionInfo(const char *cmd, int *retversion) if (len < 0) goto cleanup2; - if (sscanf(help, "vzctl version %u.%u.%u", - &major, &minor, µ) != 3) { + tmp = help; + + /* expected format: vzctl version <major>.<minor>.<micro> */ + if (! STRPREFIX(tmp, "vzctl version ")) goto cleanup2; - } - version = (major * 1000 * 1000) + (minor * 1000) + micro; + tmp += 14; /* = strlen("vzctl version ") */ + + if (virParseVersionString(tmp, &version) < 0) + goto cleanup2; if (retversion) *retversion = version; diff --git a/src/uml/uml_conf.h b/src/uml/uml_conf.h index 70c3f7d..4d434e1 100644 --- a/src/uml/uml_conf.h +++ b/src/uml/uml_conf.h @@ -44,7 +44,7 @@ struct uml_driver { int privileged; - unsigned int umlVersion; + unsigned long umlVersion; int nextvmid; virDomainObjList domains; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 835e5d4..08fbf93 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1225,17 +1225,18 @@ cleanup: static int umlGetVersion(virConnectPtr conn, unsigned long *version) { struct uml_driver *driver = conn->privateData; struct utsname ut; - int major, minor, micro; int ret = -1; - uname(&ut); - umlDriverLock(driver); - if (sscanf(ut.release, "%u.%u.%u", - &major, &minor, µ) != 3) { - umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot parse version %s"), ut.release); - goto cleanup; + + if (driver->umlVersion == 0) { + uname(&ut); + + if (virParseVersionString(ut.release, &driver->umlVersion) < 0) { + umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("cannot parse version %s"), ut.release); + goto cleanup; + } } *version = driver->umlVersion; diff --git a/src/util/util.c b/src/util/util.c index 62dc5f1..a669d17 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2074,6 +2074,43 @@ virParseNumber(const char **str) return (ret); } + +/** + * virParseVersionString: + * @str: const char pointer to the version string + * @version: unsigned long pointer to output the version number + * + * Parse an unsigned version number from a version string. Expecting + * 'major.minor.micro' format, ignoring an optional suffix. + * + * The major, minor and micro numbers are encoded into a single version number: + * + * 1000000 * major + 1000 * minor + micro + * + * Returns the 0 for success, -1 for error. + */ +int +virParseVersionString(const char *str, unsigned long *version) +{ + unsigned int major, minor, micro; + char *tmp = (char *)str; + + if (virStrToLong_ui(tmp, &tmp, 10, &major) < 0 || tmp == NULL || + *tmp != '.') + return -1; + + if (virStrToLong_ui(tmp + 1, &tmp, 10, &minor) < 0 || tmp == NULL || + *tmp != '.') + return -1; + + if (virStrToLong_ui(tmp + 1, &tmp, 10, µ) < 0) + return -1; + + *version = 1000000 * major + 1000 * minor + micro; + + return 0; +} + /** * virAsprintf * diff --git a/src/util/util.h b/src/util/util.h index 24dfbfc..c256117 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -189,6 +189,7 @@ int virMacAddrCompare (const char *mac1, const char *mac2); void virSkipSpaces(const char **str); int virParseNumber(const char **str); +int virParseVersionString(const char *str, unsigned long *version); int virAsprintf(char **strp, const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(2, 3); char *virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) ATTRIBUTE_RETURN_CHECK; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index f7a9b9f..05b075f 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -157,7 +157,7 @@ if (strUtf16) {\ typedef struct { virMutex lock; - int version; + unsigned long version; virCapsPtr caps; @@ -713,10 +713,7 @@ cleanup: } static int vboxExtractVersion(virConnectPtr conn, vboxGlobalData *data) { - unsigned int major = 0; - unsigned int minor = 0; - unsigned int micro = 0; - int ret = -1; + int ret = -1; PRUnichar *versionUtf16 = NULL; nsresult rc; @@ -729,20 +726,17 @@ static int vboxExtractVersion(virConnectPtr conn, vboxGlobalData *data) { VBOX_UTF16_TO_UTF8(versionUtf16, &vboxVersion); - if (sscanf(vboxVersion, "%u.%u.%u", &major, &minor, µ) == 3) + if (virParseVersionString(vboxVersion, &data->version) >= 0) ret = 0; VBOX_UTF8_FREE(vboxVersion); VBOX_COM_UNALLOC_MEM(versionUtf16); - } else { - ret = -1; } - data->version = (major * 1000 * 1000) + (minor * 1000) + micro; - if (ret != 0) vboxError(conn, VIR_ERR_INTERNAL_ERROR,"%s", "Cound not extract VirtualBox version"); + return ret; } diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 6e1183d..7988601 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -253,9 +253,8 @@ xenapiGetVersion (virConnectPtr conn, unsigned long *hvVer) xen_host host; xen_session *session = ((struct _xenapiPrivate *)(conn->privateData))->session; xen_string_string_map *result = NULL; - int i; + int i, ret = -1; char *version = NULL; - unsigned long major = 0, minor = 0, release = 0; if (!(xen_session_get_this_host(session, &host, session))) { xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, NULL); return -1; @@ -278,17 +277,17 @@ xenapiGetVersion (virConnectPtr conn, unsigned long *hvVer) } } if (version) { - if (sscanf(version, "%ld.%ld.%ld", &major, &minor, &release) != 3) { - xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, "Couldn't get version info"); - xen_string_string_map_free(result); - VIR_FREE(version); - return -1; - } - *hvVer = major * 1000000 + minor * 1000 + release; - VIR_FREE(version); + if (virParseVersionString(version, hvVer) < 0) + xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, + "Couldn't parse version info"); + else + ret = 0; xen_string_string_map_free(result); - return 0; + VIR_FREE(version); + return ret; } + xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, + "Couldn't get version info"); } return -1; } -- 1.6.3.3
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
+/** + * virParseVersionString: + * @str: const char pointer to the version string + * @version: unsigned long pointer to output the version number + * + * Parse an unsigned version number from a version string. Expecting + * 'major.minor.micro' format, ignoring an optional suffix. + * + * The major, minor and micro numbers are encoded into a single version number: + * + * 1000000 * major + 1000 * minor + micro + * + * Returns the 0 for success, -1 for error. + */ +int +virParseVersionString(const char *str, unsigned long *version) +{ + unsigned int major, minor, micro; + char *tmp = (char *)str; + + if (virStrToLong_ui(tmp, &tmp, 10, &major) < 0 || tmp == NULL ||
Another instance (2 times) where the tmp==NULL check is spurious. But I didn't see anything else wrong with this patch.
- if (sscanf(version, "%ld.%ld.%ld", &major, &minor, &release) != 3) { - xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, "Couldn't get version info"); - xen_string_string_map_free(result); - VIR_FREE(version); - return -1; - } - *hvVer = major * 1000000 + minor * 1000 + release; - VIR_FREE(version); + if (virParseVersionString(version, hvVer) < 0) + xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, + "Couldn't parse version info");
Not introduced by your patch, but should we be translating this error message? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
+/** + * virParseVersionString: + * @str: const char pointer to the version string + * @version: unsigned long pointer to output the version number + * + * Parse an unsigned version number from a version string. Expecting + * 'major.minor.micro' format, ignoring an optional suffix. + * + * The major, minor and micro numbers are encoded into a single version number: + * + * 1000000 * major + 1000 * minor + micro + * + * Returns the 0 for success, -1 for error. + */ +int +virParseVersionString(const char *str, unsigned long *version) +{ + unsigned int major, minor, micro; + char *tmp = (char *)str; + + if (virStrToLong_ui(tmp, &tmp, 10, &major) < 0 || tmp == NULL ||
Another instance (2 times) where the tmp==NULL check is spurious. But I didn't see anything else wrong with this patch.
- if (sscanf(version, "%ld.%ld.%ld", &major, &minor, &release) != 3) { - xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, "Couldn't get version info"); - xen_string_string_map_free(result); - VIR_FREE(version); - return -1; - } - *hvVer = major * 1000000 + minor * 1000 + release; - VIR_FREE(version); + if (virParseVersionString(version, hvVer) < 0) + xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR, + "Couldn't parse version info");
Not introduced by your patch, but should we be translating this error message?
Yes, definitely. Good catch. I added it to the list here: diff --git a/cfg.mk b/cfg.mk index 9b8ee00..45da56a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -215,6 +215,7 @@ msg_gen_function += virXenInotifyError msg_gen_function += virXenStoreError msg_gen_function += virXendError msg_gen_function += vshCloseLogFile +msg_gen_function += xenapiSessionErrorHandler msg_gen_function += xenUnifiedError msg_gen_function += xenXMError Then ran "make syntax-check" and get 53 new violations. BTW, I'm seeing a lot of these, too: nwfilter/nwfilter_ebiptables_driver.c:193: warning: format not a string literal and no format arguments [-Wformat-security] nwfilter/nwfilter_ebiptables_driver.c:204: warning: format not a string literal and no format arguments [-Wformat-security] nwfilter/nwfilter_ebiptables_dr...
--- src/util/cgroup.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 496d9d3..4cb09b6 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -12,7 +12,6 @@ #include <stdio.h> #include <stdint.h> -#include <inttypes.h> #ifdef HAVE_MNTENT_H # include <mntent.h> #endif @@ -374,7 +373,7 @@ static int virCgroupGetValueI64(virCgroupPtr group, if (rc != 0) goto out; - if (sscanf(strval, "%" SCNi64, value) != 1) + if (virStrToLong_ll(strval, NULL, 10, value) < 0) rc = -EINVAL; out: VIR_FREE(strval); -- 1.6.3.3
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
- if (sscanf(strval, "%" SCNi64, value) != 1) + if (virStrToLong_ll(strval, NULL, 10, value) < 0)
Oops. You effectively changed from %lli to %lld, which means the string now has to be decimal, instead of octal or hex. Did you mean to pass 0 as the third argument? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/3/31 Eric Blake <eblake@redhat.com>:
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
- if (sscanf(strval, "%" SCNi64, value) != 1) + if (virStrToLong_ll(strval, NULL, 10, value) < 0)
Oops. You effectively changed from %lli to %lld, which means the string now has to be decimal, instead of octal or hex. Did you mean to pass 0 as the third argument?
Good catch. I didn't notice that it was using %lli actually. But I think switching virCgroupGetValueI64 to %lld is no problem here. Because virCgroupGetValueU64 already uses virStrToLong_ull with base = 10 and virCgroupSetValueI64 uses %lld in its virAsprintf call. And I just noticed that virCgroupGetValueI64 is in an #if 0 block, so its not used yet. So it's safe to change it. Matthias
--- src/vbox/vbox_tmpl.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 05b075f..07996d4 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -598,7 +598,9 @@ static int PRUnicharToInt(PRUnichar *strUtf16) { if (!strUtf8) return -1; - ret = atoi(strUtf8); + if (virStrToLong_i(strUtf8, NULL, 10, &ret) < 0) + return -1; + g_pVBoxGlobalData->pFuncs->pfnUtf8Free(strUtf8); return ret; -- 1.6.3.3
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
--- src/vbox/vbox_tmpl.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 05b075f..07996d4 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -598,7 +598,9 @@ static int PRUnicharToInt(PRUnichar *strUtf16) { if (!strUtf8) return -1;
- ret = atoi(strUtf8); + if (virStrToLong_i(strUtf8, NULL, 10, &ret) < 0) + return -1; + g_pVBoxGlobalData->pFuncs->pfnUtf8Free(strUtf8);
Oops. Memory leak if strUtf8 was not valid. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/3/31 Eric Blake <eblake@redhat.com>:
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
--- src/vbox/vbox_tmpl.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 05b075f..07996d4 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -598,7 +598,9 @@ static int PRUnicharToInt(PRUnichar *strUtf16) { if (!strUtf8) return -1;
- ret = atoi(strUtf8); + if (virStrToLong_i(strUtf8, NULL, 10, &ret) < 0) + return -1; + g_pVBoxGlobalData->pFuncs->pfnUtf8Free(strUtf8);
Oops. Memory leak if strUtf8 was not valid.
Good catch. It seems that I missed many details in this series :( Matthias
--- src/xen/xend_internal.c | 18 ++---------------- src/xen/xm_internal.c | 19 +++++-------------- 2 files changed, 7 insertions(+), 30 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 46e19cd..0649d23 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1819,25 +1819,11 @@ xenDaemonParseSxprNets(virConnectPtr conn, tmp = sexpr_node(node, "device/vif/mac"); if (tmp) { - unsigned int mac[6]; - if (sscanf(tmp, "%02x:%02x:%02x:%02x:%02x:%02x", - (unsigned int*)&mac[0], - (unsigned int*)&mac[1], - (unsigned int*)&mac[2], - (unsigned int*)&mac[3], - (unsigned int*)&mac[4], - (unsigned int*)&mac[5]) != 6) { + if (virParseMacAddr(tmp, net->mac) < 0) { virXendError(conn, VIR_ERR_INTERNAL_ERROR, - _("malformed mac address '%s'"), - tmp); + _("malformed mac address '%s'"), tmp); goto cleanup; } - net->mac[0] = mac[0]; - net->mac[1] = mac[1]; - net->mac[2] = mac[2]; - net->mac[3] = mac[3]; - net->mac[4] = mac[4]; - net->mac[5] = mac[5]; } if (model && diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index ddbd2fe..0d42b01 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1115,20 +1115,11 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { goto no_memory; if (mac[0]) { - unsigned int rawmac[6]; - sscanf(mac, "%02x:%02x:%02x:%02x:%02x:%02x", - (unsigned int*)&rawmac[0], - (unsigned int*)&rawmac[1], - (unsigned int*)&rawmac[2], - (unsigned int*)&rawmac[3], - (unsigned int*)&rawmac[4], - (unsigned int*)&rawmac[5]); - net->mac[0] = rawmac[0]; - net->mac[1] = rawmac[1]; - net->mac[2] = rawmac[2]; - net->mac[3] = rawmac[3]; - net->mac[4] = rawmac[4]; - net->mac[5] = rawmac[5]; + if (virParseMacAddr(mac, net->mac) < 0) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("malformed mac address '%s'"), mac); + goto cleanup; + } } if (bridge[0] || STREQ(script, "vif-bridge") || -- 1.6.3.3
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
--- src/xen/xend_internal.c | 18 ++---------------- src/xen/xm_internal.c | 19 +++++-------------- 2 files changed, 7 insertions(+), 30 deletions(-)
ACK - nice cleanup.
if (mac[0]) { - unsigned int rawmac[6]; - sscanf(mac, "%02x:%02x:%02x:%02x:%02x:%02x", - (unsigned int*)&rawmac[0], - (unsigned int*)&rawmac[1], - (unsigned int*)&rawmac[2], - (unsigned int*)&rawmac[3], - (unsigned int*)&rawmac[4], - (unsigned int*)&rawmac[5]); - net->mac[0] = rawmac[0]; - net->mac[1] = rawmac[1]; - net->mac[2] = rawmac[2]; - net->mac[3] = rawmac[3]; - net->mac[4] = rawmac[4]; - net->mac[5] = rawmac[5]; + if (virParseMacAddr(mac, net->mac) < 0) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("malformed mac address '%s'"), mac); + goto cleanup; + }
Especially since it fixes a bug where we could have used uninitialized memory if sscanf had failed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Also free 2k stack space. --- src/openvz/openvz_conf.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index a580ff0..9b8412f 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -837,8 +837,9 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) { char conf_file[PATH_MAX]; char line[1024]; - char uuidbuf[1024]; - char iden[1024]; + char *saveptr; + char *uuidbuf; + char *iden; int fd, ret; int retval = 0; @@ -861,8 +862,10 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) break; } - sscanf(line, "%s %s\n", iden, uuidbuf); - if(STREQ(iden, "#UUID:")) { + iden = strtok_r(line, " ", &saveptr); + uuidbuf = strtok_r(NULL, "", &saveptr); + + if (iden != NULL && uuidbuf != NULL && STREQ(iden, "#UUID:")) { if (virStrcpy(uuidstr, uuidbuf, len) == NULL) retval = -1; break; -- 1.6.3.3
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
@@ -861,8 +862,10 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) break; }
- sscanf(line, "%s %s\n", iden, uuidbuf); - if(STREQ(iden, "#UUID:")) { + iden = strtok_r(line, " ", &saveptr); + uuidbuf = strtok_r(NULL, "", &saveptr); + + if (iden != NULL && uuidbuf != NULL && STREQ(iden, "#UUID:")) {
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/3/31 Eric Blake <eblake@redhat.com>:
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
@@ -861,8 +862,10 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) break; }
- sscanf(line, "%s %s\n", iden, uuidbuf); - if(STREQ(iden, "#UUID:")) { + iden = strtok_r(line, " ", &saveptr); + uuidbuf = strtok_r(NULL, "", &saveptr); + + if (iden != NULL && uuidbuf != NULL && STREQ(iden, "#UUID:")) {
ACK.
Actually, there is a bug in here. I noticed it while I reviewed this series for the problems you already mentioned. uuidbuf ends up containing VIR_UUID_STRING_BUFLEN + 1 (for the trailing \n) chars. But uuidstr has only room for VIR_UUID_STRING_BUFLEN indicated by len and virStrcpy will fail. If fixed this in v2 of this series by using "\n" as separator for the second strtok_r call. I think I'm going to post v2 later this day. Matthias
--- src/xenapi/xenapi_utils.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 697ad39..790eebf 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -309,7 +309,8 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen) bzero(cpumap, maplen); num = strtok_r(mask, ",", &bp); while (num != NULL) { - if (sscanf(num, "%d", &pos) != 1) + num += strspn(num, " \t"); + if (virStrToLong_i(num, NULL, 10, &pos) < 0) return; if (pos < 0 || pos > max_bits - 1) VIR_WARN ("number in str %d exceeds cpumap's max bits %d", pos, max_bits); -- 1.6.3.3
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
--- src/xenapi/xenapi_utils.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 697ad39..790eebf 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -309,7 +309,8 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen) bzero(cpumap, maplen); num = strtok_r(mask, ",", &bp); while (num != NULL) { - if (sscanf(num, "%d", &pos) != 1) + num += strspn(num, " \t"); + if (virStrToLong_i(num, NULL, 10, &pos) < 0)
No need to skip the whitespace yourself. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
--- src/xen/xend_internal.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 0649d23..c910439 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4683,7 +4683,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, else if ((p = strrchr (uri, ':')) != NULL) { /* "hostname:port" */ int port_nr, n; - if (sscanf (p+1, "%d", &port_nr) != 1) { + if (virStrToLong_i(p+1, NULL, 10, &port_nr) != 1) { virXendError (conn, VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: invalid port number")); return -1; -- 1.6.3.3
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
--- src/xen/xend_internal.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 0649d23..c910439 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4683,7 +4683,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, else if ((p = strrchr (uri, ':')) != NULL) { /* "hostname:port" */ int port_nr, n;
- if (sscanf (p+1, "%d", &port_nr) != 1) { + if (virStrToLong_i(p+1, NULL, 10, &port_nr) != 1) {
Worth documenting in the commit log that the change to no longer permit trailing garbage is intentional. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This also fixes a problem with MinGW's GCC on Windows. GCC complained bout the L modifier being unknown. --- src/util/pci.c | 59 +++++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index 99ec22a..6aa2fe0 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -282,15 +282,26 @@ pciIterDevices(pciIterPredicate predicate, } while ((entry = readdir(dir))) { - unsigned domain, bus, slot, function; + unsigned int domain, bus, slot, function; pciDevice *check; + char *tmp; /* Ignore '.' and '..' */ if (entry->d_name[0] == '.') continue; - if (sscanf(entry->d_name, "%x:%x:%x.%x", - &domain, &bus, &slot, &function) < 4) { + /* expected format: <domain>:<bus>:<slot>.<function> */ + if (/* domain */ + virStrToLong_ui(entry->d_name, &tmp, 16, &domain) < 0 || + tmp == NULL || *tmp != ':' || + /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || + tmp == NULL || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || + tmp == NULL || *tmp != '.' || + /* function */ + virStrToLong_ui(tmp + 1, NULL, 16, &function) < 0) { VIR_WARN("Unusual entry in " PCI_SYSFS "devices: %s", entry->d_name); continue; } @@ -914,10 +925,9 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) FILE *fp; char line[160]; unsigned long long start, end; - int consumed; char *rest; - unsigned long long domain; - int bus, slot, function; + char *tmp; + unsigned int domain, bus, slot, function; int in_matching_device; int ret; size_t match_depth; @@ -945,10 +955,18 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) * of these situations */ if (in_matching_device && (strspn(line, " ") == (match_depth + 2))) { - if (sscanf(line, "%Lx-%Lx : %n", &start, &end, &consumed) != 2) + tmp = line + strspn(line, " "); + + /* expected format: <start>-<end> : <suffix> */ + if (/* start */ + virStrToLong_ull(tmp, &tmp, 16, &start) < 0 || + tmp == NULL || *tmp != '-' || + /* end */ + virStrToLong_ull(tmp + 1, &tmp, 16, &end) < 0 || + tmp == NULL || ! STRPREFIX(tmp, " : ")) continue; - rest = line + consumed; + rest = tmp + 3; /* = strlen(" : ") */ if (STRPREFIX(rest, matcher)) { ret = 1; break; @@ -956,11 +974,26 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) } else { in_matching_device = 0; - if (sscanf(line, "%Lx-%Lx : %n", &start, &end, &consumed) != 2) - continue; - - rest = line + consumed; - if (sscanf(rest, "%Lx:%x:%x.%x", &domain, &bus, &slot, &function) != 4) + tmp = line + strspn(line, " "); + + /* expected format: <start>-<end> : <domain>:<bus>:<slot>.<function> */ + if (/* start */ + virStrToLong_ull(tmp, &tmp, 16, &start) < 0 || + tmp == NULL || *tmp != '-' || + /* end */ + virStrToLong_ull(tmp + 1, &tmp, 16, &end) < 0 || + tmp == NULL || ! STRPREFIX(tmp, " : ") || + /* domain */ + virStrToLong_ui(tmp + 3, &tmp, 16, &domain) < 0 || + tmp == NULL || *tmp != ':' || + /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || + tmp == NULL || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || + tmp == NULL || *tmp != '.' || + /* function */ + virStrToLong_ui(tmp + 1, &tmp, 16, &function) < 0) continue; if (domain != dev->domain || bus != dev->bus || slot != dev->slot || -- 1.6.3.3
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
This also fixes a problem with MinGW's GCC on Windows. GCC complained bout the L modifier being unknown.
s/bout/about/
--- src/util/pci.c | 59 +++++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 46 insertions(+), 13 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c index 99ec22a..6aa2fe0 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -282,15 +282,26 @@ pciIterDevices(pciIterPredicate predicate, }
while ((entry = readdir(dir))) { - unsigned domain, bus, slot, function; + unsigned int domain, bus, slot, function; pciDevice *check; + char *tmp;
/* Ignore '.' and '..' */ if (entry->d_name[0] == '.') continue;
- if (sscanf(entry->d_name, "%x:%x:%x.%x", - &domain, &bus, &slot, &function) < 4) { + /* expected format: <domain>:<bus>:<slot>.<function> */ + if (/* domain */ + virStrToLong_ui(entry->d_name, &tmp, 16, &domain) < 0 || + tmp == NULL || *tmp != ':' ||
Useless checks for NULL throughout.
+ /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || + tmp == NULL || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || + tmp == NULL || *tmp != '.' || + /* function */ + virStrToLong_ui(tmp + 1, NULL, 16, &function) < 0) {
Another trailing garbage enforcement worth documenting.
VIR_WARN("Unusual entry in " PCI_SYSFS "devices: %s", entry->d_name); continue; } @@ -914,10 +925,9 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) FILE *fp; char line[160]; unsigned long long start, end; - int consumed; char *rest; - unsigned long long domain; - int bus, slot, function; + char *tmp; + unsigned int domain, bus, slot, function;
Why the change in the size of domain?
int in_matching_device; int ret; size_t match_depth; @@ -945,10 +955,18 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) * of these situations */ if (in_matching_device && (strspn(line, " ") == (match_depth + 2))) { - if (sscanf(line, "%Lx-%Lx : %n", &start, &end, &consumed) != 2) + tmp = line + strspn(line, " "); + + /* expected format: <start>-<end> : <suffix> */ + if (/* start */ + virStrToLong_ull(tmp, &tmp, 16, &start) < 0 || + tmp == NULL || *tmp != '-' || + /* end */ + virStrToLong_ull(tmp + 1, &tmp, 16, &end) < 0 || + tmp == NULL || ! STRPREFIX(tmp, " : ")) continue;
- rest = line + consumed; + rest = tmp + 3; /* = strlen(" : ") */
Hmm, thinking aloud here: your patch series has introduced several instances of checking STRPREFIX, then advancing to the end of the prefix. Maybe it's time to introduce a helper macro that does both the check and advances the pointer to the end of the match? Something like: #define STPCMP(ptr, cmp) \ (STRPREFIX (*(ptr), cmp) ? (ptr) += strlen (cmp) : NULL) STPCMP(&tmp, " : "); if (tmp == NULL) error... else use tmp... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/31/2010 10:38 AM, Eric Blake wrote:
Hmm, thinking aloud here: your patch series has introduced several instances of checking STRPREFIX, then advancing to the end of the prefix. Maybe it's time to introduce a helper macro that does both the check and advances the pointer to the end of the match? Something like:
#define STPCMP(ptr, cmp) \ (STRPREFIX (*(ptr), cmp) ? (ptr) += strlen (cmp) : NULL)
STPCMP(&tmp, " : "); if (tmp == NULL) error... else use tmp...
More thinking aloud - that looks a bit confusing compared to other stp* interfaces. Maybe: #define STPCMP(str, cmp) \ (STRPREFIX (str, cmp) ? (str) + strlen (cmp) : NULL) if ((tmp = STPCMP(tmp, " : ") == NULL) error... else use tmp... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Mar 31, 2010 at 10:48:47AM -0600, Eric Blake wrote:
On 03/31/2010 10:38 AM, Eric Blake wrote:
Hmm, thinking aloud here: your patch series has introduced several instances of checking STRPREFIX, then advancing to the end of the prefix. Maybe it's time to introduce a helper macro that does both the check and advances the pointer to the end of the match? Something like:
#define STPCMP(ptr, cmp) \ (STRPREFIX (*(ptr), cmp) ? (ptr) += strlen (cmp) : NULL)
STPCMP(&tmp, " : "); if (tmp == NULL) error... else use tmp...
More thinking aloud - that looks a bit confusing compared to other stp* interfaces. Maybe:
#define STPCMP(str, cmp) \ (STRPREFIX (str, cmp) ? (str) + strlen (cmp) : NULL) if ((tmp = STPCMP(tmp, " : ") == NULL) error... else use tmp...
I think the name is a little unclear here - I wouldn't be expecting something named 'CMP' it to modify the argument. How about calling it STR_ADVANCE or STR_SKIP Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2010/3/31 Eric Blake <eblake@redhat.com>:
On 03/31/2010 10:38 AM, Eric Blake wrote:
Hmm, thinking aloud here: your patch series has introduced several instances of checking STRPREFIX, then advancing to the end of the prefix. Maybe it's time to introduce a helper macro that does both the check and advances the pointer to the end of the match? Something like:
#define STPCMP(ptr, cmp) \ (STRPREFIX (*(ptr), cmp) ? (ptr) += strlen (cmp) : NULL)
STPCMP(&tmp, " : "); if (tmp == NULL) error... else use tmp...
More thinking aloud - that looks a bit confusing compared to other stp* interfaces. Maybe:
#define STPCMP(str, cmp) \ (STRPREFIX (str, cmp) ? (str) + strlen (cmp) : NULL) if ((tmp = STPCMP(tmp, " : ") == NULL) error... else use tmp...
Good idea, but I think the name STPCMP is confusing. I misread it as STRCMP at first. Maybe STRPREFIXSKIP or STRPFXSKIP are better names. Matthias
2010/3/31 Eric Blake <eblake@redhat.com>:
On 03/30/2010 10:20 AM, Matthias Bolte wrote:
This also fixes a problem with MinGW's GCC on Windows. GCC complained bout the L modifier being unknown.
VIR_WARN("Unusual entry in " PCI_SYSFS "devices: %s", entry->d_name); continue; } @@ -914,10 +925,9 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) FILE *fp; char line[160]; unsigned long long start, end; - int consumed; char *rest; - unsigned long long domain; - int bus, slot, function; + char *tmp; + unsigned int domain, bus, slot, function;
Why the change in the size of domain?
domain is parsed and stored everywhere else as unsigned int. Do the same here, there's no reason for domain to be handled as 64bit Matthias
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Jim Meyering -
Matthias Bolte -
Stefan Berger