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

[Martin Kletzander]

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