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

[Florian Weimer <fweimer at redhat dot com>]

On 03/31/2016 07:16 PM, Florian Weimer wrote:
> On 03/31/2016 01:27 AM, Roland McGrath wrote:
>>>> 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.
> 
> I'm going to use unsigned char instead.
> 
>>>>> -		      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.
> 
> Two more things:
> 
> For the non-glob64 case, I removed a copy of the name, which amounts to
> one traversal less.  The internal strlen operation in strdup adds back a
> string traversal.  So there is no net difference of the work done
> (except that there are fewer writes).  CONVERT_DIRENT_DIRENT64 is used
> in the glob64 code, so there is an unnecessary strlen operation there.
> 
> But the glibc tests for GLOB_ALTDIRFUNC do not set the d_namlen field.
> Neither does the code in OpenSSH.  (GNU make sets d_namlen, though.)
> Not looking at d_namlen avoids introducing application bugs.
> 
> d_ino/d_fileno has a similar problem, and even make doesn't set it. We
> should probably ignore it altogether in GLOB_ALTDIRFUNC mode.

Correction: make initializes d_ino to 1.

Florian