Re: [Bug-gnulib] addition: findprog.h, findprog.c

[Bruno Haible]

Thanks Paul for the thorough comments!

> >           /* Add the "./" prefix for real, that concatenated_pathname()
> >              optimized away.  */
> 
> If PROGNAME is "x" and will be found in the current directory, why is
> it necessary to return "./x"?

Since the function is only an optimization, and can return PROGNAME
unchanged in weird cases, the caller must still use execlp, execvp,
etc. The "./" avoids that these functions search the PATH again.

> >    Otherwise,
> >    return PROGNAME unmodified.  */
> 
> The comment should say that if the result is not PROGNAME it was
> allocated via malloc, and should be freed when no longer needed.

Yes. Done.

> >   path = getenv ("PATH");
> >   if (path == NULL || *path == '\0')
> 
> The "|| *path == '\0'" should be omitted.

No. POSIX:2001 section 8.3 (line 5766 in draft6) says:

   If PATH is unset or is set to null, the path search is
   implementation-defined.

In this case returning PROGNAME unchanged is the only safe thing that
can be done.

>  For example:
> 
>    $ env - sh
>    $ echo $PATH
> 
>    $ echo ${PATH-x}     
>    x
>    $ type sh
>    sh is /usr/bin/sh
>    $ PATH=
>    $ type sh
>    sh not found

Your particular sh treat an empty PATH like an empty list of
directories. It could also treat it like an unset PATH without
violating POSIX.

> >   /* Make a copy, to prepare for destructive modifications.  */
> >   path = xstrdup (path);
> 
> This allocates O(length(PATH)) storage, so it appears that you're
> trying to minimize time and not space.  But can't we do even better,
> and issue at most one call to malloc, which allocates strlen (path) +
> strlen (progname) + 1 bytes?

But then the returned string would be allocated in a block of memory
that is way too long for it. I.e. we would waste memory.

> This would probably result in a bit cleaner code since it wouldn't
> need to destructively modify a copy of path.

Hmm, I think it simpler code to work on a single char* than to copy a
'const char *' into a 'char *' buffer incrementally.

> >       int last;
> 
> 'last' should be bool.

Yes, agreed. This file predated the stdbool module.

> >       for (cp = dir; *cp != '\0' && *cp != ':'; cp++);
> >       last = (*cp == '\0');
> 
> Can you please avoid ");"?  It's confusing.  Better is this:
> 
>    for (cp = dir; *cp != '\0' && *cp != ':'; cp++)
>      continue;

The 'continue;' is confusing too. I'll use

     for (cp = dir; *cp != '\0' && *cp != ':'; cp++)
       ;

> Also, why not use memchr (dir, ':') for this?

A call to memchr followed by a call to strlen is not necessarily
faster than just scanning the string with a 'for' loop.

> >       /* Concatenate dir and progname.  */
> >       progpathname = concatenated_pathname (dir, progname, NULL);
> 
> Sorry, what is concatenated_pathname?  I am concerned about the case
> where DIR ends in "/".

concatenated_pathname is in a module I added a week ago. It does
handle the case of a trailing directory separator correctly.

> >       /* This program is usually not installed setuid or setgid, therefore
> >      it is ok to call access() despite its design flaw.  */
> 
> That is true for gettext, but what about other possible applications?
> Perhaps it is better to invoke eaccess or whatever other messy function
> is available.  (Yuck, I admit.)

Yes, I'll use eaccess if it exists, since it appears to be supported
by SVR4 and FreeBSD systems.

Bruno