Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Roland McGrath <roland at hack dot frob dot com>]

> > uint8_t is big enough for d_type, and will make the struct smaller on
> > ILP32.
> 
> I assume the struct is turned into scalars anyway, but I added the
> __typeof__.

I like that fine, but we need to check if it's OK in gnulib.

> Further cleanups should wait until we have a decision whether we will
> ever try to synchronize with gnulib again.

The starting place should always be to expect that we want to share
everything we can with gnulib.  If there is something we in the abstract
can share with gnulib but you want to duplicate and maintain separately
rather than share, you need to make a clear case justifying the ways in
which it's better not to share.  Just that we have an annoying amount of
divergence already is not such a justification.  Rather, we should always
start by seeing if we can reharmonize before doing any other changes.  If
some cleanups on one side or the other make it easier to reharmonize, then
that's a reason to do them first.  But otherwise, we should be achieving
harmony first and then fixing things afterwards so the fixes are uniform
in both places.

> >> -		      len = NAMLEN (d);
> >> -		      names->name[cur] = (char *) malloc (len + 1);
> >> +		      names->name[cur] = strdup (e.d_name);
> >>  		      if (names->name[cur] == NULL)
> >>  			goto memory_error;
> >> -		      *((char *) mempcpy (names->name[cur++], name, len))
> >> -			= '\0';
> >> +		      ++cur;
> > 
> > In the _DIRENT_HAVE_D_NAMLEN case, this assumes that strdup is as efficient
> > as malloc+mempcpy (with no strlen required).  Perhaps it's close enough,
> > but that is a subtle change you didn't mention as intended.
> 
> I want to avoid conditionalized code because these things tend to break
> after a while (or never work in the first place).

That might be a sufficient reason to avoid writing such code in the first
place.  But when you are changing something from how it already is, you
need to be clear about what you are changing and why.  My point was not
that I had no idea what your motivation for the specific change might have
been.  My point was that you billed this whole larger change as doing one
thing, and then slipped this in without comment.

In this case, we already have supported configurations of both cases.
Hurd is _DIRENT_HAVE_D_NAMLEN (it uses the generic bits/dirent.h), while
others are not.  So just the configurations that work today not being
broken in the future (and being adequately tested) prevents "tending to
break after a while".

I am making a big deal out of this little case, and it's not because I
think this particular little performance-relevant change is so likely to
be important or even that I personally necessarily object to it.  What I
want to make a big deal about is the project's standards for attention
to detail and for clarity and transparency about intent and effect of
changes.  Sometimes "and I slipped this other little change in while I
was at it" is fine, though the starting place is always that every
separately-motivated change should be a separate change.  What's never
fine is slipping such a change in without comment or discussion, which
is what happened here.  Now we're discussing it rather than it going
unnoticed, which is what matters most.  But I am also concerned by the
way you responded to my review, which to me read as an off-hand remark
about a personal preference as if that were an adequate response.  Such
preferences are normal in the reasoning behind writing new code in a
particular way, and in such cases will likely go unremarked.  But here
you are changing existing code in a way that changes the compiled code
and its performance characteristics, and the only motivation you cited
was your preferences for how code should be all else being equal.  That
completely ignores the history behind the code being how it is today,
and hence ignores the project's key principle of conservatism.

> +/* Extract name and type from directory entry.  No copy of the name is
> +   made.  If SOURCE is NULL, result name is NULL.  */
> +static struct readdir_result
> +convert_dirent (const struct dirent *source)
> +{
> +  if (source == NULL)
> +    return (struct readdir_result) {};
> +  else
> +    return (struct readdir_result)
> +      {
> +	.name = source->d_name,
> +	D_INO_TO_RESULT (source),
> +	D_TYPE_TO_RESULT (source)
> +      };
> +}
> +
> +#ifndef COMPILE_GLOB64
> +/* Like convert_dirent, but works on struct dirent64 instead.  */
> +static struct readdir_result
> +convert_dirent64 (const struct dirent64 *source)
> +{
> +  if (source == NULL)
> +    return (struct readdir_result) {};
> +  else
> +    return (struct readdir_result)
> +      {
> +	.name = source->d_name,
> +	D_INO_TO_RESULT (source),
> +	D_TYPE_TO_RESULT (source)
> +      };
> +}
> +#endif

While inlines are better than macros when all else is equal, this
duplication of something that lends itself to a single-expression form
(?:) is a good reason to make it a macro instead.

Also, compound literals are a GNU extension and I'm not sure if they
meet gnulib's portability requirements or not.


Thanks,
Roland