Re: [Gtkpod-devel] libgpod.get_local_timezone.awn.1.patch

[Andrew W. N. <and...@gm...>]

On Sat, Mar 21, 2009 at 3:28 PM, Christophe Fergeau <te...@gn...> wrote:
> Hi Andrew,
>
> Andrew W. Nosenko wrote:
>>
>> Attached patch avoids using of the global variable 'timezone' (from
>> the time.h) in favor of struct
>> tm.tm_gmtoff field (if available).
>>
>
> I finally committed your patch, sorry for the delay. I made a few changes to

Excuse me also... I wanted to send 2nd version of the patch, but
didn't made it yet because of workload....

> configure.ac to get my linux box to use localtime_r (#define _BSD_SOURCE was
> needed, and then I had to include time.h in the test for gmtoff). And I

Thanks.  On FreeBSD the time.h seems was included by dependence of
some other header...
But defining of the _BSD_SOURCE should be unneeded, at least on Linux
with Gnu Libc because defining of _GNU_SOURCE (sorry, don't remember
correct spelling, set by AC_GNU_SOURCE autoconf macro when applicable)
includes _BSD_SOURCE.

> added a call to tzset before calling localtime_r since the manpage
> recommends to do it. See attached patch for these changes.

Can you point me to this manpage?  Explicit calling of the tzset() is
unneeded as far as I know, because localtime() and localtime_r() both
check whether TZ data already initialized and initializes them if not.
 As far, as I understand, you need to call tzset() explicitly only if
you changed TZ environment variable to signal that "internal caches"
should be refilled.  But it is just my understanding and I may be
mistaken...

>
>> Why I preferred to check tm_gmtoff and fallback to the timezone
>> variable instead of checking both and use timezone variable at the
>> first and fallback to the tm_gmtoff only as second?
>
> The use of the timezone global var could probably be dropped anyway,
> localtime(_r) should be available everywhere libgpod compile nowadays
> (that's just a wild assumption, feel free to tell me if I'm wrong ;)

Not a problem :-)  It makes my task even simpler :-)

>>
>> Using of the localtime_r() in favor of localtime(), and protecting
>> non-threadsafe localtime() by G_LOCK() is made for thread safety.  It
>> is a library and, threfore, extra paranoia cannot harm.
>>
>
> I removed these locks, especially after your comments in another mail.

Yes, absolutely.

> Thanks for the nice patch!

Thank you for nice program!

>
> Christophe
>
> diff --git a/configure.ac b/configure.ac
> index e240249..5c56ac7 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -40,6 +40,7 @@ AC_SUBST(ACLOCAL_AMFLAGS, "$ACLOCAL_FLAGS")
>  AM_MAINTAINER_MODE
>
>  AC_GNU_SOURCE
> +AC_DEFINE([_BSD_SOURCE], 1, [BSD Functions])
>  AC_PROG_CC
>  AM_PROG_CC_C_O
>
> @@ -57,7 +58,7 @@ AC_PROG_MAKE_SET
>  AC_PROG_INTLTOOL([0.21])
>
>  AC_CHECK_FUNCS([localtime_r])
> -AC_CHECK_MEMBERS([struct tm.tm_gmtoff])
> +AC_CHECK_MEMBERS([struct tm.tm_gmtoff],,,[#include <time.h>])
>  PKG_CHECK_MODULES(LIBGPOD, glib-2.0 >= 2.8.0 gobject-2.0)
>
>  dnl **************************************************
> diff --git a/src/itdb_device.c b/src/itdb_device.c
> index f9d74c0..5bcbcbe 100644
> --- a/src/itdb_device.c
> +++ b/src/itdb_device.c
> @@ -1639,6 +1639,7 @@ static gint get_local_timezone (void)
>  #   ifdef HAVE_LOCALTIME_R
>     {
>         struct tm tmb;
> +        tzset ();
>         localtime_r(&t, &tmb);
>         seconds_east_utc = tmb.tm_gmtoff;
>     }
>
>

-- 
Andrew W. Nosenko <and...@gm...>