You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Branko Čibej <br...@xbc.nu> on 2001/06/28 05:06:00 UTC
apr_implode_time and time zones
Right now, the Unix version of apr_implode_time completely ignores the
tm_gmtoff field and does not adjust for the time zone.. This means that
imploding an apr_time_t with a non-zero tm_gmtoffset will *not* yield a
correct time value.
My first reaction upon noticing this was to simply go and fix it, but
then I noticed that tests/testtime.c expects this behaviour.
Is this the expected behaviour, or is it a bug? When would you ever want
to ignore the time zone, but wouldn't want to explicitly set the
tm_gmtoff field to 0?
--
Brane Čibej
home: <br...@xbc.nu> http://www.xbc.nu/brane/
work: <br...@hermes.si> http://www.hermes-softlab.com/
ACM : <br...@acm.org> http://www.acm.org/
Re: apr_implode_time and time zones
Posted by Branko Čibej <br...@xbc.nu>.
David Reid wrote:
>AFAICT there's no way to
>figure out how to set the gmt offset on Solaris when passing in a value,
>
After two minutes of manpage reading and one minute of coding:
#include <time.h>
#include <stdio.h>
int main (void)
{
time_t t1, t2;
struct tm tm;
t1 = time(0);
tm = *gmtime(&t1);
t2 = mktime(&tm);
printf("GMT offset = %ld\n", (long) difftime(t1, t2));
return 0;
}
All of this code uses only standard ANSI time functions. And the results:
$ cc -o gmtoff gmtoff.c
$ TZ=PST8PDT ./gmtoff
GMT offset = -28800
$ TZ=CET-1DST ./gmtoff
GMT offset = 3600
$ TZ=GMT ./gmtoff
GMT offset = 0
That's on sparc-sun-solaris2.6, hppa1.1-hp-hpux10.20, and
i686-pc-linux-gnu. I fail to see when this wouldn't work. Would this be
a satisfactory solution on systems that don't have a tm_gmtoff field?
--
Brane Čibej
home: <br...@xbc.nu> http://www.xbc.nu/brane/
work: <br...@hermes.si> http://www.hermes-softlab.com/
ACM : <br...@acm.org> http://www.acm.org/
Re: apr_implode_time and time zones
Posted by Branko Čibej <br...@xbc.nu>.
David Reid wrote:
>What's with the HTML email????
>
Nothing. Check your mail client.
Content-Type: text/plain; charset=ISO-8859-2; format=flowed
>>Hmm, good point. But that's easily fixed, I think.
>>
>
>Why should it be fixed???
>
I think we're having a bit of a misunderstanding here. My fault; I
should have got some sleep before posting.
So, let me try again.
First, some history: Subversion uses the APR time functions to record
the time stamps of revision-controlled files in the working copy. It
does this by statting the file, using apr_explode_localtime and writing
a text representation of the result to disk (including the tm_gmtoff
field). Later on it uses that timestamp to check if the file has
changed: it reads the timestamp back in, scans it into an
apr_exploded_time_t and uses apr_implode_time to get an apr_time_t,
which it compares to the one coming from apr_stat. Obviously, this
didn't work, because the apr_time_t we got from apr_implode_time wasn't UTC.
So I looked at apr_implode_time, and thought, "Aha, that'a a bug, we're
ignoring tm_gmtoff." Then I was told, by Ryan and you, that this was
intentional. I was confused by two things: first, I understood that the
reason for this was that we can't calculate the GMT offset on Solaris;
and second, that having an apr_time_t that's /not/ UTC is valid:
>>Actually, what was happening is something like this:
>>
>> apr_time_t old, new;
>> apr_exploded_time_t xt;
>> apr_time_now(&old);
>> apr_explode_localtime(&xt, old);
>> apr_implode_time(&new, &xt);
>> if (old != new)
>> it's a bug!
>>
>
>Yeah, but that's what we expect! It's not a bug! If you call
>apr_time_now() you get GMT! That's the basic problem in your logic - apr
>will normally work in GMT as that's a sensible (the only really sensible)
>starting point. What you're saying is
>
>- get GMT time as old
>- get localtime based on GMT
>- if gmt != localtime - it's a bug!
>
>Unless you live in gmt_offset = 0 then it'll always be different! In fact
>most places in the gmt_offset use DST and so it'll be different for half the
>year anyway!
>
What I expected was that the apr_time_t coming from apr_implode_time
would be UTC, just like the one coming from apr_time_now. That's why I
talked about fixing the problem; after all, just making sure that the
tm_gmtoff field is always correct after an explode, and accounting for
it in an implode, would be enough.
>This was what we decided at Santa Clara was the way we'd do the time zones.
>
>>>When I raised the question there were probably 50% of the active APR folks
>>>in the room :)
>>>
>>Hm. Good thing I wasn't there, then. :-)
>>
>
>Why? You don't like participating in discussions? :)
>
Not at all, but I'd have opposed the decision most loudly. :-)
>>I think what this boils down to is that the explode/implode
>>implementation is wrong. The result of an implode should always be the
>>same as the original time before the explode, regardless of what we told
>>explode to use as the offset.
>>
>
>Nope. Sorry but in that case we wouldn't need 3 different implementations
>would we? apr_time_now gives back GMT. apr_explode_localtime does just
>that - gives localtime. If you want GMT use apr_explode_gmt (as testtime
>does) and it'll work as you expect.
>
It's not apr_explode_* I'm ranting about, it's apr_implode_time not
giving me an apr_time_t that's UTC, even though it could. I understand
now that this is intentional (although I still don't understand why).
I would now like to formally suggest that we consider changing the
behaviour of apr_implode_time. It seems to be the only function in APR
that can produce an apr_time_t that's not UTC, which is confusing. If
that can't be done, i.e., if there is a platform where we truly can't
compute tm_gmtoff correctly in the apr_explode_* functions, then we
should document that.
Brane
--
Brane Čibej
home: <br...@xbc.nu> http://www.xbc.nu/brane/
work: <br...@hermes.si> http://www.hermes-softlab.com/
ACM: <br...@acm.org> http://www.acm.org/
Re: apr_implode_time and time zones
Posted by David Reid <dr...@jetnet.co.uk>.
What's with the HTML email????
> David Reid wrote:
>
> >OK, basically apr_implode_time ONLY works with an apr_exploded_time_t.
We
> >have a number of ways of getting one of those,
> >
> >/**
> > * convert a time to its human readable components using an offset
> > * from GMT
> > * @param result the exploded time
> > * @param input the time to explode
> > * @param offs the number of seconds offset to apply
> >* @deffunc apr_status_t apr_explode_time(apr_exploded_time_t *result,
> >apr_time_t input, apr_int32_t offs)
> > */
> >APR_DECLARE(apr_status_t) apr_explode_time(apr_exploded_time_t *result,
> > apr_time_t input,
> > apr_int32_t offs);
> >
> >/**
> > * convert a time to its human readable components in GMT timezone
> > * @param result the exploded time
> > * @param input the time to explode
> > * @deffunc apr_status_t apr_explode_gmt(apr_exploded_time_t *result,
> >apr_time_t input)
> > */
> >APR_DECLARE(apr_status_t) apr_explode_gmt(apr_exploded_time_t *result,
> > apr_time_t input);
> >
> >/**
> > * convert a time to its human readable components in local timezone
> > * @param result the exploded time
> > * @param input the time to explode
> > * @deffunc apr_status_t apr_explode_localtime(apr_exploded_time_t
*result,
> >apr_time_t input)
> > */
> >APR_DECLARE(apr_status_t) apr_explode_localtime(apr_exploded_time_t
*result,
> > apr_time_t input);
> >
> >
> >Basically, the first will always give GMT/UTC (0 offset), the next will
give
> >localtime (offset set accordingly) and the final gives GMT/UTC +/- the
> >offset you pass in.
> >
> (I think you mixd up first/last here, but so far so good).
yeah, I was going to write them all and ended up pasting in some stuff from
apr_time.h, sorry :) Glad it didn't confuse you :)
>
> > The time recorded in the various parts of the structure
> >is that of the time with the offset applied - hence applying it again is
> >evil and wrong!
> >
>
> Hmm, good point. But that's easily fixed, I think.
Why should it be fixed???
>
> >The part that was probably giving you problems, although you don't
describe
> >what problems you were seeing, was the code that takes an ostime
structure
> >and morph's it into an apr_exploded_time_t, which ignores the gmt_offset
at
> >present.
> >
> Actually, what was happening is something like this:
>
> apr_time_t old, new;
> apr_exploded_time_t xt;
> apr_time_now(&old);
> apr_explode_localtime(&xt, old);
> apr_implode_time(&new, &xt);
> if (old != new)
> it's a bug!
Yeah, but that's what we expect! It's not a bug! If you call
apr_time_now() you get GMT! That's the basic problem in your logic - apr
will normally work in GMT as that's a sensible (the only really sensible)
starting point. What you're saying is
- get GMT time as old
- get localtime based on GMT
- if gmt != localtime - it's a bug!
Unless you live in gmt_offset = 0 then it'll always be different! In fact
most places in the gmt_offset use DST and so it'll be different for half the
year anyway!
> > However, I have a fix for that and some code cleanup on the way as
> >soon as I get a connection I can commit via (in Bahrain for another 4
hours
> >or so before going home so not long to wait). AFAICT there's no way to
> >figure out how to set the gmt offset on Solaris when passing in a value,
but
> >if you want localtime simply use apr_explode_localtime and you'll get it
> >that way.
> >
> But set_xt_gmtoff_from_tm already has a workaround for the solaris
problem.
>
> >This was what we decided at Santa Clara was the way we'd do the time
zones.
> >When I raised the question there were probably 50% of the active APR
folks
> >in the room :)
> >
>
> Hm. Good thing I wasn't there, then. :-)
Why? You don't like participating in discussions? :) It's more use face to
face 99% of the time.
>
> I think what this boils down to is that the explode/implode
> implementation is wrong. The result of an implode should always be the
> same as the original time before the explode, regardless of what we told
> explode to use as the offset.
Nope. Sorry but in that case we wouldn't need 3 different implementations
would we? apr_time_now gives back GMT. apr_explode_localtime does just
that - gives localtime. If you want GMT use apr_explode_gmt (as testtime
does) and it'll work as you expect.
>
> AFAIK the Win32 implementation does this correctly. Now, we wouldn't
> want APR on Win32 to be more correct than APR on Unix, would we? :-)
If that's the case then Win32 is broken, so once again Unix rules :) I'm
surprised though as Bill Rowe was the main collaborator in these changes!
>
> I'll look into fixing this.
>
Don't think it's broken, but the documentation probably needs to be improved
on.
david
Re: apr_implode_time and time zones
Posted by Branko Čibej <br...@xbc.nu>.
David Reid wrote:
>OK, basically apr_implode_time ONLY works with an apr_exploded_time_t. We
>have a number of ways of getting one of those,
>
>/**
> * convert a time to its human readable components using an offset
> * from GMT
> * @param result the exploded time
> * @param input the time to explode
> * @param offs the number of seconds offset to apply
>* @deffunc apr_status_t apr_explode_time(apr_exploded_time_t *result,
>apr_time_t input, apr_int32_t offs)
> */
>APR_DECLARE(apr_status_t) apr_explode_time(apr_exploded_time_t *result,
> apr_time_t input,
> apr_int32_t offs);
>
>/**
> * convert a time to its human readable components in GMT timezone
> * @param result the exploded time
> * @param input the time to explode
> * @deffunc apr_status_t apr_explode_gmt(apr_exploded_time_t *result,
>apr_time_t input)
> */
>APR_DECLARE(apr_status_t) apr_explode_gmt(apr_exploded_time_t *result,
> apr_time_t input);
>
>/**
> * convert a time to its human readable components in local timezone
> * @param result the exploded time
> * @param input the time to explode
> * @deffunc apr_status_t apr_explode_localtime(apr_exploded_time_t *result,
>apr_time_t input)
> */
>APR_DECLARE(apr_status_t) apr_explode_localtime(apr_exploded_time_t *result,
> apr_time_t input);
>
>
>Basically, the first will always give GMT/UTC (0 offset), the next will give
>localtime (offset set accordingly) and the final gives GMT/UTC +/- the
>offset you pass in.
>
(I think you mixd up first/last here, but so far so good).
> The time recorded in the various parts of the structure
>is that of the time with the offset applied - hence applying it again is
>evil and wrong!
>
Hmm, good point. But that's easily fixed, I think.
>The part that was probably giving you problems, although you don't describe
>what problems you were seeing, was the code that takes an ostime structure
>and morph's it into an apr_exploded_time_t, which ignores the gmt_offset at
>present.
>
Actually, what was happening is something like this:
apr_time_t old, new;
apr_exploded_time_t xt;
apr_time_now(&old);
apr_explode_localtime(&xt, old);
apr_implode_time(&new, &xt);
if (old != new)
it's a bug!
> However, I have a fix for that and some code cleanup on the way as
>soon as I get a connection I can commit via (in Bahrain for another 4 hours
>or so before going home so not long to wait). AFAICT there's no way to
>figure out how to set the gmt offset on Solaris when passing in a value, but
>if you want localtime simply use apr_explode_localtime and you'll get it
>that way.
>
But set_xt_gmtoff_from_tm already has a workaround for the solaris problem.
>This was what we decided at Santa Clara was the way we'd do the time zones.
>When I raised the question there were probably 50% of the active APR folks
>in the room :)
>
Hm. Good thing I wasn't there, then. :-)
I think what this boils down to is that the explode/implode
implementation is wrong. The result of an implode should always be the
same as the original time before the explode, regardless of what we told
explode to use as the offset.
AFAIK the Win32 implementation does this correctly. Now, we wouldn't
want APR on Win32 to be more correct than APR on Unix, would we? :-)
I'll look into fixing this.
--
Brane Čibej
home: <br...@xbc.nu> http://www.xbc.nu/brane/
work: <br...@hermes.si> http://www.hermes-softlab.com/
ACM: <br...@acm.org> http://www.acm.org/
Re: apr_implode_time and time zones
Posted by David Reid <dr...@jetnet.co.uk>.
I've reviewed the code and can't quite see what you mean...
> >I believe this is a bug. However, we have never been able to reliably
fix
> >it. You should check the APR list archives, because David Reid and I
> >discussed this a while back. It was probably around March of this year.
Read on...
> >
> Hmm. Couldn't find anything there ...
>
> >You could also check CVS for the logs for the time code on Unix. That
> >would give you a better idea of when we had the discussion.
> >
> All I saw was something about tm_gmtoff not being present on Solaris.
> But there's already a workaround for that, isn't there?
>
> I fail to see how apr_implode_time() could /ever/ be wrong if it uses
> tm_gmtoff. Whether that field is set correctly or not is another matter,
> and most probably shows up only on very few platforms. Right now,
> apr_implode_time() is /always/ wrong. Surely it sould be a lesser evil
> to just fix it?
OK, basically apr_implode_time ONLY works with an apr_exploded_time_t. We
have a number of ways of getting one of those,
/**
* convert a time to its human readable components using an offset
* from GMT
* @param result the exploded time
* @param input the time to explode
* @param offs the number of seconds offset to apply
* @deffunc apr_status_t apr_explode_time(apr_exploded_time_t *result,
apr_time_t input, apr_int32_t offs)
*/
APR_DECLARE(apr_status_t) apr_explode_time(apr_exploded_time_t *result,
apr_time_t input,
apr_int32_t offs);
/**
* convert a time to its human readable components in GMT timezone
* @param result the exploded time
* @param input the time to explode
* @deffunc apr_status_t apr_explode_gmt(apr_exploded_time_t *result,
apr_time_t input)
*/
APR_DECLARE(apr_status_t) apr_explode_gmt(apr_exploded_time_t *result,
apr_time_t input);
/**
* convert a time to its human readable components in local timezone
* @param result the exploded time
* @param input the time to explode
* @deffunc apr_status_t apr_explode_localtime(apr_exploded_time_t *result,
apr_time_t input)
*/
APR_DECLARE(apr_status_t) apr_explode_localtime(apr_exploded_time_t *result,
apr_time_t input);
Basically, the first will always give GMT/UTC (0 offset), the next will give
localtime (offset set accordingly) and the final gives GMT/UTC +/- the
offset you pass in. The time recorded in the various parts of the structure
is that of the time with the offset applied - hence applying it again is
evil and wrong!
The part that was probably giving you problems, although you don't describe
what problems you were seeing, was the code that takes an ostime structure
and morph's it into an apr_exploded_time_t, which ignores the gmt_offset at
present. However, I have a fix for that and some code cleanup on the way as
soon as I get a connection I can commit via (in Bahrain for another 4 hours
or so before going home so not long to wait). AFAICT there's no way to
figure out how to set the gmt offset on Solaris when passing in a value, but
if you want localtime simply use apr_explode_localtime and you'll get it
that way.
This was what we decided at Santa Clara was the way we'd do the time zones.
When I raised the question there were probably 50% of the active APR folks
in the room :)
david
Re: apr_implode_time and time zones
Posted by David Reid <dr...@jetnet.co.uk>.
Ryan and I did discuss this but I think it was face to face at Santa Clara
when I made some big changes to that code to get it working :) I'll have a
look at the code and see what's going on, but we should be using the offset
if there is one, or at least so I believe without reviewing the code.
david
> rbb@covalent.net wrote:
>
> >I believe this is a bug. However, we have never been able to reliably
fix
> >it. You should check the APR list archives, because David Reid and I
> >discussed this a while back. It was probably around March of this year.
> >
> Hmm. Couldn't find anything there ...
>
> >You could also check CVS for the logs for the time code on Unix. That
> >would give you a better idea of when we had the discussion.
> >
> All I saw was something about tm_gmtoff not being present on Solaris.
> But there's already a workaround for that, isn't there?
>
> I fail to see how apr_implode_time() could /ever/ be wrong if it uses
> tm_gmtoff. Whether that field is set correctly or not is another matter,
> and most probably shows up only on very few platforms. Right now,
> apr_implode_time() is /always/ wrong. Surely it sould be a lesser evil
> to just fix it?
Re: apr_implode_time and time zones
Posted by Branko Čibej <br...@xbc.nu>.
rbb@covalent.net wrote:
>I believe this is a bug. However, we have never been able to reliably fix
>it. You should check the APR list archives, because David Reid and I
>discussed this a while back. It was probably around March of this year.
>
Hmm. Couldn't find anything there ...
>You could also check CVS for the logs for the time code on Unix. That
>would give you a better idea of when we had the discussion.
>
All I saw was something about tm_gmtoff not being present on Solaris.
But there's already a workaround for that, isn't there?
I fail to see how apr_implode_time() could /ever/ be wrong if it uses
tm_gmtoff. Whether that field is set correctly or not is another matter,
and most probably shows up only on very few platforms. Right now,
apr_implode_time() is /always/ wrong. Surely it sould be a lesser evil
to just fix it?
--
Brane Čibej
home: <br...@xbc.nu> http://www.xbc.nu/brane/
work: <br...@hermes.si> http://www.hermes-softlab.com/
ACM : <br...@acm.org> http://www.acm.org/
Re: apr_implode_time and time zones
Posted by rb...@covalent.net.
I believe this is a bug. However, we have never been able to reliably fix
it. You should check the APR list archives, because David Reid and I
discussed this a while back. It was probably around March of this year.
You could also check CVS for the logs for the time code on Unix. That
would give you a better idea of when we had the discussion.
Ryan
On Thu, 28 Jun 2001, Branko [ISO-8859-2] �ibej wrote:
> Right now, the Unix version of apr_implode_time completely ignores the
> tm_gmtoff field and does not adjust for the time zone.. This means that
> imploding an apr_time_t with a non-zero tm_gmtoffset will *not* yield a
> correct time value.
>
> My first reaction upon noticing this was to simply go and fix it, but
> then I noticed that tests/testtime.c expects this behaviour.
>
> Is this the expected behaviour, or is it a bug? When would you ever want
> to ignore the time zone, but wouldn't want to explicitly set the
> tm_gmtoff field to 0?
>
>
> --
> Brane �ibej
> home: <br...@xbc.nu> http://www.xbc.nu/brane/
> work: <br...@hermes.si> http://www.hermes-softlab.com/
> ACM : <br...@acm.org> http://www.acm.org/
>
>
>
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------