[libvirt] [PATCH v3] json: fix interface locale dependency
I'm trying to get a optimized and reusable version of the code fixing libvirt's locale dependency. This time I'm sending two versions of the patch: v1) adds static function virNumberToStr() that converts one number to string base on a parameter defining the format. For float, double and long double there are functions that should be used in the code. v2) adds function for converting only double to string. This version has also a little bit differently separated the conditional code. I personally like the second version better. for two reasons. 1) it looks better, 2) there is no need for special-formatting with C locale unless the result is being parsed by machine in which case the '%lf' is always sufficient. Martin Kletzander (1): json: fix interface locale dependency bootstrap.conf | 1 + configure.ac | 2 +- src/libvirt_private.syms | 3 + src/util/json.c | 2 +- src/util/util.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 7 ++ 6 files changed, 150 insertions(+), 2 deletions(-) -- 1.7.8.6
libvirt creates invalid commands if wrong locale is selected. For example with locale that uses comma as a decimal point, JSON commands created with decimal numbers are invalid because comma separates the entries in JSON. Fortunately even when decimal point is affected, thousands grouping is not, because for grouping to be enabled with *printf, there has to be an apostrophe flag specified (and supported). This patch adds specific internal function for converting doubles to strings with C locale. --- bootstrap.conf | 1 + configure.ac | 2 +- src/libvirt_private.syms | 1 + src/util/json.c | 2 +- src/util/util.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 2 + 6 files changed, 69 insertions(+), 2 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index c112ccd..a4e1c2f 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -62,6 +62,7 @@ ioctl isatty largefile listen +localeconv maintainer-makefile manywarnings mkstemp diff --git a/configure.ac b/configure.ac index 8a04d91..e614b48 100644 --- a/configure.ac +++ b/configure.ac @@ -133,7 +133,7 @@ dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \ getpwuid_r getuid initgroups kill mmap posix_fallocate posix_memalign \ - regexec sched_getaffinity]) + regexec sched_getaffinity newlocale]) dnl Availability of pthread functions (if missing, win32 threading is dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c023dbf..018d3a9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1140,6 +1140,7 @@ virArgvToString; virAsprintf; virBuildPathInternal; virDirCreate; +virDoubleToStr; virEnumFromString; virEnumToString; virEventAddHandle; diff --git a/src/util/json.c b/src/util/json.c index 5132989..40d50a1 100644 --- a/src/util/json.c +++ b/src/util/json.c @@ -201,7 +201,7 @@ virJSONValuePtr virJSONValueNewNumberDouble(double data) { virJSONValuePtr val = NULL; char *str; - if (virAsprintf(&str, "%lf", data) < 0) + if (virDoubleToStr(&str, data) < 0) return NULL; val = virJSONValueNewNumber(str); VIR_FREE(str); diff --git a/src/util/util.c b/src/util/util.c index e5bb27b..c318153 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -44,6 +44,7 @@ #include <signal.h> #include <termios.h> #include <pty.h> +#include <locale.h> #if HAVE_LIBDEVMAPPER_H # include <libdevmapper.h> @@ -2060,6 +2061,68 @@ int virEnumFromString(const char *const*types, return -1; } +/* In case thread-safe locales are available */ +#if HAVE_NEWLOCALE + +locale_t virLocale; + +static int +virLocaleOnceInit(void) +{ + virLocale = newlocale(LC_ALL_MASK, "C", NULL); + if (!virLocale) + return -1; + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virLocale) +#endif + +/** + * virDoubleToStr + * + * converts double to string with C locale (thread-safe). + * + * Returns -1 on error, size of the string otherwise. + */ +int +virDoubleToStr(char **strp, double number) +{ + int ret = -1; + +#if HAVE_NEWLOCALE + + locale_t old_loc; + + if (virLocaleInitialize() < 0) + goto error; + + old_loc = uselocale(virLocale); + ret = virAsprintf(strp, "%lf", number); + uselocale(old_loc); + +#else + + char *radix, *tmp; + struct lconv *lc; + + if ((ret = virVasprintf(strp, "%lf", number) < 0) + goto error; + + lc = localeconv(); + radix = lc->decimal_point; + tmp = strstr(*strp, radix); + if (tmp) { + *tmp = '.'; + if (strlen(radix) > 1) + memmove(tmp + 1, tmp + strlen(radix), strlen(*strp) - (tmp - str)); + } + +#endif /* HAVE_NEWLOCALE */ + error: + return ret; +} + const char *virEnumToString(const char *const*types, unsigned int ntypes, int type) diff --git a/src/util/util.h b/src/util/util.h index d151c27..a4e34c6 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -209,6 +209,8 @@ char *virStrcpy(char *dest, const char *src, size_t destbytes) ATTRIBUTE_RETURN_CHECK; # define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest)) +int virDoubleToStr(char **strp, double number) ATTRIBUTE_NONNULL(1); + int virDiskNameToIndex(const char* str); char *virIndexToDiskName(int idx, const char *prefix); -- 1.7.8.6
On 08/12/2012 01:55 AM, Martin Kletzander wrote:
libvirt creates invalid commands if wrong locale is selected. For example with locale that uses comma as a decimal point, JSON commands created with decimal numbers are invalid because comma separates the entries in JSON. Fortunately even when decimal point is affected, thousands grouping is not, because for grouping to be enabled with *printf, there has to be an apostrophe flag specified (and supported).
This patch adds specific internal function for converting doubles to strings with C locale. --- bootstrap.conf | 1 + configure.ac | 2 +- src/libvirt_private.syms | 1 + src/util/json.c | 2 +- src/util/util.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 2 + 6 files changed, 69 insertions(+), 2 deletions(-)
+++ b/configure.ac @@ -133,7 +133,7 @@ dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \ getpwuid_r getuid initgroups kill mmap posix_fallocate posix_memalign \ - regexec sched_getaffinity]) + regexec sched_getaffinity newlocale])
Insert in sorted order.
+/* In case thread-safe locales are available */ +#if HAVE_NEWLOCALE + +locale_t virLocale;
Mark this static.
+ +static int +virLocaleOnceInit(void) +{ + virLocale = newlocale(LC_ALL_MASK, "C", NULL);
NULL is technically incorrect here; locale_t is not necessarily a pointer type. POSIX says the third argument should be '(locale_t)0'.
+ if (!virLocale) + return -1; + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virLocale) +#endif + +/** + * virDoubleToStr + * + * converts double to string with C locale (thread-safe). + * + * Returns -1 on error, size of the string otherwise. + */ +int +virDoubleToStr(char **strp, double number) +{ + int ret = -1; + +#if HAVE_NEWLOCALE + + locale_t old_loc; + + if (virLocaleInitialize() < 0) + goto error; + + old_loc = uselocale(virLocale); + ret = virAsprintf(strp, "%lf", number);
Technically, %f and %lf are identical. I also wonder if we should be favoring %g instead of %f here, or if we need to care about the rounding errors introduced by the fact that the default precision of %f is smaller than the actual precision in the double value being converted. But as this is the pre-existing format, changing it would deserve a separate commit, so it does not impact this commit.
+++ b/src/util/util.h @@ -209,6 +209,8 @@ char *virStrcpy(char *dest, const char *src, size_t destbytes) ATTRIBUTE_RETURN_CHECK; # define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest))
+int virDoubleToStr(char **strp, double number) ATTRIBUTE_NONNULL(1); +
I'd also add ATTRIBUTE_RETURN_CHECK. Of the two alternatives, I prefer this one for now. If we ever have need for further enhancements down the road, we can add them at that time. No need to add code that will be unused by being more flexible than our current needs. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/13/2012 05:51 PM, Eric Blake wrote:
Of the two alternatives, I prefer this one for now. If we ever have need for further enhancements down the road, we can add them at that time. No need to add code that will be unused by being more flexible than our current needs.
I see I've sent them in different order, in the cover letter I switched them by mistake. I also think this is cleaner one. I'll send the fixed version right away. Martin
libvirt creates invalid commands if wrong locale is selected. For example with locale that uses comma as a decimal point, JSON commands created with decimal numbers are invalid because comma separates the entries in JSON. Fortunately even when decimal point is affected, thousands grouping is not, because for grouping to be enabled with *printf, there has to be an apostrophe flag specified (and supported). This patch adds specific internal function for converting numbers to strings with C locale and helper functions for each of the float, double and long double. --- bootstrap.conf | 1 + configure.ac | 2 +- src/libvirt_private.syms | 3 + src/util/json.c | 2 +- src/util/util.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 7 ++ 6 files changed, 150 insertions(+), 2 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index c112ccd..a4e1c2f 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -62,6 +62,7 @@ ioctl isatty largefile listen +localeconv maintainer-makefile manywarnings mkstemp diff --git a/configure.ac b/configure.ac index 8a04d91..e614b48 100644 --- a/configure.ac +++ b/configure.ac @@ -133,7 +133,7 @@ dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \ getpwuid_r getuid initgroups kill mmap posix_fallocate posix_memalign \ - regexec sched_getaffinity]) + regexec sched_getaffinity newlocale]) dnl Availability of pthread functions (if missing, win32 threading is dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c023dbf..ba7fc76 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1140,6 +1140,7 @@ virArgvToString; virAsprintf; virBuildPathInternal; virDirCreate; +virDoubleToStr; virEnumFromString; virEnumToString; virEventAddHandle; @@ -1172,6 +1173,7 @@ virFileUnlock; virFileWaitForDevices; virFileWriteStr; virFindFileInPath; +virFloatToStr; virGetGroupID; virGetGroupName; virGetHostname; @@ -1185,6 +1187,7 @@ virHexToBin; virIndexToDiskName; virIsDevMapperDevice; virKillProcess; +virLongDoubleToStr; virParseNumber; virParseVersionString; virPipeReadUntilEOF; diff --git a/src/util/json.c b/src/util/json.c index 5132989..1a58a16 100644 --- a/src/util/json.c +++ b/src/util/json.c @@ -201,7 +201,7 @@ virJSONValuePtr virJSONValueNewNumberDouble(double data) { virJSONValuePtr val = NULL; char *str; - if (virAsprintf(&str, "%lf", data) < 0) + if (virDoubleToStr(&str, 'f', data) < 0) return NULL; val = virJSONValueNewNumber(str); VIR_FREE(str); diff --git a/src/util/util.c b/src/util/util.c index e5bb27b..903a249 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -44,6 +44,7 @@ #include <signal.h> #include <termios.h> #include <pty.h> +#include <locale.h> #if HAVE_LIBDEVMAPPER_H # include <libdevmapper.h> @@ -2060,6 +2061,142 @@ int virEnumFromString(const char *const*types, return -1; } +/* In case thread-safe locales are available */ +#if HAVE_NEWLOCALE + +locale_t virLocale; + +static int +virLocaleOnceInit(void) { + virLocale = newlocale(LC_ALL_MASK, "C", NULL); + if (!virLocale) + return -1; + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virLocale) +#endif + +/** + * virNumberToStr + * + * converts *one* number to string with C locale (thread-safe). + * + * This function is intended for use in helper files only (hence static). + * For converting numbers in other code, use the helper functions below. + * + * Second parameter is the format for the number ("%lle", "%lG", etc.) + * + * Returns -1 on error, size of the string otherwise. + */ +static int +virNumberToStr(char **strp, const char *fmt, ...) +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); + +static int +virNumberToStr(char **strp, const char *fmt, ...) +{ + int ret = -1; + va_list ap; + +#if HAVE_NEWLOCALE + locale_t old_loc; + if (virLocaleInitialize() < 0) + goto error; + old_loc = uselocale(virLocale); +#else /* HAVE_NEWLOCALE */ + char *radix, *tmp; + struct lconv *lc; +#endif /* HAVE_NEWLOCALE */ + + va_start(ap, fmt); + ret = virVasprintf(strp, fmt, ap); + va_end(ap); + +#if HAVE_NEWLOCALE + uselocale(old_loc); +#else /* HAVE_NEWLOCALE */ + if (ret < 0) + goto error; + + lc = localeconv(); + radix = lc->decimal_point; + tmp = strstr(*strp, radix); + if (tmp) { + *tmp = '.'; + if (strlen(radix) > 1) + memmove(tmp + 1, tmp + strlen(radix), strlen(*strp) - (tmp - str)); + } +#endif /* HAVE_NEWLOCALE */ + + error: + return ret; +} + +/** + * virFloatToStr + * + * the same as virDoubleToStr, but for floats. The function virDoubleToStr is + * called because float gets promoted to double with vararg functions anyway. + */ +int +virFloatToStr(char **strp, char fmt, float number) +{ + return virDoubleToStr(strp, fmt, (double)number); +} + +/** + * virDoubleToStr + * + * converts double to string with C locale (thread-safe) + * + * Second parameter may be one of the floating point format characters + * used with *printf functions (e, E, f, g, G). + * + * Returns -1 on error, size of the string otherwise. + */ +int +virDoubleToStr(char **strp, char fmt, double number) +{ + char f[] = "%lf"; + int ret = -1; + + if (strchr("eEfgG", fmt) == NULL) + goto error; + f[2] = fmt; + + ret = virNumberToStr(strp, f, number); + + error: + return ret; +} + +/** + * virLongDoubleToStr + * + * converts long double to string with C locale (thread-safe) + * + * Second parameter may be one of the floating point format characters + * used with *printf functions (e, E, f, g, G). + * + * Returns -1 on error, size of the string otherwise. + */ +int +virLongDoubleToStr(char **strp, char fmt, long double number) +{ + char f[] = "%llf"; + int ret = -1; + + if (strchr("eEfgG", fmt) == NULL) + goto error; + f[3] = fmt; + + ret = virNumberToStr(strp, f, number); + + error: + return ret; +} + const char *virEnumToString(const char *const*types, unsigned int ntypes, int type) diff --git a/src/util/util.h b/src/util/util.h index d151c27..3db8b8e 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -209,6 +209,13 @@ char *virStrcpy(char *dest, const char *src, size_t destbytes) ATTRIBUTE_RETURN_CHECK; # define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest)) +int virFloatToStr(char **strp, char fmt, float number) + ATTRIBUTE_NONNULL(1); +int virDoubleToStr(char **strp, char fmt, double number) + ATTRIBUTE_NONNULL(1); +int virLongDoubleToStr(char **strp, char fmt, long double number) + ATTRIBUTE_NONNULL(1); + int virDiskNameToIndex(const char* str); char *virIndexToDiskName(int idx, const char *prefix); -- 1.7.8.6
participants (2)
-
Eric Blake -
Martin Kletzander