Re: getusershell: Split file by lines instead of spaces.

[Collin Funk]

Hi Bruno,

You said:
>> #if ! defined ADDITIONAL_DEFAULT_SHELLS && defined __MSDOS__
>> # define ADDITIONAL_DEFAULT_SHELLS \
>>   "c:/dos/command.com", "c:/windows/command.com", "c:/command.com",
>
> Inside '#if defined __MSDOS__' or '#if defined _WIN32' this is OK.
> I was worried there was explicit code to accept such prefixes on POSIX
> compatible platforms. Fortunately not.

Sorry for the confusion. I didn't mean to suggest adding drive
prefixes on non-Windows systems hahaha. That would be an "interesting"
choice.

>> I've attached a patch following this method.
> 
> I see a method 'next_shell' that is only called in a single place. Would
> it make sense to inline it (and move its function comments into the
> body of the caller function)?

Good point. I thought it was a bit strange to pass the file stream and
buffer as arguments when they are static global variables in that
file.

I've attached a patch moving it to the end of getusershell. Makes
things easier to understand IMO. I haven't pushed it yet so let me
know what you think.

> The main change is to call getline() instead of parsing the file
> character-by-character. I guess that the motivation is that the
> behaviour of trimming leading whitespace and trimming trailing whitespace
> before '#' would lead to convoluted code with the previous approach?

Pretty much, yes. I tried to do that originally but I don't think you
would have liked the patch. :)

At the start of 'readname' there is a loop to skip spaces:

     while ((c == getc (stream)) != EOF
            && isspace ((unsigned char) c))
       /* Do nothing.  */;

Essentially in the main for (;;) loop you would have to compare every
character for '#'. If one is found then you have to do a loop similar
to that and then break afterwards.

Since the file format is for the most part "one shell per line", with
a few caveats, I think it makes sense to just use getline(). Makes
things easier to read in my opinion.

Judging by the copyright date on that file (1991), I assume getline
wasn't portable enough to use (or didn't exist?) then. If I remember
correctly it was a glibc extension that was later adopted by POSIX.

>> The patch removes 'GNULIB_GETUSERSHELL_SINGLE_THREAD' which uses
>> getc_unlocked instead of getc for optimization ...
>>
>> I don't see too much harm in it though since on most platforms getline
>> should lock much less then repeatedly calling getc.
> 
> Right. When the function uses getline instead of getc, it will lock 10x
> less than before. I agree there is not much need to optimize this.
> Especially since we don't have a getline_unlocked function. And since
> locking has become cheaper since this code was written. (Futexes were
> invented in 2002-2003.)

Interesting. I don't know very much about Futexes.

Do most libc implementations keep track of if a stream needs to be
locked? I'm thinking about single-threaded programs that don't define
GNULIB_GETUSERSHELL_SINGLE_THREAD. I imagine the stream doesn't locked
x10 more, but a status flag is checked x10 more or something.

Collin

0001-getusershell-Split-file-by-lines-instead-of-spaces.patch
Description: Text Data