* [PATCH] tzset did not catch changes to localtime [BZ #21060 ]
@ 2017-11-02 9:03 ma.jiang
2017-11-02 15:33 ` Carlos O'Donell
2017-11-02 23:48 ` J William Piggott
0 siblings, 2 replies; 6+ messages in thread
From: ma.jiang @ 2017-11-02 9:03 UTC (permalink / raw)
To: libc-alpha
[-- Attachment #1.1: Type: text/plain, Size: 1235 bytes --]
Hi,
If users modified the local timezone file (usually /etc/localtime) while keep the TZ environment variable untouched, tzset will not recalculate tzname. This is not right. Patch attached should fix this problem, is it OK for trunk?
diff -Nuarp a/time/tzset.c b/time/tzset.c
--- a/time/tzset.c 2017-10-31 17:28:58.105026060 +0800
+++ b/time/tzset.c 2017-10-31 17:28:43.470025216 +0800
@@ -387,21 +387,18 @@ tzset_internal (int always)
if (tz && *tz == ':')
++tz;
- /* Check whether the value changed since the last run. */
- if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
- /* No change, simply return. */
- return;
-
if (tz == NULL)
/* No user specification; use the site-wide default. */
tz = TZDEFAULT;
- tz_rules[0].name = NULL;
- tz_rules[1].name = NULL;
-
- /* Save the value of `tz'. */
- free (old_tz);
- old_tz = tz ? __strdup (tz) : NULL;
+ /* Check whether the value changed since the last run. */
+ if ((old_tz == NULL)
+ || (old_tz != NULL && strcmp (tz, old_tz) != 0))
+ {
+ /* Save the value of `tz'. */
+ free (old_tz);
+ old_tz = tz ? __strdup (tz) : NULL;
+ }
/* Try to read a data file. */
__tzfile_read (tz, 0, NULL);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tzset did not catch changes to localtime [BZ #21060 ]
2017-11-02 9:03 [PATCH] tzset did not catch changes to localtime [BZ #21060 ] ma.jiang
@ 2017-11-02 15:33 ` Carlos O'Donell
2017-11-07 10:27 ` ma.jiang
2017-11-02 23:48 ` J William Piggott
1 sibling, 1 reply; 6+ messages in thread
From: Carlos O'Donell @ 2017-11-02 15:33 UTC (permalink / raw)
To: ma.jiang, libc-alpha
On 11/02/2017 02:03 AM, ma.jiang@zte.com.cn wrote:
> Hi, If users modified the local timezone file (usually
> /etc/localtime) while keep the TZ environment variable untouched,
> tzset will not recalculate tzname. This is not right. Patch attached
> should fix this problem, is it OK for trunk?
Thank you for the patch! The idea you present is interesting, but it
makes tzset() expensive when run in a loop, and several interfaces
call tzset() unconditionally to update the timezone in the event the
API call is the first in the sequence. For example mktime must call
tzset, and previously this might have been fast, but now it re-reads
the file every time. I don't think this is an acceptable solution
to the problem, but without a microbenchmark we don't know. So one
way to make your point would be to contribute a microbenchmark for
tzset or mktime, and show that the performance difference is
negligible. The other alternative is to go a layer down and stat
/etc/localtime (if that file is going to be used), and therefore
provide a quick way to avoid reloading the file if it hasn't changed.
All of this also will need a test case, which may or may not be
possible e.g. test starts in one timezone, updates the file used
(via TZ to full path of timezone), and then calls tzset, and verifies
that timezone has changed.
Lastly, this patch is a non-trivial number of lines and the copyright
status of ZTE is not yet clear. I will continue the copyright
discussion off-list with you directly.
Thank you again for your contribution.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tzset did not catch changes to localtime [BZ #21060 ]
2017-11-02 9:03 [PATCH] tzset did not catch changes to localtime [BZ #21060 ] ma.jiang
2017-11-02 15:33 ` Carlos O'Donell
@ 2017-11-02 23:48 ` J William Piggott
2017-11-03 8:16 ` 答复: " ma.jiang
1 sibling, 1 reply; 6+ messages in thread
From: J William Piggott @ 2017-11-02 23:48 UTC (permalink / raw)
To: ma.jiang, libc-alpha
On 11/02/2017 05:03 AM, ma.jiang@zte.com.cn wrote:
> Hi,
> If users modified the local timezone file (usually /etc/localtime)
> while keep the TZ environment variable untouched, tzset will not
> recalculate tzname. This is not right.
Yes it is. The TZ env overrides the system timezone. If TZ exists then
tzset should ignore any system wide setting whether something modifies
it or not. Users should not have permission to modify anything in /etc/
anyway.
> Patch attached should fix this
> problem, is it OK for trunk?
>
>
> diff -Nuarp a/time/tzset.c b/time/tzset.c
> --- a/time/tzset.c 2017-10-31 17:28:58.105026060 +0800
> +++ b/time/tzset.c 2017-10-31 17:28:43.470025216 +0800
> @@ -387,21 +387,18 @@ tzset_internal (int always)
> if (tz && *tz == ':')
> ++tz;
>
> - /* Check whether the value changed since the last run. */
> - if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
> - /* No change, simply return. */
> - return;
> -
> if (tz == NULL)
> /* No user specification; use the site-wide default. */
> tz = TZDEFAULT;
>
> - tz_rules[0].name = NULL;
> - tz_rules[1].name = NULL;
> -
> - /* Save the value of `tz'. */
> - free (old_tz);
> - old_tz = tz ? __strdup (tz) : NULL;
> + /* Check whether the value changed since the last run. */
> + if ((old_tz == NULL)
> + || (old_tz != NULL && strcmp (tz, old_tz) != 0))
> + {
> + /* Save the value of `tz'. */
> + free (old_tz);
> + old_tz = tz ? __strdup (tz) : NULL;
> + }
>
> /* Try to read a data file. */
> __tzfile_read (tz, 0, NULL);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* 答复: Re: [PATCH] tzset did not catch changes to localtime [BZ #21060 ]
2017-11-02 23:48 ` J William Piggott
@ 2017-11-03 8:16 ` ma.jiang
0 siblings, 0 replies; 6+ messages in thread
From: ma.jiang @ 2017-11-03 8:16 UTC (permalink / raw)
To: elseifthen; +Cc: libc-alpha
[-- Attachment #1.1: Type: text/plain, Size: 2230 bytes --]
> Yes it is. The TZ env overrides the system timezone. If TZ exists then
> tzset should ignore any system wide setting whether something modifies
> it or not.
Please , think about this situation.
Set Tz=":/mydir/mytime“
Start my application Xapp
Modify /mydir/mytime
use tzset in Xapp
What will I get from tzset? I do not think it could be something a user want...
------------------原始邮件------------------
发件人： <elseifthen@gmx.com>;
收件人：马江10100629; <libc-alpha@sourceware.org>;
日 期 ：2017年11月03日 07:47
主 题 ：Re: [PATCH] tzset did not catch changes to localtime [BZ #21060 ]
On 11/02/2017 05:03 AM, ma.jiang@zte.com.cn wrote:
> Hi,
> If users modified the local timezone file (usually /etc/localtime)
> while keep the TZ environment variable untouched, tzset will not
> recalculate tzname. This is not right.
Yes it is. The TZ env overrides the system timezone. If TZ exists then
tzset should ignore any system wide setting whether something modifies
it or not. Users should not have permission to modify anything in /etc/
anyway.
> Patch attached should fix this
> problem, is it OK for trunk?
>
>
> diff -Nuarp a/time/tzset.c b/time/tzset.c
> --- a/time/tzset.c 2017-10-31 17:28:58.105026060 +0800
> +++ b/time/tzset.c 2017-10-31 17:28:43.470025216 +0800
> @@ -387,21 +387,18 @@ tzset_internal (int always)
> if (tz && *tz == ':')
> ++tz;
>
> - /* Check whether the value changed since the last run. */
> - if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
> - /* No change, simply return. */
> - return;
> -
> if (tz == NULL)
> /* No user specification; use the site-wide default. */
> tz = TZDEFAULT;
>
> - tz_rules[0].name = NULL;
> - tz_rules[1].name = NULL;
> -
> - /* Save the value of `tz'. */
> - free (old_tz);
> - old_tz = tz ? __strdup (tz) : NULL;
> + /* Check whether the value changed since the last run. */
> + if ((old_tz == NULL)
> + || (old_tz != NULL && strcmp (tz, old_tz) != 0))
> + {
> + /* Save the value of `tz'. */
> + free (old_tz);
> + old_tz = tz ? __strdup (tz) : NULL;
> + }
>
> /* Try to read a data file. */
> __tzfile_read (tz, 0, NULL);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] tzset did not catch changes to localtime [BZ #21060 ]
2017-11-02 15:33 ` Carlos O'Donell
@ 2017-11-07 10:27 ` ma.jiang
0 siblings, 0 replies; 6+ messages in thread
From: ma.jiang @ 2017-11-07 10:27 UTC (permalink / raw)
To: libc-alpha
[-- Attachment #1.1: Type: text/plain, Size: 1899 bytes --]
Thanks for your advice, I agree with you. I will reconsider the performance things.
------------------原始邮件------------------
发件人： <carlos@redhat.com>;
收件人：马江10100629; <libc-alpha@sourceware.org>;
日 期 ：2017年11月02日 23:33
主 题 ：Re: [PATCH] tzset did not catch changes to localtime [BZ #21060 ]
On 11/02/2017 02:03 AM, ma.jiang@zte.com.cn wrote:
> Hi, If users modified the local timezone file (usually
> /etc/localtime) while keep the TZ environment variable untouched,
> tzset will not recalculate tzname. This is not right. Patch attached
> should fix this problem, is it OK for trunk?
Thank you for the patch! The idea you present is interesting, but it
makes tzset() expensive when run in a loop, and several interfaces
call tzset() unconditionally to update the timezone in the event the
API call is the first in the sequence. For example mktime must call
tzset, and previously this might have been fast, but now it re-reads
the file every time. I don't think this is an acceptable solution
to the problem, but without a microbenchmark we don't know. So one
way to make your point would be to contribute a microbenchmark for
tzset or mktime, and show that the performance difference is
negligible. The other alternative is to go a layer down and stat
/etc/localtime (if that file is going to be used), and therefore
provide a quick way to avoid reloading the file if it hasn't changed.
All of this also will need a test case, which may or may not be
possible e.g. test starts in one timezone, updates the file used
(via TZ to full path of timezone), and then calls tzset, and verifies
that timezone has changed.
Lastly, this patch is a non-trivial number of lines and the copyright
status of ZTE is not yet clear. I will continue the copyright
discussion off-list with you directly.
Thank you again for your contribution.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tzset did not catch changes to localtime [BZ #21060 ]
2018-12-17 9:40 ma.jiang
@ 2018-12-17 9:40 ` Andreas Schwab
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Schwab @ 2018-12-17 9:40 UTC (permalink / raw)
To: ma.jiang; +Cc: carlos, libc-alpha
On Dez 17 2018, <ma.jiang@zte.com.cn> wrote:
> + /* Check whether the tz value changed since the last run. */
> + int tz_changed = old_tz ? strcmp (tz, old_tz) : 1;
This should be written as old_tz == NULL || strcmp (tz, old_tz) != 0,
and tz_changed should be bool.
> + if ((old_tz == NULL)
> + || (old_tz != NULL && tz_changed))
This is the same as tz_changed.
> + {
> + /* Save the value of `tz'. */
> + free (old_tz);
> + old_tz = __strdup (tz);
> + }
Wrong indentation.
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-17 9:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-02 9:03 [PATCH] tzset did not catch changes to localtime [BZ #21060 ] ma.jiang
2017-11-02 15:33 ` Carlos O'Donell
2017-11-07 10:27 ` ma.jiang
2017-11-02 23:48 ` J William Piggott
2017-11-03 8:16 ` 答复: " ma.jiang
2018-12-17 9:40 ma.jiang
2018-12-17 9:40 ` [PATCH] " Andreas Schwab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).