[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-gnulib] addition: findprog.h, findprog.c
From: |
Bruno Haible |
Subject: |
Re: [Bug-gnulib] addition: findprog.h, findprog.c |
Date: |
Thu, 10 Apr 2003 22:15:53 +0200 (CEST) |
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