On Fri, May 31, 2013 at 01:05:08AM -0430, Andres Perera wrote:
> Besides being redundant, it's generally better to bail on open() error
> instead of checking if the length is less than PATH_MAX in
> anticipation.
>
> [Unrelated to your patch, but since we are on the subject of path
> checks in setlocale()...]
>
> However, since the routines also use locale data paths as keys for a
> dynamically allocated cache of their contents, a single check should
> persist. This raises an unrelated issue about the cache index: since
> libc doesn't support arbitrary paths for locale files, there's no need
> to index the cache by full locale data paths because all of the paths
> are anchored at _PATH_LOCALE.
That's true nowadays. However, there used to be an environment
variable to override _PATH_LOCALE. But that code had a bug and
was deemed unnecessary, so I removed it in r1.6 of setrunelocale.c.
The caching could now be adjusted, I guess. But it's not a big deal
so I'll leave it alone.
> >> on src/lib/libc/locale/setrunelocale.c, _xpg4_setrunelocale():
> >>
> >> 184 len = snprintf(path, sizeof(path),
> >> 185 "%s/%s/LC_CTYPE", _PATH_LOCALE, encoding);
> >> 186 if (len < 0 || len >= sizeof(path))
> >> 187 return ENAMETOOLONG;
> >
> > This section is modified as part of my diff, isn't it?
>
> Sorry, this assertion remains correct after your diff so there was no
> need for me to list it.
>
> In any case, it's also redundant and should be zapped to prevent
> further confusion. There's yet another (x < PATH_MAX) check--making it
> the third one--which is a better candidate to keep due to its coupling
> with the cache population logic:
Right, the check is redundant during the
_xpg4_setrunelocale->_newrunelocale. But I'd like keep the idiom
that checks the result of snprintf() in place.
>
> 125 int
> 126 _newrunelocale(const char *path)
> 127 {
> 128 struct localetable *lt;
> 129 FILE *fp;
> 130 _RuneLocale *rl;
> 131
> 132 if (strlen(path) + 1 > sizeof(lt->path))
> 133 return EINVAL;
>
> >
> >> > + /* Assume "<whatever>.<encoding>" locale name. */
> >>
> >> There should be some notion of syntax for cc_LL.CTYPE, even if only
> >> mentioned in comments.
> >>
> >> E.g.,
> >>
> >> ISO 3166-1 for country codes and BCP 47 for language tags.
> >>
> >> glibc did not do this and directly because of that it's a mess to
> >> navigate their structure.
> >
> > You mean we should specify encoding name syntax in a comment?
> > If so, my answer would be that recognized encoding names are
> > specified by filenames we use in /usr/share/locale. A comment
> > documenting the same would risk becoming obsolete over time.
> >
> > Updated diff, with the redundant check removed:
>
> [...]
