0

I am very confused. In the while loop I am adding each file name into an array and printing it afterwards.

However in the for loop it prints some weird things and stops executing in the middle.

Please help me to fix it.

char *commands[1000];

char *str;
DIR * dir;

struct dirent * entry;

char *env = getenv("PATH");

do {
    str = strsep(&env, ":");

    if(str != NULL)
        if(strlen(newEnv) > 0) {

            dir = opendir(str);
            if( dir == NULL ) break;

            flag = 0;
            while((entry = readdir(dir)) != NULL) {
                commands[++count] = entry->d_name;
                printf("---%i %s\n", count ,commands[count]);  // prints fine
            }
            closedir(dir); // close directory

        }

} while(newEnv);

commands[++count] = '\0';
printf("count = : %i\n", count);

for(int i = 0; i <  count; i ++)
{
    if(commands[i] == NULL)
        break;
    printf("aaa%i %s\n\n", i, commands[i]);  //problem loop
}
Improve this question
5
  • 1
    Please try to construct a minimal test case (see sscce.org). All the stuff to do with opendir, etc. is probably irrelevant to the problem, so you can probably remove it from your code snippet. Commented Aug 30, 2011 at 11:06
  • Note that it is not safe to use strsep on environment variables, since it modifies them. However this is not the source of your problems. Commented Aug 30, 2011 at 11:07
  • a properly indented source code helps a lot... what is this newEnvvariable ? Commented Aug 30, 2011 at 11:11
  • Why are you skipping the first array element? Commented Aug 30, 2011 at 11:15
  • Your code doesn't compile as newEnv, flag, and count are not defined. strsep()is not found either but this is maybe because I did not include the proper include file. Which one is it? Commented Aug 30, 2011 at 11:21

4 Answers 4

Reset to default
2

You are setting your pointers pointing into the dir struct received by readdir. However, that struct is deallocated by closedir.

You need to copy the strings (strdup and friends).

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

Replace commands[++count] = entry->d_name; by commands[++count] = strdup(entry->d_name);.
ok that seems working fine but for loop is not executed for some reason??
I guess, this is because you never set commands[0]. So it might well be 0 and cause your loop to break immediately. You should consider incrementing count after you have written to the array, remove that (then) useless line "commands[++count] = '\0';" and take care for what MatthieuW pointed out.
I mean, replace commands[++count] = entry->d_name; printf("---%i %s\n", count ,commands[count]); by commands[count++] = entry->d_name; printf("---%i %s\n", count,commands[count-1]); (assuming you will remove the printf later).
2

commands[++count] = entry->d_name doesn't copy the entry->d_name string into commands[++count], but merely assigns the pointer. In the next iteration entry->d_name is overwritten since readdir uses static memory for returning the result.

Use strcpy instead and allocate the memory yourself.

Improve this answer

3 Comments

is it sth like this??? char *new; new = strcpy(new, entry->d_name); commands[++count] = new;
@kanoz: Before the strcpy you need to allocate memory like this: new = malloc(strlen(entry->d_name) + 1).
did that but for loop is not executed this time... why do u think??
0

The problems:

  • you add pointers to the array, which point to strings that will be deallocated by readdir/closedir shortly after
  • if the starting value of count is 0, commands[0] is never assigned to and contains garbage
  • the commands[++count] = '\0'; line seems to be a mistake, but luckily it adds 1 to count so the print count line prints right number and the for loop iterates over the right range
Improve this answer

Comments

0

In addition to @Eugene Homyakov's answer:

You should not rely in your last loop on "commands" array to be initialized with NULL values.

Improve this answer

2 Comments

The order in which answers are displayed on SO is variable - you should probably make this a comment to the relevant answer otherwise it's not clear which answer you are referring to
Tanks for the tip. Actually I was referring to Eugene comment. My remark is not related but I just prefer not to repeat his useful comments.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.