[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paul Eggert
Subject: Re: [Bug-gnulib] addition: findprog.h, findprog.c
Date: 10 Apr 2003 11:59:19 -0700
User-agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.3

Bruno Haible <address@hidden> writes:

> /* Look up a program in the PATH.
>    Attempt to determine the pathname that would be called by execlp/execvp
>    of PROGNAME.  If successful, return a pathname containing a slash
>    (either absolute or relative to the current directory).
>  ....
>             /* 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"?  Presumably the caller will pass
PROGNAME to execl, execv, execle, or execve, and these functions are
don't need "./x".  It will simplify your code if you omit that, and I
don't see what it would hurt.

>    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.

>   path = getenv ("PATH");
>   if (path == NULL || *path == '\0')

The "|| *path == '\0'" should be omitted.  For example:

   $ env - sh
   $ echo $PATH

   $ echo ${PATH-x}     
   $ type sh
   sh is /usr/bin/sh
   $ PATH=
   $ type sh
   sh not found

>   /* 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?  This would probably result in a bit
cleaner code since it wouldn't need to destructively modify a copy of

>       int last;

'last' should be bool.

>       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++)
   last = (*cp == '\0');

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

>       /* 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 "/".

>       /* 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.)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]