[libvirt] [PATCH v2] json: fix interface locale dependency

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120809/e603c655/attachment-0001.sig>