tags:

views:

1080

answers:

2

I'm trying do to a basic bash with the use of system calls but I'm having some little problems with a pointer array.

To resume my code, I read commands from the stdin with read() to a buffer then I use strsep() to separate the command from the arguments and all the arguments into an array. Then I create a new process with fork() and execute that command with the associated arguments with execvp().

All this goes into a infinite loop until the user types "quit" (not coded yet). The problem is that after the first iteration, I need *pArgs to be empty, for the next command and arguments. And I don't know how to do it...

Here's my code:

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

int main(int argc, char **argv) {
    char bBuffer[BUFSIZ], *pArgs[10], *aPtr = NULL, *sPtr;
    int aCount;
    pid_t pid;

    while(1) {
     write(1, "\e[1;31mmyBash \e[1;32m# \e[0m", 27);
     read(0, bBuffer, BUFSIZ);

     sPtr = bBuffer;
     aCount = 0;

     do {
      aPtr = strsep(&sPtr, " ");
      pArgs[aCount++] = aPtr;
     } while(aPtr);

     pArgs[aCount-2][strlen(pArgs[aCount-2])-1] = '\0';

     // Debug code to output pArgs content
     write(1, "|>", 2);
     write(1, pArgs[0], strlen(pArgs[0]));
     write(1, "<|", 2);

     if(strlen(pArgs[0]) > 1) {
      pid = fork();

      if(pid == -1) {
       perror("fork");
       exit(1);
      }

      if(pid == 0) {
       execvp(pArgs[0], pArgs);
       exit(0);
      }
     }
    }

    return 0;
}

P.S: Sorry but I can't provide an input and output test case at the moment. Hopefully, this is not that hard to understand and fix that you guys don't need it. I'll post it later if it's needed though...

Just to clear things up:
I know I asked how to clear the array and I got an answer for that. But it seems now obvious to me that my problem wasn't that, but the garbage that the buffer was collecting as pointed out by litb. It makes more sense to terminate the string with the null character than to clear the array. That's why I'm marking litb's answer as right one.

+7  A: 
int i;
for (i = 0; i < 10; i++)
   pArgs[i] = NULL;
rpetrich
Much better than a memset()!
Jonathan Leffler
I answered with memset() and then deleted my answer because it's too hard to explain why sizeof(pArgs) is OK in this case but might not be OK in a different case (eg. when the type of pArgs is char**). +1 to the 'for' loop, -1 for memset in this context.
RichieHindle
You should have left it up. Memset is still proper and is the better way to go when speed is an issue. (see: http://www.cppreference.com/wiki/c/string/memset) A decent compiler will probably optimize this for loop to a memset anyway tho
rpetrich
I agree, better remove the memset. It's not always correct. The loop always works. If you know that null pointers are comprised by null bytes, you can do that, but by no means you will notice a speed difference in that shell!
Johannes Schaub - litb
Dingo
I just reread the C FAQ and Dingo appears to be right. Although NULL must equal ((void *)0), the bit pattern for a "zero pointer" may not be the same as for a "zero integer". I will remove the memset as it's invalid on certain architectures.
rpetrich
+1  A: 

Your problem is that you don't add a null character after the data read. So the strsep calls don't know where to stop. In C, strings must be terminated by a null character (that's called the terminating null character).

// don't forget to add error handling at some point (s == -1)
ssize_t s = read(0, bBuffer, BUFSIZ-1);
bBuffer[s] = '\0';

With that in place, i don't see what array should be cleared now, since execvp will read arguments until the first null pointer. The do loop, however, adds that null pointer already, which is the null pointer returned by the last invocation of strsep.

The problem would of course also be solved too by just clearing bBuffer (the data where *pArgs is pointing to after the first command was scanned). Note that you have to do that also before scanning the first time, since you can't assume the chars in bBuffer array are initialized to any sensible values.

memset(bBuffer, 0, sizeof bBuffer);

Place that just before the read invocation (but in any case, read only maximally BUFSIZE-1, because the terminating null character must have space too!).

But as I've shown above, you don't need this memset call. Just add the terminating null character manually.

Johannes Schaub - litb
I don't understand why my answer is downvoted. Clearly, his problem is not generated by not clearing the pointer-array, but the problem is somewhere else. So simply showing the correct way to clear out that array is good, but certainly showing how to correct the bug is at least equally important!
Johannes Schaub - litb
I think his "infinite loop" is intentional, and you misread the question. But I am upvoting this because it is important information that the submitter should heed.
rlbond
I did not intend to break the infinite loop, of course. See the read, let's assume BUFSIZ is 255. And you type "echo hello". the buffer then contains [echo helloblahblusomerandomdata.......]. How is strsep supposed to ever operate sanely? And just clearing the pointer array just won't do it. What he really wanted is clearing the bBuffer array. But that's not necessary as i told in the answer
Johannes Schaub - litb
However, my recommendation that he could get rid of the = '\0'; line after the do-while loop of course was wrong. He did it to get rid of the '\n' character. I'm afraid i totally overlooked the purpose of that line.
Johannes Schaub - litb
I can still get rid of the line to clear the '\n' character if I do bBuffer[s-1] = '\0' instead of bBuffer[s] = '\0' as you suggested. And since I have to clear that anyways, it's better to do s-1, than to have that big and confusing line with strlen() just to clear the '\n' character. :)
Nazgulled
Indeed, that's what i thought about it too. Nice to see i haven't lost my marbles yet :)
Johannes Schaub - litb