Re: [PATCH 2/2] newlib/libc/time/tzset_r.c(_tzset_unlocked_r): POSIX angle bracket <> support

[Corinna Vinschen <vinschen@redhat.com>]

On Feb 28 11:55, Brian Inglis wrote:
> On 2022-02-28 05:04, Corinna Vinschen wrote:
> > On Feb 25 09:39, Brian Inglis wrote:
> > > +#define TZNAME_MIN	3	/* POSIX specified minimum TZ abbr size */
> > Are you sure?
> 
> Minimum is historical.

It's no POSIX value, so the comment is misleading.  The value of 3
is fine, after all it's part of the POSIX description for TZ.

> > > +#elif	 defined(_SC_TZNAME_MAX)
> > > +#define TZNAME_MAX	sysconf(_SC_TZNAME_MAX)	/* use sysconf value */
> > 
> > This is not a safe bet.  _SC_TZNAME_MAX is defined in unistd.h
> > unconditionally, even for targets not providing sysconf().  And
> 
> Why define indices if they are unusable on that platform?

They are never unusable!  They have to be defined as minimum set of
index values requested by POSIX.  A platform can easily return -1 for
all of them to indicate it doesn't care for limits.  Or, by also
setting errno to EINVAL, indicate that a value is entirly unavailable
in the current environment (which may differ on the same system).

> > The real problem is, you can't use sysconf(_SC_TZNAME_MAX) like this:
> > - You don't know at compile time if the function is really supported.
> > - You don't know if _SC_TZNAME_MAX returns a positive value or -1.
> 
> The docs appeared unclear whether the index or return value could be -1.
> POSIX sysconf(3p) man page says -1/EINVAL means invalid argument value which
> should not apply if _SC_TZNAME_MAX is defined, otherwise -1/errno is
> unchanged means no definite limit?!

Right.  The required usage of sysconf goes like this:

  #include <errno.h>
  #include <stdio.h>
  #include <unistd.h>

  errno = 0;
  int ret = sysconf (_SC_foo);
  if (ret != -1)
    printf ("We have a limit %d\n", ret);
  else if (errno)
    printf ("We have an error %d\n", errno);
  else
    printf ("We have no limit\n");

Incidentally, Cygwin sets errno for _SC_TZNAME_MAX, rather than just
returning -1.  I just fixed that.

> I can just enclose it in HAVE_SYSCONF,

Newlib configury does not run this kind of test macros.  After all, they
would only test if that function is available on the build system.  If
the function is available on the target system, it would be defined by
newlib itself, or be part of libgloss.

> > So, either the system defines TZNAME_MAX, or the system provides
> > sysconf() *and* sysconf(_SC_TZNAME_MAX) returns a positive value.  In
> > these cases, a check against that value makes sense.  In all other
> > cases, there's just no limit to check for.
> 
> So I may as well default to using the hardwired limit of 10, which is the
> most sensible value allowing for five letters, sign, four digits.

Yeah, we might keep it at that.  Cygwin isn't affected anyway, since it
runs its own tzset code, see tzparse() in winsup/cygwin/localtime.cc.
So we're doing this mostely for small targets.  10 should be fine.


Thanks,
Corinna