[libvirt] [PATCHv2 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 cases. Changes in v2: - don't test tmp for NULL - note in commit message if parsing is stricter now - don't leak in PRUnicharToInt (vbox) - strip a trailing \n in openvzGetVPSUUID, as it was done before - don't use strspn to skip whitespaces, virStrToLong* does this already - fix virStrToLong* return value tests to check for < 0 instead of < 3 or != 1 - add STRSKIP to skip a given prefix of a string Thanks to Eric Blake and Jim Meyering. Matthias
Parsing is stricter now and doesn't accept trailing characters after the actual <domain>:<bus>:<slot> sequence anymore. --- src/conf/domain_conf.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bf4e657..0ad2e4a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1290,6 +1290,23 @@ cleanup: return ret; } +static int +virDomainParseLegacyDeviceAddress(char *devaddr, + virDomainDevicePCIAddressPtr pci) +{ + char *tmp; + + /* expected format: <domain>:<bus>:<slot> */ + if (/* domain */ + virStrToLong_ui(devaddr, &tmp, 16, &pci->domain) < 0 || *tmp != ':' || + /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &pci->bus) < 0 || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, NULL, 16, &pci->slot) < 0) + return -1; + + return 0; +} int virDomainDiskDefAssignAddress(virDomainDiskDefPtr def) @@ -1541,10 +1558,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) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse devaddr parameter '%s'"), devaddr); @@ -1901,10 +1916,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); @@ -3222,10 +3235,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/31/2010 03:41 PM, Matthias Bolte wrote:
Parsing is stricter now and doesn't accept trailing characters after the actual <domain>:<bus>:<slot> sequence anymore.
+static int +virDomainParseLegacyDeviceAddress(char *devaddr, + virDomainDevicePCIAddressPtr pci) +{ + char *tmp; + + /* expected format: <domain>:<bus>:<slot> */ + if (/* domain */ + virStrToLong_ui(devaddr, &tmp, 16, &pci->domain) < 0 || *tmp != ':' || + /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &pci->bus) < 0 || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, NULL, 16, &pci->slot) < 0) + return -1; + + return 0; +}
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Parsing is stricter now and doesn't accept trailing characters after the actual value anymore. --- 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 876b3ef..fbf6f33 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1239,7 +1239,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; @@ -1253,7 +1253,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; @@ -1277,7 +1277,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 = @@ -1331,7 +1331,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 = @@ -1659,7 +1659,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
On 03/31/2010 03:41 PM, Matthias Bolte wrote:
Parsing is stricter now and doesn't accept trailing characters after the actual value anymore.
The other instances are okay, but...
@@ -1659,7 +1659,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)
This looks gross. Since priority is unsigned int, why not just use: virStrToLong_ui(,,,&priority) >= 0 if (priority <= MAX_RULE_PRIORITY) ACK if you make that tweak. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
virParseVersionString uses virStrToLong_ui instead of sscanf. This also fixes a bug in the UML driver, that always returned 0 as version number. Introduce STRSKIP to check if a string has a certain prefix and to skip this prefix. --- src/esx/esx_driver.c | 29 ++++++----------------------- src/internal.h | 1 + src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 10 ++-------- src/openvz/openvz_conf.c | 14 ++++++++------ src/uml/uml_conf.h | 2 +- src/uml/uml_driver.c | 17 +++++++++-------- src/util/util.c | 35 +++++++++++++++++++++++++++++++++++ src/util/util.h | 1 + src/vbox/vbox_tmpl.c | 14 ++++---------- src/xenapi/xenapi_driver.c | 20 +++++++++----------- 11 files changed, 77 insertions(+), 67 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 20376e9..7a92c4d 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -684,34 +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 != '.') { - goto failure; - } - - if (virStrToLong_ui(temp + 1, &temp, 10, &minor) < 0 || *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/internal.h b/src/internal.h index f82fbd2..807288b 100644 --- a/src/internal.h +++ b/src/internal.h @@ -57,6 +57,7 @@ # define STRNEQLEN(a,b,n) (strncmp(a,b,n) != 0) # define STRCASENEQLEN(a,b,n) (strncasecmp(a,b,n) != 0) # define STRPREFIX(a,b) (strncmp(a,b,strlen(b)) == 0) +# define STRSKIP(a,b) (STRPREFIX(a,b) ? (a) + strlen(b) : NULL) # define STREQ_NULLABLE(a, b) \ ((!(a) && !(b)) || ((a) && (b) && STREQ((a), (b)))) 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..7fc3cd1 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,14 @@ 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 ((tmp = STRSKIP(tmp, "vzctl version ")) == NULL) goto cleanup2; - } - version = (major * 1000 * 1000) + (minor * 1000) + micro; + 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..6c48a93 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2074,6 +2074,41 @@ 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 != '.') + return -1; + + if (virStrToLong_ui(tmp + 1, &tmp, 10, &minor) < 0 || *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 d48bb4d..dcfdc1e 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,18 +277,17 @@ xenapiGetVersion (virConnectPtr conn, unsigned long *hvVer) } } if (version) { - if (sscanf(version, "%ld.%ld.%ld", &major, &minor, &release) != 3) { + if (virParseVersionString(version, hvVer) < 0) 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); + _("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/31/2010 03:41 PM, Matthias Bolte wrote:
virParseVersionString uses virStrToLong_ui instead of sscanf.
This also fixes a bug in the UML driver, that always returned 0 as version number.
Introduce STRSKIP to check if a string has a certain prefix and to skip this prefix.
+++ b/src/esx/esx_driver.c @@ -684,34 +684,17 @@ static int esxGetVersion(virConnectPtr conn, unsigned long *version) { ... + 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);
String needs translation.
diff --git a/src/internal.h b/src/internal.h index f82fbd2..807288b 100644 --- a/src/internal.h +++ b/src/internal.h @@ -57,6 +57,7 @@ # define STRNEQLEN(a,b,n) (strncmp(a,b,n) != 0) # define STRCASENEQLEN(a,b,n) (strncasecmp(a,b,n) != 0) # define STRPREFIX(a,b) (strncmp(a,b,strlen(b)) == 0) +# define STRSKIP(a,b) (STRPREFIX(a,b) ? (a) + strlen(b) : NULL) ...
+ /* expected format: vzctl version <major>.<minor>.<micro> */ + if ((tmp = STRSKIP(tmp, "vzctl version ")) == NULL) goto cleanup2;
Yes, that looks nicer. Glad I did my thinking out loud, and for other's input on the name.
+++ b/src/util/util.c @@ -2074,6 +2074,41 @@ 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;
Coreutils uses an idiom bad_cast(str) to make it obvious that we are casting away the const, and that we have audited that the results are still sane (virStrToLong_ui does not modify its argument), while at the same time reducing the number of C-style casts that require you to think why we are casting. But introducing bad_cast() into libvirt would be a separate patch; your code is fine as-is for this round. ACK with the translation markup. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/4/1 Eric Blake <eblake@redhat.com>:
On 03/31/2010 03:41 PM, Matthias Bolte wrote:
virParseVersionString uses virStrToLong_ui instead of sscanf.
This also fixes a bug in the UML driver, that always returned 0 as version number.
Introduce STRSKIP to check if a string has a certain prefix and to skip this prefix.
+++ b/src/esx/esx_driver.c @@ -684,34 +684,17 @@ static int esxGetVersion(virConnectPtr conn, unsigned long *version) { ... + 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);
String needs translation.
Currently, no string in the ESX driver code is marked for translation. I'll fix that in a separate patch.
+++ b/src/util/util.c @@ -2074,6 +2074,41 @@ 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;
Coreutils uses an idiom bad_cast(str) to make it obvious that we are casting away the const, and that we have audited that the results are still sane (virStrToLong_ui does not modify its argument), while at the same time reducing the number of C-style casts that require you to think why we are casting. But introducing bad_cast() into libvirt would be a separate patch; your code is fine as-is for this round.
I can just pass str to the first virStrToLong_ui, as it takes a const char pointer as input. So I can get rid of the bad char pointer cast.
ACK with the translation markup.
As said before I defer that to a separate patch, that covers the whole ESX driver code. Matthias
The switch from %lli to %lld in virCgroupGetValueI64 is intended, as virCgroupGetValueU64 uses base 10 too, and virCgroupSetValueI64 uses %lld to format the number to string. Parsing is stricter now and doesn't accept trailing characters after the actual value anymore. --- 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/31/2010 03:41 PM, Matthias Bolte wrote:
The switch from %lli to %lld in virCgroupGetValueI64 is intended, as virCgroupGetValueU64 uses base 10 too, and virCgroupSetValueI64 uses %lld to format the number to string.
Parsing is stricter now and doesn't accept trailing characters after the actual value anymore.
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Parsing is stricter now and doesn't accept trailing characters after the actual value or non-number strings anymore. atoi just returns 0 in case it cannot parse a number from the given string. Now an error is reported for such a string. --- 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..14fdcda 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) + ret = -1; + g_pVBoxGlobalData->pFuncs->pfnUtf8Free(strUtf8); return ret; -- 1.6.3.3
On 03/31/2010 03:41 PM, Matthias Bolte wrote:
Parsing is stricter now and doesn't accept trailing characters after the actual value or non-number strings anymore. atoi just returns 0 in case it cannot parse a number from the given string. Now an error is reported for such a string.
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This also fixes a bug in xenXMDomainConfigParse where uninitialized memory would be used as MAC address if sscanf fails. --- 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/31/2010 03:41 PM, Matthias Bolte wrote:
This also fixes a bug in xenXMDomainConfigParse where uninitialized memory would be used as MAC address if sscanf fails.
My ACK from last round carries forward; thanks for improving the commit comment. -- 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 7fc3cd1..d447eb9 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -835,8 +835,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; @@ -859,8 +860,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, "\n", &saveptr); + + if (iden != NULL && uuidbuf != NULL && STREQ(iden, "#UUID:")) { if (virStrcpy(uuidstr, uuidbuf, len) == NULL) retval = -1; break; -- 1.6.3.3
On 03/31/2010 03:42 PM, Matthias Bolte wrote:
Also free 2k stack space. - sscanf(line, "%s %s\n", iden, uuidbuf); - if(STREQ(iden, "#UUID:")) { + iden = strtok_r(line, " ", &saveptr); + uuidbuf = strtok_r(NULL, "\n", &saveptr); + + if (iden != NULL && uuidbuf != NULL && STREQ(iden, "#UUID:")) {
ACK, and you did fix the bug from v1 that I had missed, by looking for \n instead of \0. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Parsing is stricter now and doesn't accept trailing characters after the actual value anymore. --- src/xenapi/xenapi_utils.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 60e3c8d..581bd90 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -309,7 +309,7 @@ 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) + 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/31/2010 03:42 PM, Matthias Bolte wrote:
Parsing is stricter now and doesn't accept trailing characters after the actual value anymore.
- if (sscanf(num, "%d", &pos) != 1) + if (virStrToLong_i(num, NULL, 10, &pos) < 0)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Parsing is stricter now and doesn't accept trailing characters after the actual value anymore. --- 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..691a940 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) < 0) { virXendError (conn, VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: invalid port number")); return -1; -- 1.6.3.3
On 03/31/2010 03:42 PM, Matthias Bolte wrote:
Parsing is stricter now and doesn't accept trailing characters after the actual value anymore.
- if (sscanf (p+1, "%d", &port_nr) != 1) { + if (virStrToLong_i(p+1, NULL, 10, &port_nr) < 0) {
ACK. -- 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 about the L modifier being unknown. Parsing is stricter now and doesn't accept trailing characters after the actual value anymore. Change domain from unsigned long long to unsigned int in pciWaitForDeviceCleanup, because everywhere else domain is handled as unsigned int too. --- src/util/pci.c | 53 ++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index 99ec22a..640e9f8 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -282,15 +282,23 @@ 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 != ':' || + /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || *tmp != '.' || + /* function */ + virStrToLong_ui(tmp + 1, NULL, 16, &function) < 0) { VIR_WARN("Unusual entry in " PCI_SYSFS "devices: %s", entry->d_name); continue; } @@ -913,11 +921,9 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher) { FILE *fp; char line[160]; + char *tmp; unsigned long long start, end; - int consumed; - char *rest; - unsigned long long domain; - int bus, slot, function; + unsigned int domain, bus, slot, function; int in_matching_device; int ret; size_t match_depth; @@ -945,22 +951,39 @@ 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 != '-' || + /* end */ + virStrToLong_ull(tmp + 1, &tmp, 16, &end) < 0 || + (tmp = STRSKIP(tmp, " : ")) == NULL) continue; - rest = line + consumed; - if (STRPREFIX(rest, matcher)) { + if (STRPREFIX(tmp, matcher)) { ret = 1; break; } } 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 != '-' || + /* end */ + virStrToLong_ull(tmp + 1, &tmp, 16, &end) < 0 || + (tmp = STRSKIP(tmp, " : ")) == NULL || + /* domain */ + virStrToLong_ui(tmp, &tmp, 16, &domain) < 0 || *tmp != ':' || + /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || *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/31/2010 03:42 PM, Matthias Bolte wrote:
This also fixes a problem with MinGW's GCC on Windows. GCC complained about the L modifier being unknown.
Parsing is stricter now and doesn't accept trailing characters after the actual value anymore.
+ /* expected format: <start>-<end> : <domain>:<bus>:<slot>.<function> */ + if (/* start */ + virStrToLong_ull(tmp, &tmp, 16, &start) < 0 || *tmp != '-' || + /* end */ + virStrToLong_ull(tmp + 1, &tmp, 16, &end) < 0 || + (tmp = STRSKIP(tmp, " : ")) == NULL || + /* domain */ + virStrToLong_ui(tmp, &tmp, 16, &domain) < 0 || *tmp != ':' || + /* bus */ + virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || *tmp != ':' || + /* slot */ + virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || *tmp != '.' || + /* function */ + virStrToLong_ui(tmp + 1, &tmp, 16, &function) < 0) continue;
According to your commit comments, shouldn't you use NULL instead of &tmp in that last virStrToLong_ui? ACK, possibly after tweaking that line. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/3/31 Matthias Bolte <matthias.bolte@googlemail.com>:
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 cases.
Changes in v2: - don't test tmp for NULL - note in commit message if parsing is stricter now - don't leak in PRUnicharToInt (vbox) - strip a trailing \n in openvzGetVPSUUID, as it was done before - don't use strspn to skip whitespaces, virStrToLong* does this already - fix virStrToLong* return value tests to check for < 0 instead of < 3 or != 1 - add STRSKIP to skip a given prefix of a string
Thanks to Eric Blake and Jim Meyering.
Matthias
I folded in Eric's comments and pushed the whole v2 series. Matthias
participants (2)
-
Eric Blake -
Matthias Bolte