[libvirt] [PATCH v2] json: fix interface locale dependency
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 printing with C locale and a fallback for the particular case with double formatting in case these functions are not supported. --- v2: - added support for xlocale - fallback option kept, but main function moved to util.c src/libvirt_private.syms | 1 + src/util/json.c | 23 ++++++++++++++++++++++- src/util/util.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 2 ++ 4 files changed, 67 insertions(+), 1 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 79b4a18..6ce87bf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1138,6 +1138,7 @@ safezero; virArgvToString; virAsprintf; virBuildPathInternal; +virCasprintf; virDirCreate; virEnumFromString; virEnumToString; diff --git a/src/util/json.c b/src/util/json.c index 5132989..3dfa039 100644 --- a/src/util/json.c +++ b/src/util/json.c @@ -22,6 +22,7 @@ #include <config.h> +#include <locale.h> #include "json.h" #include "memory.h" @@ -201,8 +202,28 @@ virJSONValuePtr virJSONValueNewNumberDouble(double data) { virJSONValuePtr val = NULL; char *str; - if (virAsprintf(&str, "%lf", data) < 0) + + int ret = virCasprintf(&str, "%lf", data) + + if (ret == -1) { return NULL; + } else if (ret == -2) { + char *radix, *tmp; + struct lconv *lc; + + if ((ret = virAsprintf(&str, "%lf", data)) < 0) + return NULL; + + lc = localeconv(); + radix = lc->decimal_point; + tmp = strstr(str, radix); + if (tmp) { + *tmp = '.'; + if (strlen(radix) > 1) + memmove(tmp + 1, tmp + strlen(radix), strlen(str) - (tmp - str)); + } + } + val = virJSONValueNewNumber(str); VIR_FREE(str); return val; diff --git a/src/util/util.c b/src/util/util.c index e5bb27b..de090a6 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> @@ -2003,6 +2004,47 @@ virAsprintf(char **strp, const char *fmt, ...) } /** + * virCasprintf + * + * the same as virAsprintf, but with C locale (thread-safe) + * + * If thread-safe locales aren't supported, then the return value is -2, + * no memory (or other vasnprintf errors) results in -1. + * When successful, the value returned by virAsprintf is passed. + */ +#if HAVE_XLOCALE_H +int +virCasprintf(char **strp, const char *fmt, ...) +{ + va_list ap; + int ret = -1; + locale_t c_loc, old_loc; + c_loc = newlocale(LC_ALL, "C", NULL); + if (!c_loc) + goto no_memory; + + old_loc = uselocale(c_loc); + + va_start(ap, fmt); + ret = virVasprintf(strp, fmt, ap); + va_end(ap); + + uselocale(old_loc); + + no_memory: + freelocale(c_loc); + return ret; +} +#else +int +virCasprintf(char **strp ATTRIBUTE_UNUSED, + const char *fmt ATTRIBUTE_UNUSED, ...) +{ + return -2; +} +#endif + +/** * virStrncpy * * A safe version of strncpy. The last parameter is the number of bytes diff --git a/src/util/util.h b/src/util/util.h index d151c27..4b7f286 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -201,6 +201,8 @@ int virParseVersionString(const char *str, unsigned long *version, bool allowMissing); int virAsprintf(char **strp, const char *fmt, ...) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); +int virCasprintf(char **strp, const char *fmt, ...) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); int virVasprintf(char **strp, const char *fmt, va_list list) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 0); char *virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) -- 1.7.8.6
On 08/09/2012 07:44 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 printing with C locale and a fallback for the particular case with double formatting in case these functions are not supported. --- v2: - added support for xlocale - fallback option kept, but main function moved to util.c
src/libvirt_private.syms | 1 + src/util/json.c | 23 ++++++++++++++++++++++- src/util/util.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 2 ++ 4 files changed, 67 insertions(+), 1 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 79b4a18..6ce87bf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1138,6 +1138,7 @@ safezero; virArgvToString; virAsprintf; virBuildPathInternal; +virCasprintf;
Hmm. Our <c-ctype.h> header prefers the c_ prefix for designating the C locale, but then again that is from gnulib, and 'c_virAsprintf' doesn't match our naming conventions. But I do think it looks better as 'virCAsprintf' (asprintf for the C local, and not some new function 'casprintf').
+++ b/src/util/json.c @@ -22,6 +22,7 @@
#include <config.h> +#include <locale.h>
Why do we need <locale.h> in json.c? That just means we didn't create the correct helper function in util.h.
#include "json.h" #include "memory.h" @@ -201,8 +202,28 @@ virJSONValuePtr virJSONValueNewNumberDouble(double data) { virJSONValuePtr val = NULL; char *str; - if (virAsprintf(&str, "%lf", data) < 0) + + int ret = virCasprintf(&str, "%lf", data)
virCasprintf() seems like overkill, for now. Since printing a floating point value is the only case where locale matters, we should be able to provide a single helper function that guarantees a formatted float, rather than trying to provide a generic virCasprintf that covers all possible format strings. I don't even think we need to worry about things like 0 padding, justification, or precision - either we are outputting things to the user (where alignment and precision make things look nice, but then we WANT to obey locale), or we are converting to an internal string (where we want the string to be reproducible, and worrying about formatting is unneeded bulk). That is, I think we are better off with this signature in util.c: /* Return a malloc'd string representing D in the C locale, or NULL on OOM. */ char *virDoubleToStr(double d); (and possibly virFloatToStr and virLongDoubleToStr if we ever have reason for them later on; or even add an option argument to let the user choose between a, e, f, or g format). At any rate, the fallback code for replacing the decimal character should live in util.c, not in the clients.
+ if (ret == -1) { return NULL; + } else if (ret == -2) { + char *radix, *tmp; + struct lconv *lc;
I'm still worried about whether 'struct lconv' will compile on mingw. Then again, any system that lacks localeconf() probably also lacks any locale that would use ',' for the decimal separator, so maybe appropriate ifdef protection is all we need.
+ + if ((ret = virAsprintf(&str, "%lf", data)) < 0) + return NULL; + + lc = localeconv(); + radix = lc->decimal_point; + tmp = strstr(str, radix); + if (tmp) { + *tmp = '.'; + if (strlen(radix) > 1) + memmove(tmp + 1, tmp + strlen(radix), strlen(str) - (tmp - str));
Again, this should all live in util.c; the json.c code should just be a one-liner conversion of double to string.
#if HAVE_LIBDEVMAPPER_H # include <libdevmapper.h> @@ -2003,6 +2004,47 @@ virAsprintf(char **strp, const char *fmt, ...) }
/** + * virCasprintf + * + * the same as virAsprintf, but with C locale (thread-safe) + * + * If thread-safe locales aren't supported, then the return value is -2, + * no memory (or other vasnprintf errors) results in -1. + * When successful, the value returned by virAsprintf is passed. + */ +#if HAVE_XLOCALE_H
And where does HAVE_XLOCALE_H get defined? Autoconf conventions say it would map to <xlocale.h> existing, but that is a non-standard header name. Not to mention that we really care about whether newlocale and setlocale exist.
+int +virCasprintf(char **strp, const char *fmt, ...) +{ + va_list ap; + int ret = -1; + locale_t c_loc, old_loc; + c_loc = newlocale(LC_ALL, "C", NULL);
This is expensive to recreate on the fly for every caller; I think it should be a static variable and created only once (the virInitOnce stuff makes the most sense here). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/09/2012 06:17 PM, Eric Blake wrote:
virCasprintf() seems like overkill, for now. Since printing a floating point value is the only case where locale matters, we should be able to provide a single helper function that guarantees a formatted float, rather than trying to provide a generic virCasprintf that covers all possible format strings. I don't even think we need to worry about things like 0 padding, justification, or precision - either we are outputting things to the user (where alignment and precision make things look nice, but then we WANT to obey locale), or we are converting to an internal string (where we want the string to be reproducible, and worrying about formatting is unneeded bulk). That is, I think we are better off with this signature in util.c:
/* Return a malloc'd string representing D in the C locale, or NULL on OOM. */ char *virDoubleToStr(double d);
(and possibly virFloatToStr and virLongDoubleToStr if we ever have reason for them later on; or even add an option argument to let the user choose between a, e, f, or g format). At any rate, the fallback code for replacing the decimal character should live in util.c, not in the clients.
I see I should've stuck with one of my versions and don't listen to guys around here ;-) I'm with you on this one.
+ if (ret == -1) { return NULL; + } else if (ret == -2) { + char *radix, *tmp; + struct lconv *lc;
I'm still worried about whether 'struct lconv' will compile on mingw. Then again, any system that lacks localeconf() probably also lacks any locale that would use ',' for the decimal separator, so maybe appropriate ifdef protection is all we need.
As we based it on glib code, I'd say: "they have it like this" :)
@@ -2003,6 +2004,47 @@ virAsprintf(char **strp, const char *fmt, ...) }
/** + * virCasprintf + * + * the same as virAsprintf, but with C locale (thread-safe) + * + * If thread-safe locales aren't supported, then the return value is -2, + * no memory (or other vasnprintf errors) results in -1. + * When successful, the value returned by virAsprintf is passed. + */ +#if HAVE_XLOCALE_H
And where does HAVE_XLOCALE_H get defined? Autoconf conventions say it would map to <xlocale.h> existing, but that is a non-standard header name. Not to mention that we really care about whether newlocale and setlocale exist.
I tried to create a have_xlocale with AC_COMPILE_IFELSE but since uselocale, newlocale and freelocale are part of libc, I haven't managed to make it fail. Even after removing all the header files the only problem was with NULL, but no problem with these function. I'll recreate what I threw away and I'll send it as a new version and in the meantime I'll try to find a machine/configuration the test will fail on.
+int +virCasprintf(char **strp, const char *fmt, ...) +{ + va_list ap; + int ret = -1; + locale_t c_loc, old_loc; + c_loc = newlocale(LC_ALL, "C", NULL);
This is expensive to recreate on the fly for every caller; I think it should be a static variable and created only once (the virInitOnce stuff makes the most sense here).
OK, will do. Thanks for the review and ideas, I'll jump right on that ;) Martin
On 08/10/2012 03:43 AM, Martin Kletzander wrote:
I'm still worried about whether 'struct lconv' will compile on mingw. Then again, any system that lacks localeconf() probably also lacks any locale that would use ',' for the decimal separator, so maybe appropriate ifdef protection is all we need.
As we based it on glib code, I'd say: "they have it like this" :)
But part of the code depends on the configure time checks that were performed, and glib uses different configure checks than libvirt's use of gnulib. While copying the code is easy, tweaking the configure script to pick up the right conditions is where all the porting fun comes when porting a glib function to live in isolation.
+#if HAVE_XLOCALE_H
And where does HAVE_XLOCALE_H get defined? Autoconf conventions say it would map to <xlocale.h> existing, but that is a non-standard header name. Not to mention that we really care about whether newlocale and setlocale exist.
I tried to create a have_xlocale with AC_COMPILE_IFELSE but since uselocale, newlocale and freelocale are part of libc, I haven't managed to make it fail.
Here's how I'd do it. In configure.ac, add: AC_CHECK_FUNCS_ONCE([newlocale]) if newlocale exits, then so do uselocale and freelocale; if newlocale does not exist, then we go to fallback code. In bootstrap.conf, add localeconv to gnulib_modules. Then gnulib's localeconv is guaranteed to exist, and we are guaranteed that our fallback code will compile even on mingw. In util.c, write your code to do: #if HAVE_NEWLOCALE code using thread-safe locale swapping to our once-init C locale_t #else code using strstr of the localeconv()->decimal_point #endif -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/10/2012 05:39 PM, Eric Blake wrote:
On 08/10/2012 03:43 AM, Martin Kletzander wrote:
I'm still worried about whether 'struct lconv' will compile on mingw. Then again, any system that lacks localeconf() probably also lacks any locale that would use ',' for the decimal separator, so maybe appropriate ifdef protection is all we need.
As we based it on glib code, I'd say: "they have it like this" :)
But part of the code depends on the configure time checks that were performed, and glib uses different configure checks than libvirt's use of gnulib. While copying the code is easy, tweaking the configure script to pick up the right conditions is where all the porting fun comes when porting a glib function to live in isolation.
+#if HAVE_XLOCALE_H
And where does HAVE_XLOCALE_H get defined? Autoconf conventions say it would map to <xlocale.h> existing, but that is a non-standard header name. Not to mention that we really care about whether newlocale and setlocale exist.
I tried to create a have_xlocale with AC_COMPILE_IFELSE but since uselocale, newlocale and freelocale are part of libc, I haven't managed to make it fail.
Here's how I'd do it. In configure.ac, add:
AC_CHECK_FUNCS_ONCE([newlocale])
if newlocale exits, then so do uselocale and freelocale; if newlocale does not exist, then we go to fallback code.
In bootstrap.conf, add localeconv to gnulib_modules. Then gnulib's localeconv is guaranteed to exist, and we are guaranteed that our fallback code will compile even on mingw.
In util.c, write your code to do:
#if HAVE_NEWLOCALE code using thread-safe locale swapping to our once-init C locale_t #else code using strstr of the localeconv()->decimal_point #endif
I partially had that when I read this mail. I finished it now, but to be honest, even though it works, it's reusable and so on it doesn't look very good. I think I'll cook one more version of that and will post both of them. And if you'd be so kind and had a look once more at that, I'd appreciate that. Martin
participants (2)
-
Eric Blake -
Martin Kletzander