| Message ID | 1417573661-9902-1-git-send-email-james@loowit.net |
|---|---|
| State | New |
| Headers | show |
On 3 December 2014 at 02:27, James Perkins <james@loowit.net> wrote: > This is a fix for [BZ #16141] strptime %z offset restriction. > > strptime supports the parsing of a timezone offset from UTC time into the > broken-out time field tm_gmtoff. It supports timezone offsets between > UTC-12:00 and UTC+12:00, returning an error (NULL) for values outside > that range. > > However, time zone offsets outside the current range limits exist both > currently and historically: > > * Present day: > - Pacific/Auckland (New Zealand) summer time +13:00 > - Pacific/Kiritimati (Christmas Island) +14:00 > - Pacific/Apia (Samoa) summer time +14:00 > * Historical offsets exceeded +15:00/-15:00 > * POSIX supports -24:00 to +25:00 > * Paul Eggert notes: > - https://sourceware.org/ml/libc-alpha/2014-12/msg00068.html > > So, extend the timezone offset range to UTC-24:00 to UTC+25:00. > > James > > 2014-12-02 James Perkins james@loowit.net > > [BZ #16141] > * time/strptime_l.c (__strptime_internal): Extend %z timezone > offset range limits to UTC-24:00 to UTC+25:00 to parse current > and historical uses. > --- > time/strptime_l.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) The patch looks ok modulo the nit below. It would be good to add a test however. > diff --git a/time/strptime_l.c b/time/strptime_l.c > index b3a612e..1cdce41 100644 > --- a/time/strptime_l.c > +++ b/time/strptime_l.c > @@ -777,7 +777,10 @@ __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM) > return NULL; > val = (val / 100) * 100 + ((val % 100) * 50) / 30; > } > - if (val > 1200) > + /* valid range UTC-24 to +25, ala POSIX */ The comment should be a sentence so start with a capital and end with a full-stop and a couple of spaces. > + if (neg && val > 2400) > + return NULL; > + if (!neg && val > 2500) > return NULL; > tm->tm_gmtoff = (val * 3600) / 100; > if (neg) > -- > 1.7.9.5 >
On Wed, Dec 3, 2014 at 1:41 AM, Will Newton <will.newton@linaro.org> wrote: > On 3 December 2014 at 02:27, James Perkins <james@loowit.net> wrote: >> This is a fix for [BZ #16141] strptime %z offset restriction. > The patch looks ok modulo the nit below. It would be good to add a test however. I'm reworking it to add tests. >> + /* valid range UTC-24 to +25, ala POSIX */ > > The comment should be a sentence so start with a capital and end with > a full-stop and a couple of spaces. Thanks for the feedback, Will. In my rework I will also drop the range limit per discussion with Paul Eggert, to honor a -9959 to +9959 input offset range, so the comment will go away. Cheers, James
On 12/03/2014 05:28 PM, James Perkins wrote: > On Wed, Dec 3, 2014 at 1:41 AM, Will Newton <will.newton@linaro.org> wrote: >> On 3 December 2014 at 02:27, James Perkins <james@loowit.net> wrote: >>> This is a fix for [BZ #16141] strptime %z offset restriction. >> The patch looks ok modulo the nit below. It would be good to add a test however. > > I'm reworking it to add tests. Please keep in mind that as your change gets more complicated it will eventually require copyright assignment. Please review: https://sourceware.org/glibc/wiki/Contribution%20checklist All glibc developers should remind new contributors to look through the contribution checklist :-) Cheers, Carlos.
On Wed, Dec 3, 2014 at 9:41 PM, Carlos O'Donell <carlos@redhat.com> wrote: > Please keep in mind that as your change gets more complicated it will > eventually require copyright assignment. Thank you, Carlos. I would agree that a Copyright assignment is required if I contribute any creative work. As it stands I've added a few lines to an array initializer in one file and removed more lines than I added in another, so I can safely say none of my patch content is creative. > Please review: > https://sourceware.org/glibc/wiki/Contribution%20checklist I am glad to say I read through once to determine how best to submit my patches. It was very helpful in explaining the patch format and precise expected content. I will make sure to read through it again before post my next submission. > All glibc developers should remind new contributors to look through > the contribution checklist :-) Agreed. As this patch series represents my first contribution to glibc, I regret there are preferences of the community I missed the first pass through. I'm grateful for the constructive reviews I have received from Paul Eggert, Will Newton, and yourself. I feel I've had an encouraging introduction. Cheers, James
On 12/04/2014 04:23 PM, James Perkins wrote: > As this patch series represents my first contribution to glibc, I regret there > are preferences of the community I missed the first pass through. I'm > grateful for the constructive reviews I have received from Paul Eggert, Will > Newton, and yourself. I feel I've had an encouraging introduction. I'm very glad to hear that. I look forward to your contributions. Cheers, Carlos.
diff --git a/time/strptime_l.c b/time/strptime_l.c index b3a612e..1cdce41 100644 --- a/time/strptime_l.c +++ b/time/strptime_l.c @@ -777,7 +777,10 @@ __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM) return NULL; val = (val / 100) * 100 + ((val % 100) * 50) / 30; } - if (val > 1200) + /* valid range UTC-24 to +25, ala POSIX */ + if (neg && val > 2400) + return NULL; + if (!neg && val > 2500) return NULL; tm->tm_gmtoff = (val * 3600) / 100; if (neg)