Re: [PATCH v2 2/2] [BZ #16141] strptime: fix %z minutes calculation

[Will Newton <will dot newton at linaro dot org>]

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. The offset includes an hour portion
> and an optional minute portion (ex. -08 or -0800 means eight hours
> before UTC, whereas -0830 means eight hours and thirty minutes before
> UTC).
>
> However, in the current implementation, the minutes portion calculation
> is correct only for minutes portion values evenly divisible by 3. This
> is because the minutes value is converted to decimal time, and not
> enough precision is used for rounding to cause incorrect results. For
> example, a +1159 offset string results in a tm_gmtoff of 43128 (== 11 *
> 3600 + 3528) seconds, instead of 43140 (== 11 * 3600 + 59 * 60) seconds.
>
> This fix calculates the offset from the hour and minute portions directly,
> without the rounding errors introduced by decimal time.
>
> James
>
> 2014-12-02  James Perkins  james@loowit.net
>
>         [BZ #16141]
>         * time/strptime_l.c (__strptime_internal): Fix %z minutes
>         calculation, removing incorrect decimal time rounding, so that
>         all minute values result in a valid seconds value
> ---
>  time/strptime_l.c |   12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/time/strptime_l.c b/time/strptime_l.c
> index 1cdce41..659fd91 100644
> --- a/time/strptime_l.c
> +++ b/time/strptime_l.c
> @@ -770,19 +770,15 @@ __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM)
>             else if (n != 4)
>               /* Only two or four digits recognized.  */
>               return NULL;
> -           else
> -             {
> -               /* We have to convert the minutes into decimal.  */
> -               if (val % 100 >= 60)
> -                 return NULL;
> -               val = (val / 100) * 100 + ((val % 100) * 50) / 30;
> -             }
> +           else if (val % 100 >= 60)
> +             /* minutes valid range is 0 through 59. */

Comment should start with a capital and have two spaces at the end.

> +             return NULL;
>             /* valid range UTC-24 to +25, ala POSIX */
>             if (neg && val > 2400)
>               return NULL;
>             if (!neg && val > 2500)
>               return NULL;

Are these conditions still correct?

It would be good to add a test case or two to ensure the changes work
as intended.

-- 
Will Newton
Toolchain Working Group, Linaro