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

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.

> @@ -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).