Re: [PATCH v2] Ensure mktime sets errno on error (bug 23789)

[Paul Eggert <eggert at cs dot ucla dot edu>]

Albert ARIBAUD (3ADEV) wrote:

>     __tzset ();
> +  /* record errno from __tzset() but do not fail now.  */
> +  errno_from_tzset = errno;

Why not fail now? If tzset fails, mktime is pointless.

Also, remember this code is shared with Gnulib, and POSIX does not specify what 
tzset does to errno. So you can't rely on tzset setting errno, at least not in 
the Gnulib usage of this code. And can you even rely on glibc __tzset doing it? 
It's not part of tzset's spec, surely.

I thought the idea was to define a glibc-specific tzset variant that returned an 
error number, or something like that. I don't see that here.

Also, how about moving the tzset call into the # if defined _LIBC || 
NEED_MKTIME_WORKING branch? tzset shouldn't be needed otherwise.


> +  if (result == -1 && errno == 0)
> +    return result;> +  errno = 0;
> +  else if (errno != 0)
> +    return -1;
> +  else if (errno_from_tzset != 0)
> +    __set_errno(errno_from_tzset);
> +  else
> +    __set_errno(errno_before_mktime);
> +  return result;

Sorry, but this is all too complicated. Remind me again why we can't rely on 
__mktime_internal to set errno on failure.

We don't care what happens to errno when __mktime_internal returns any value 
other than -1. Does that help?

I was hoping that mktime itself could look something like the following, which 
would be a lot simpler. Of course __tzset_with_result would have to be written 
(both in the Gnulib and the glibc case; the Gnulib version is easy), and 
__mktime_internal would have to do the right thing with errno, but this is the idea.

time_t
mktime (struct tm *tp)
{
  int r = __tzset_with_result ();
  if (r < 0)
    return r;

# if defined _LIBC || NEED_MKTIME_WORKING
  static mktime_offset_t localtime_offset;
  return __mktime_internal (tp, __localtime_r, &localtime_offset);
# else
#  undef mktime
  return mktime (tp);
# endif
}