7:36 a.m.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/util/virstring.c | 96 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 60 insertions(+), 36 deletions(-)
diff --git a/src/util/virstring.c b/src/util/virstring.c
index feea5be05198..6125725364f3 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base,
#if HAVE_NEWLOCALE
static locale_t virLocale;
+static locale_t virLocaleOld;
static int
virLocaleOnceInit(void)
@@ -533,7 +534,58 @@ virLocaleOnceInit(void)
}
VIR_ONCE_GLOBAL_INIT(virLocale);
-#endif
+
+static int
+virLocaleSet(void)
+{
+ if (virLocaleInitialize() < 0)
+ return -1;
+ virLocaleOld = uselocale(virLocale);
+ return 0;
+}
+
+static void
+virLocaleRevert(void)
+{
+ uselocale(virLocaleOld);
+}
+
+static void
+virLocaleFixupRadix(char **strp ATTRIBUTE_UNUSED)
+{
+}
+
+#else /* !HAVE_NEWLOCALE */
+
+static int
+virLocaleSet(void)
+{
+ return 0;
+}
+
+static void
+virLocaleRevert(void)
+{
+}
+
+static void
+virLocaleFixupRadix(char **strp)
+{
+ char *radix, *tmp;
+ struct lconv *lc;
+
+ 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 - *strp));
+ }
+}
+
+#endif /* !HAVE_NEWLOCALE */
+
/**
* virStrToDouble
@@ -552,17 +604,11 @@ virStrToDouble(char const *s,
int err;
errno = 0;
-#if HAVE_NEWLOCALE
- locale_t old_loc;
- if (virLocaleInitialize() < 0)
+ if (virLocaleSet() < 0)
return -1;
-
- old_loc = uselocale(virLocale);
-#endif
val = strtod(s, &p); /* exempt from syntax-check */
-#if HAVE_NEWLOCALE
- uselocale(old_loc);
-#endif
+ virLocaleRevert();
+
err = (errno || (!end_ptr && *p) || p == s);
if (end_ptr)
*end_ptr = p;
@@ -584,36 +630,14 @@ virDoubleToStr(char **strp, double number)
{
int ret = -1;
-#if HAVE_NEWLOCALE
-
- locale_t old_loc;
-
- if (virLocaleInitialize() < 0)
- goto error;
+ if (virLocaleSet() < 0)
+ return -1;
- old_loc = uselocale(virLocale);
ret = virAsprintf(strp, "%lf", number);
- uselocale(old_loc);
-
-#else
-
- char *radix, *tmp;
- struct lconv *lc;
-
- if ((ret = virAsprintf(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 - *strp));
- }
+ virLocaleRevert();
+ virLocaleFixupRadix(strp);
-#endif /* HAVE_NEWLOCALE */
- error:
return ret;
}
--
2.13.1
8:49 a.m.
On Thu, Jun 22, 2017 at 14:36:53 +0200, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/util/virstring.c | 96 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 60 insertions(+), 36 deletions(-)
diff --git a/src/util/virstring.c b/src/util/virstring.c
index feea5be05198..6125725364f3 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base,
#if HAVE_NEWLOCALE
static locale_t virLocale;
+static locale_t virLocaleOld;
This is not a thread local variable ...
static int
virLocaleOnceInit(void)
@@ -533,7 +534,58 @@ virLocaleOnceInit(void)
}
VIR_ONCE_GLOBAL_INIT(virLocale);
-#endif
+
+static int
+virLocaleSet(void)
+{
+ if (virLocaleInitialize() < 0)
+ return -1;
+ virLocaleOld = uselocale(virLocale);
... and this is called multiple times ...
+ return 0;
+}
+
+static void
+virLocaleRevert(void)
+{
+ uselocale(virLocaleOld);
... so while this at this moment will behave correctly, since
virLocaleOld will only be ever set to the same old locale, the general
approach is not correct.
3:21 a.m.
On Thu, Jun 22, 2017 at 03:49:03PM +0200, Peter Krempa wrote:
On Thu, Jun 22, 2017 at 14:36:53 +0200, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/util/virstring.c | 96 ++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 60 insertions(+), 36 deletions(-)
>
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index feea5be05198..6125725364f3 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base,
> #if HAVE_NEWLOCALE
>
> static locale_t virLocale;
> +static locale_t virLocaleOld;
This is not a thread local variable ...
Oh. shoot, you're right. So we need to wrap it in our struct that will
be defined conditionally as well, or allocate it on the heap, copy it
there and set a pointer to it as one additional parameter or make thread
local variable, but that one will need to be a pointer to the locale, so
we'll need to allocate it on the heap as well. I don't like the way
this is going. Anyone else feel free to continue on this if you want to
clean this up, but I like the few small conditional blocks better,
especially since we have them in only two functions.
3:32 a.m.
On Fri, Jun 23, 2017 at 10:21:32AM +0200, Martin Kletzander wrote:
On Thu, Jun 22, 2017 at 03:49:03PM +0200, Peter Krempa wrote:
> On Thu, Jun 22, 2017 at 14:36:53 +0200, Martin Kletzander wrote:
> > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> > ---
> > src/util/virstring.c | 96
++++++++++++++++++++++++++++++++--------------------
> > 1 file changed, 60 insertions(+), 36 deletions(-)
> >
> > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > index feea5be05198..6125725364f3 100644
> > --- a/src/util/virstring.c
> > +++ b/src/util/virstring.c
> > @@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base,
> > #if HAVE_NEWLOCALE
> >
> > static locale_t virLocale;
> > +static locale_t virLocaleOld;
>
> This is not a thread local variable ...
>
Oh. shoot, you're right. So we need to wrap it in our struct that will
be defined conditionally as well, or allocate it on the heap, copy it
there and set a pointer to it as one additional parameter or make thread
local variable, but that one will need to be a pointer to the locale, so
we'll need to allocate it on the heap as well. I don't like the way
this is going. Anyone else feel free to continue on this if you want to
clean this up, but I like the few small conditional blocks better,
especially since we have them in only two functions.
Just have virLocaleSet() return the original locale to the caller, and
it can pass it back in when it reverts, avoiding any global variables.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
4:42 a.m.
On Fri, Jun 23, 2017 at 09:32:07AM +0100, Daniel P. Berrange wrote:
On Fri, Jun 23, 2017 at 10:21:32AM +0200, Martin Kletzander wrote:
> On Thu, Jun 22, 2017 at 03:49:03PM +0200, Peter Krempa wrote:
> > On Thu, Jun 22, 2017 at 14:36:53 +0200, Martin Kletzander wrote:
> > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> > > ---
> > > src/util/virstring.c | 96
++++++++++++++++++++++++++++++++--------------------
> > > 1 file changed, 60 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > > index feea5be05198..6125725364f3 100644
> > > --- a/src/util/virstring.c
> > > +++ b/src/util/virstring.c
> > > @@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int
base,
> > > #if HAVE_NEWLOCALE
> > >
> > > static locale_t virLocale;
> > > +static locale_t virLocaleOld;
> >
> > This is not a thread local variable ...
> >
>
> Oh. shoot, you're right. So we need to wrap it in our struct that will
> be defined conditionally as well, or allocate it on the heap, copy it
> there and set a pointer to it as one additional parameter or make thread
> local variable, but that one will need to be a pointer to the locale, so
> we'll need to allocate it on the heap as well. I don't like the way
> this is going. Anyone else feel free to continue on this if you want to
> clean this up, but I like the few small conditional blocks better,
> especially since we have them in only two functions.
Just have virLocaleSet() return the original locale to the caller, and
it can pass it back in when it reverts, avoiding any global variables.
You cannot do that when you don't have 'locale_t', hence the
complications. Or is the existence of locale_t somehow guaranteed by
gnulib? If it is, then that's solved.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:45 a.m.
On Fri, Jun 23, 2017 at 11:42:01AM +0200, Martin Kletzander wrote:
On Fri, Jun 23, 2017 at 09:32:07AM +0100, Daniel P. Berrange wrote:
> On Fri, Jun 23, 2017 at 10:21:32AM +0200, Martin Kletzander wrote:
> > On Thu, Jun 22, 2017 at 03:49:03PM +0200, Peter Krempa wrote:
> > > On Thu, Jun 22, 2017 at 14:36:53 +0200, Martin Kletzander wrote:
> > > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> > > > ---
> > > > src/util/virstring.c | 96
++++++++++++++++++++++++++++++++--------------------
> > > > 1 file changed, 60 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > > > index feea5be05198..6125725364f3 100644
> > > > --- a/src/util/virstring.c
> > > > +++ b/src/util/virstring.c
> > > > @@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s, char **end_ptr,
int base,
> > > > #if HAVE_NEWLOCALE
> > > >
> > > > static locale_t virLocale;
> > > > +static locale_t virLocaleOld;
> > >
> > > This is not a thread local variable ...
> > >
> >
> > Oh. shoot, you're right. So we need to wrap it in our struct that will
> > be defined conditionally as well, or allocate it on the heap, copy it
> > there and set a pointer to it as one additional parameter or make thread
> > local variable, but that one will need to be a pointer to the locale, so
> > we'll need to allocate it on the heap as well. I don't like the way
> > this is going. Anyone else feel free to continue on this if you want to
> > clean this up, but I like the few small conditional blocks better,
> > especially since we have them in only two functions.
>
> Just have virLocaleSet() return the original locale to the caller, and
> it can pass it back in when it reverts, avoiding any global variables.
>
You cannot do that when you don't have 'locale_t', hence the
complications. Or is the existence of locale_t somehow guaranteed by
gnulib? If it is, then that's solved.
Just wrap it in a typedef so we don't directly expose it in headers:
#if HAVE_LOCALE_T
typedef virLocale locale_t
#else
typedef virLocale void
#endif
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
6:51 a.m.
On Fri, Jun 23, 2017 at 10:45:30AM +0100, Daniel P. Berrange wrote:
On Fri, Jun 23, 2017 at 11:42:01AM +0200, Martin Kletzander wrote:
> On Fri, Jun 23, 2017 at 09:32:07AM +0100, Daniel P. Berrange wrote:
> > On Fri, Jun 23, 2017 at 10:21:32AM +0200, Martin Kletzander wrote:
> > > On Thu, Jun 22, 2017 at 03:49:03PM +0200, Peter Krempa wrote:
> > > > On Thu, Jun 22, 2017 at 14:36:53 +0200, Martin Kletzander wrote:
> > > > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> > > > > ---
> > > > > src/util/virstring.c | 96
++++++++++++++++++++++++++++++++--------------------
> > > > > 1 file changed, 60 insertions(+), 36 deletions(-)
> > > > >
> > > > > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > > > > index feea5be05198..6125725364f3 100644
> > > > > --- a/src/util/virstring.c
> > > > > +++ b/src/util/virstring.c
> > > > > @@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s, char
**end_ptr, int base,
> > > > > #if HAVE_NEWLOCALE
> > > > >
> > > > > static locale_t virLocale;
> > > > > +static locale_t virLocaleOld;
> > > >
> > > > This is not a thread local variable ...
> > > >
> > >
> > > Oh. shoot, you're right. So we need to wrap it in our struct that
will
> > > be defined conditionally as well, or allocate it on the heap, copy it
> > > there and set a pointer to it as one additional parameter or make thread
> > > local variable, but that one will need to be a pointer to the locale, so
> > > we'll need to allocate it on the heap as well. I don't like the
way
> > > this is going. Anyone else feel free to continue on this if you want to
> > > clean this up, but I like the few small conditional blocks better,
> > > especially since we have them in only two functions.
> >
> > Just have virLocaleSet() return the original locale to the caller, and
> > it can pass it back in when it reverts, avoiding any global variables.
> >
>
> You cannot do that when you don't have 'locale_t', hence the
> complications. Or is the existence of locale_t somehow guaranteed by
> gnulib? If it is, then that's solved.
Just wrap it in a typedef so we don't directly expose it in headers:
#if HAVE_LOCALE_T
typedef virLocale locale_t
#else
typedef virLocale void
#endif
That's what I was talking about. We need this, but also a possibility
to return error, so this needs to be an argument... And it's way more
code that is there right now. I'm not saying NO to the change, I just
that I like it like it's now (until more things need this treatment).
So for others, feel free to propose changes as you like :)
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
6:56 a.m.
On Fri, Jun 23, 2017 at 01:51:41PM +0200, Martin Kletzander wrote:
On Fri, Jun 23, 2017 at 10:45:30AM +0100, Daniel P. Berrange wrote:
> On Fri, Jun 23, 2017 at 11:42:01AM +0200, Martin Kletzander wrote:
> > On Fri, Jun 23, 2017 at 09:32:07AM +0100, Daniel P. Berrange wrote:
> > > On Fri, Jun 23, 2017 at 10:21:32AM +0200, Martin Kletzander wrote:
> > > > On Thu, Jun 22, 2017 at 03:49:03PM +0200, Peter Krempa wrote:
> > > > > On Thu, Jun 22, 2017 at 14:36:53 +0200, Martin Kletzander
wrote:
> > > > > > Signed-off-by: Martin Kletzander
<mkletzan(a)redhat.com>
> > > > > > ---
> > > > > > src/util/virstring.c | 96
++++++++++++++++++++++++++++++++--------------------
> > > > > > 1 file changed, 60 insertions(+), 36 deletions(-)
> > > > > >
> > > > > > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > > > > > index feea5be05198..6125725364f3 100644
> > > > > > --- a/src/util/virstring.c
> > > > > > +++ b/src/util/virstring.c
> > > > > > @@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s, char
**end_ptr, int base,
> > > > > > #if HAVE_NEWLOCALE
> > > > > >
> > > > > > static locale_t virLocale;
> > > > > > +static locale_t virLocaleOld;
> > > > >
> > > > > This is not a thread local variable ...
> > > > >
> > > >
> > > > Oh. shoot, you're right. So we need to wrap it in our struct
that will
> > > > be defined conditionally as well, or allocate it on the heap, copy
it
> > > > there and set a pointer to it as one additional parameter or make
thread
> > > > local variable, but that one will need to be a pointer to the locale,
so
> > > > we'll need to allocate it on the heap as well. I don't like
the way
> > > > this is going. Anyone else feel free to continue on this if you want
to
> > > > clean this up, but I like the few small conditional blocks better,
> > > > especially since we have them in only two functions.
> > >
> > > Just have virLocaleSet() return the original locale to the caller, and
> > > it can pass it back in when it reverts, avoiding any global variables.
> > >
> >
> > You cannot do that when you don't have 'locale_t', hence the
> > complications. Or is the existence of locale_t somehow guaranteed by
> > gnulib? If it is, then that's solved.
>
> Just wrap it in a typedef so we don't directly expose it in headers:
>
> #if HAVE_LOCALE_T
> typedef virLocale locale_t
> #else
> typedef virLocale void
> #endif
>
That's what I was talking about. We need this, but also a possibility
to return error, so this needs to be an argument... And it's way more
code that is there right now. I'm not saying NO to the change, I just
that I like it like it's now (until more things need this treatment).
So for others, feel free to propose changes as you like :)
But the current code you've proposed isn't thread safe, so it has to
change in some way surely ?
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
7:04 a.m.
On Fri, Jun 23, 2017 at 12:56:10PM +0100, Daniel P. Berrange wrote:
On Fri, Jun 23, 2017 at 01:51:41PM +0200, Martin Kletzander wrote:
> On Fri, Jun 23, 2017 at 10:45:30AM +0100, Daniel P. Berrange wrote:
> > On Fri, Jun 23, 2017 at 11:42:01AM +0200, Martin Kletzander wrote:
> > > On Fri, Jun 23, 2017 at 09:32:07AM +0100, Daniel P. Berrange wrote:
> > > > On Fri, Jun 23, 2017 at 10:21:32AM +0200, Martin Kletzander wrote:
> > > > > On Thu, Jun 22, 2017 at 03:49:03PM +0200, Peter Krempa wrote:
> > > > > > On Thu, Jun 22, 2017 at 14:36:53 +0200, Martin Kletzander
wrote:
> > > > > > > Signed-off-by: Martin Kletzander
<mkletzan(a)redhat.com>
> > > > > > > ---
> > > > > > > src/util/virstring.c | 96
++++++++++++++++++++++++++++++++--------------------
> > > > > > > 1 file changed, 60 insertions(+), 36 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/util/virstring.c
b/src/util/virstring.c
> > > > > > > index feea5be05198..6125725364f3 100644
> > > > > > > --- a/src/util/virstring.c
> > > > > > > +++ b/src/util/virstring.c
> > > > > > > @@ -522,6 +522,7 @@ virStrToLong_ullp(char const *s,
char **end_ptr, int base,
> > > > > > > #if HAVE_NEWLOCALE
> > > > > > >
> > > > > > > static locale_t virLocale;
> > > > > > > +static locale_t virLocaleOld;
> > > > > >
> > > > > > This is not a thread local variable ...
> > > > > >
> > > > >
> > > > > Oh. shoot, you're right. So we need to wrap it in our struct
that will
> > > > > be defined conditionally as well, or allocate it on the heap,
copy it
> > > > > there and set a pointer to it as one additional parameter or make
thread
> > > > > local variable, but that one will need to be a pointer to the
locale, so
> > > > > we'll need to allocate it on the heap as well. I don't
like the way
> > > > > this is going. Anyone else feel free to continue on this if you
want to
> > > > > clean this up, but I like the few small conditional blocks
better,
> > > > > especially since we have them in only two functions.
> > > >
> > > > Just have virLocaleSet() return the original locale to the caller,
and
> > > > it can pass it back in when it reverts, avoiding any global
variables.
> > > >
> > >
> > > You cannot do that when you don't have 'locale_t', hence the
> > > complications. Or is the existence of locale_t somehow guaranteed by
> > > gnulib? If it is, then that's solved.
> >
> > Just wrap it in a typedef so we don't directly expose it in headers:
> >
> > #if HAVE_LOCALE_T
> > typedef virLocale locale_t
> > #else
> > typedef virLocale void
> > #endif
> >
>
> That's what I was talking about. We need this, but also a possibility
> to return error, so this needs to be an argument... And it's way more
> code that is there right now. I'm not saying NO to the change, I just
> that I like it like it's now (until more things need this treatment).
> So for others, feel free to propose changes as you like :)
But the current code you've proposed isn't thread safe, so it has to
change in some way surely ?
No, this proposed patch isn't thread-safe. Current master is OK, this
is only about "prettifying" the code.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
2712
days inactive
2713
days old
9 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Martin Kletzander -
Michal Privoznik -
Peter Krempa