This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/3] posix: execvpe cleanup



On 26-02-2016 16:38, Paul Eggert wrote:
> This one has similar problems with int vs ptrdiff_t. Also:
> 
> On 02/26/2016 05:56 AM, Adhemerval Zanella wrote:
>>   for (const char *p = path; ; p = subp)
>>     {
>>        if (errno == ENOEXEC)
>>         maybe_script_execute (buffer, argv, envp);
> 
> This has O(P*C) behavior, where P is the length of the path and C is the argument count. How about changing it to have O(P + C) behavior instead, by allocating the substitute argv in __execvpe, and reusing it each time through the loop? (Admittedly the current code also has this performance bug.)
> 

I do not oppose, although I would like to focus on fixing the usability and
conformance bugs first and set the performance goal as possible future
improvement. I will add a comment about this possible optimization.

>>   /* Construct an argument list for the shell.  */
>>   char *new_argv[argc];
> 
> This should be "argc +1", not "argc". Shouldn't we have a test case to catch bugs like this?

Indeed I will change that.  I add a testcase.

> 
>>   new_argv[0] = (char *) _PATH_BSHELL;
>>   new_argv[1] = (char *) file;
>>   while (argc > 1)
>>     {
>>       new_argv[argc] = argv[argc - 1];
>>       --argc;
>>     }
> 
> This mishandles the case where argc == 1, which is possible with an empty argument vector. The resulting argument vector is not null-terminated. (Admittedly the current code also has this bug.) I suppose we should have a test case for this.
> 

I will fix that.

>>   bool got_eacces = false;
>>   for (const char *p = path; ; p = subp)
>>     {
>>       char buffer[path_len + file_len + 1];
> 
> Declare 'buffer' just before the loop starts, not in the loop body. That will make the code run a bit faster, as the buffer will be allocated and deallocated only once. This is OK since (path_len + file_len + 1) does not change.
> 

I will change that.

>>       /* Set current path considered plus a '/'.  */
>>       memcpy (buffer, p, subp - p);
>>       buffer[subp - p] = '/';
>>       /* And the file to execute.  */
>>       memcpy (buffer + (subp - p) + !!(subp > p), file, file_len + 1);
> 
> Shouldn't this be using mempcpy? That should simplify the code. I find the "!!" ugly and unnecessary; there's nothing wrong with treating a boolean as an int, and besides, !! on a boolean still gives you a boolean so what's the point?  Something like this, perhaps:
> 
>      /* Use the current path entry, plus a '/' if nonempty, plus the file to execute.  */
>      char *pend = mempcpy (buffer, p, subp - p);
>      *pend = '/';
>      memcpy (pend + (p < subp), file, file_len + 1);
> 
> 

I do not have a preference regarding to "!!" and your suggestion seems better.
I will change that.

>> +      /* Record the we got a 'Permission denied' error.  If we end
> 
> the -> that

Ack.

> 
>> +# define EXECVP(__file, __argv) execvp (__file, __argv)
> 
> No need for the underscores here.

OK, I will change that.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]