making shell in C

Question

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <string.h>
#include <sys/stat.h>
#include <fcntl.h>

void exec(char **args){
    pid_t  pid;
    int status;
    if ((pid = fork()) < 0) {    
        printf("*** ERROR: forking child process failed\n");
        exit(1);
    }
    else if (pid == 0) {      
        if(execvp(args[0],args)<0)//{
            //printf("argv[0]=%s argv[1]=%s",args[0],args[1]);
            printf("**error in exec\n");
    }
    else {                                  
        while (wait(&status) != pid);
    }
}

void exec2(char **args, char *file){
    printf("file =%s\n",file);
    int fd;
    pid_t pid;
    int status;
    if ((pid = fork()) < 0) {    
        printf("*** ERROR: forking child process failed\n");
        exit(1);
    }

    else if (pid == 0) {   
        fd = open(file, O_RDWR | O_CREAT, (mode_t)0600);
        close(1);
        dup2(fd, 1);
        if(execvp(args[0],args)<0){
            printf("**error in exec");
        }

        else {  
            printf("\nhere\n");
            close(fd);                                
            while (wait(&status) != pid){
                fflush(stdout) ;
            }
    }
}
    close (fd);
}


void main(){
    char *command;
    char inp[512];
    char *filepath;
    size_t size=0;
    char *substr;
    char *args[512];
    command = (char *) malloc(sizeof(char *) * 512);
    int flag=0;
    int redirect=0;
    int i=0;
    while (1){
        printf("$ ");
        command = fgets(inp, 512/*sizeof(char *)*/, stdin);

        command[strlen(command)-1]='\0';

        if (strchr(command,'>')){
            redirect=1;
            strtok_r(command,">",&filepath);
        }

        size_t siz=4;
        //printf("command=%s\n",command);
        int i=0;
        while(1){
            //printf("i=%d\n",i);
            char *tok = strtok_r(command," ",&substr);
            if (tok==NULL){
                break;
            }
            args[i++] = tok;
/*              printf("tok=%s\n",tok);
            printf("len tok = %d\n",(int)strlen(tok));
            printf("command=%s\n",command);
            printf("substr=%s\n",substr);     
*/           command = substr;
    }

    //printf("args[0]=%s",args[0]);
    if (!strncasecmp(args[0],"exit",siz) || !strncasecmp(args[0],"quit",siz))
    {
        printf("\nBye\n");
        exit(0);
    }

    else if(strcmp(args[0],"cd")==0){
        chdir(args[1]);
        //printf("chdir")   ;
        //system("pwd");
} 

else if (redirect==1){
    exec2(args,filepath);
}

else exec(args);
}
}

Okay this is my code for my shell. When i run it, i put ls and it gives correct output. Then i put ls -l and then ls again and it gives:

ls: cannot access : No such file or directory

Also when I use cd, ls doesnt give output and pwd says:

"ignoring unused arguments"

Also cat doesnt work. Though mkdir, ps and ls -l works.

Note that `main()` returns an `int`; therefore you should not write `void main()` but `int main(void)` (or `int main(int argc, char **argv)` or an equivalent). — Jonathan Leffler, Nov 12 '12 at 18:01
You need to make sure that your child process exits if the `execvp()` fails. At the moment, it prints an error message (on standard output (bad); it should go to standard error!) and then resumes operation, so you have two processes trying to run as a shell, which is a sure-fire way of getting confused. — Jonathan Leffler, Nov 12 '12 at 18:04

score 2 · Accepted Answer · edited May 23 '17 at 12:04

2

I see a few problems in you code, first you allocate way more than 512 for command because you use sizeof(char*) which is most likely 4:

command = (char *) malloc(sizeof(char*) * 512);

Instead, use sizeof(char) and you shouldn't cast the result of malloc there's some logic behind it, but it's probably a matter of preference, I personally don't:

command = malloc(sizeof(char) * 512);

Second, you're not using strtok_r correctly, first time you call it you pass the string, subsequent times, you pass NULL so it should be:

char *tok = strtok_r(command, " ", &substr);
while(tok){
    args[i++] = tok;
    tok = strtok_r(NULL, " ", &substr);
}

Finally, this is the main problem, the last argument needs to be NULL:

args[i] = 0;

edited May 23 '17 at 12:04

Community

1
1

answered Nov 12 '12 at 16:53

iabdalkader

17,009
4
47
74

+1 for the comment on casting the return value of `malloc()`. – Nov 12 '12 at 17:06
What is wrong with the way that `strtok_r()` is being used? I suppose that your concern is restarting the scan at the address pointed to by the 3rd pointer; it's aconventional (you're supposed to use a null pointer as the first argument to resume where you left off) but probably works. And there are sound reasons for not using `strtok()` — I want to say 'never use it', but that's a fraction too strong, but it's very, _very_, **very**, ***very*** seldom better to `strtok()` than `strtok_r()`. I'm very close to downvoting, especially since I don't agree with the 'never cast `malloc()`' meme. – Jonathan Leffler Nov 12 '12 at 17:57
@JonathanLeffler he's assigning the result of `strtok_r` to the args array, and those are pointers to a static buffer used by `strtok_r` for parsing, as you probably know, and as I've explained,that's what I meant by using it wrong, also the only reason it works is because he passes the `saveptr` again, I was wondering why go through all the trouble if you're not going for thread-safety. – iabdalkader Nov 12 '12 at 18:07
@JonathanLeffler about the `never cast malloc()` I actually never said that :) I said you should, but you're right, it's probably a matter of preference, will edit. – iabdalkader Nov 12 '12 at 18:08
The pointers returned by `strtok()` (with or without the `_r` or `_s` suffix) is a pointer to a location in the string passed as the first argument. Especially with `strtok_r()`, where the `_r` is for 're-entrant', there is no internal storage. Indeed, the static internal storage required by plain `strtok()` is the main reason you should not use it; that and the fact that if `strtok()` is in a library function, it is completely poisonous. The C standard mandates 'The implementation shall behave as if no library function calls the `strtok` function.' _[...continued...]_ – Jonathan Leffler Nov 12 '12 at 18:15
_[...continuation...}_ So, the pointers from `strtok()` and `strtok_r()` are pointers into the string being parsed (or NULL); there is no problem with assigning those to `argv[i]` unless the original string will go out of scope before the `argv` array is used. That's not a problem in this code; it could be in other circumstances. – Jonathan Leffler Nov 12 '12 at 18:16
@JonathanLeffler I agree about the `strtok_r`, will edit now, but you say that both `strtok` and `strtok_r` return pointers to the string, but the man page says `strtok() returns a pointer to a null-terminated string` this means it modifies the string ? – iabdalkader Nov 12 '12 at 18:20
@JonathanLeffler never mind that, it does modify the first argument, anyway, thanks for pointing that out, I've edited the question. – iabdalkader Nov 12 '12 at 18:32
gives me a segmentation fault if i do `command = malloc(sizeof(char) * 512);` – gitter Nov 14 '12 at 10:14
@user1818202 if you need more than 512 bytes for the command then allocate more, but it could be something else also follow Jonathan's comments on your question regarding the child process – iabdalkader Nov 14 '12 at 10:53

making shell in C

1 Answers1