Is this code reasonable?

The 'spec' is to write a 'whereis' program for Windows. There was no
need to have argv[1] be valid sot hat bit I've added.

The spec also said that to detect an existing file, one could use
anything. As gcc had access() I used that.

Note that I've yet to comment this up - so please don't tell me to do
that - I will.

The question I mostly have is, as this is a short program, is it ok to
have reused 'path'?

Firstly it's assigned the return from getenv(), then the return from
the initial strtok() call, and then it's assigned NULL to keep
strtok() going.

Thanks

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <io.h>

char const * whereis(char const * const prg)
{
static char filepath[MAX_PATH];

char * path;

path = getenv("PATH");

while((path = strtok(path, ";")) != NULL)
{
strncpy(filepath, path, MAX_PATH);

if(path[strlen(path) - 1] != '\\')
{
strncat(filepath, "\\", MAX_PATH);
}

strncat(filepath, prg, MAX_PATH);

if(access(filepath, X_OK) == 0)
{
return filepath;
}

path = NULL;
}

return NULL;
}


int main(int argc, char * argv[])
{
char const * filepath;

if(argc != 2)
{
puts("Arg required");
}

else

{
if((filepath = whereis(argv[1])) != NULL)
{
puts(filepath);
}

else

{
puts("Not found");
}
}

return 0;
}

re: Is this code reasonable?

sosij.morris@gmail.com wrote:What's <io.h>? Some kind of Windows-ism, perhaps?
What's MAX_PATH? Something from <io.h>, maybe? Is it an
array size or a string length (i.e., is the trailing '\0'
accounted for)? Does it differ in any important way from the
Standard-defined FILENAME_MAX?
Two problems here, both potentially fatal. First, if getenv()
returned a NULL, passing that NULL to strtok() is an error. Second,
strtok() may modify the pointed-to string, and trying to modify the
string returned by getenv() is also an error.
You've fallen for the "strncpy() is a safe strcpy()" myth.
Note that strncpy() does not necessarily produce a string: it may
leave `filepath' without a trailing '\0'.
The third argument of strncat() doesn't mean what you think
it means.
The third argument of strncat() doesn't mean what you think
it means.
I guess access() and X_OK are from <io.h>?
I see nothing objectionable in NULLing the variable for the
next iteration. My own preference would have been to gather all
the iteration-by-iteration stuff in one place:

for ( ; (path = strtok(path, ";")) != NULL; path = NULL) {
...
}

.... but that's only a preference, and yours are allowed to differ.
De gustibus non disputandum est (Latin: "There's no point arguing
with Gus").
Other remarks: For use on Windows, `whereis waldo' should
probably also look for `waldo.exe' and `waldo.bat' and maybe
`waldo.com' and `waldo.lnk' and a few others. Also (you should
double-check this) I think Windows will always search the current
directory for executables and things even if they're not in the
PATH, so you might want to look there.

For further advice on the Windows-specific stuff, I suggest
you try a Windows forum.

--
Eric.Sosman@sun.com

re: Is this code reasonable?

On Mon, 21 Jul 2008 11:57:48 -0700 (PDT),
sosij.morris@gmail.com <sosij.morris@gmail.comwrote:Yes.

path is a pointer, not the string itself. strtok() may modify the string
pointed to by its first argument, and will return a pointer into that
same string on a successful match.

getenv() returns a pointer to the value in the environment, so when you
use strtok() on that, you're modifying that value. It's not a local copy.
In fact, the C99 standard (and the POSIX standard) specifically states
that you're not allowed to modify the string pointed to by the pointer
returned by getenv(), so you shouldn't (as Eric already stated). Make a
local copy.
You can never infer the full specification of a function from its
signature. You need to read the documentation that comes with it; in
this case that is the C standard, although your local documentation
should tell you the same thing.
Correct. It isn't so. It modifies what you give it. This is one of the
reasons I never use strtok() to tokenise strings that I need for
something else later on. I only really ever use it to tokenise lines I'm
reading from files. Anything more permanenent in the program I generally
use strpbrk(), strcspn() or strchr() for

Martien
--
| All code should be deliberately written for
Martien Verbruggen | the purposes of instruction. If your code
| isn't readable, it isn't finished yet.
| --Richard Heathfield

re: Is this code reasonable?

sosij.morris@gmail.com wrote:Do you have access to a C textbook or reference? Do you
have access to `man' pages or `info' or `help'? Do you have
access to Google? Do you have enough intellectual curiosity
to prompt you to use any of these? Or would you like a few
more grapes peeled for you? Sheesh ...
A trusting soul, I see. Sosij, a person like you really
ought to own something of importance, something of monumental
scale. It happens that I own a bridge -- architectural marvel,
widely celebrated, the pride of its city and used by thousands
of vehicles every single day -- and since I myself hardly ever
visit Brooklyn to enjoy my bridge, I'm thinking of selling it.
Would you be interested? It can be yours for only -- well, how
much have you got?
strtok() doesn't modify `path'; it modifies the string that
`path' points to. `path' is your own variable and you can do
with it whatever you like, but the string it points to is NOT
yours: it is part of the process' environment. The C Standard
is quite clear on this point: the program "shall not modify" the
string, and a program that violates a "shall" it invokes undefined
behavior. Once you indulge in U.B. there's no telling what might
happen, and the C Standard washes its hands of your program.
If your documentation doesn't describe what strtok() does,
your documentation is deficient. If your documentation says that
strtok() makes a copy of the string, your documentation is wrong.
I suggest you re-read the documentation -- and if it's truly as
bad as you say, complain to the vendor.
The "best" way, in my view, is not to rely on constant-sized
arrays for this sort of thing. Get the length of the PATH piece,
add one for the \, add the length of the appendage, add another 1
for the terminating zero, and use malloc(). Then use strcpy() and
strcat() with no 'n's.
Did your documentation fail to describe strncat(), too? If
so, you should not just complain to the vendor: you should blog
at length about the vendor's obstinate nitwittedness until you
can shame them into doing a better job. If the documentation is
as bad as you imply, "half-hearted" would be an improvement.

--
Eric Sosman
esosman@ieee-dot-org.invalid