current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 7 down vote favorite
3

I am just getting started with C and made this simple shell to test my understanding of some basic principles. The shell reads commands from standard input, forks itself and executes the command. How am I doing, and where can I improve?

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

void parseCmd(char* cmd, char** params);
int executeCmd(char** params);

#define MAX_COMMAND_LENGTH 100
#define MAX_NUMBER_OF_PARAMS 10

int main()
{
    char cmd[MAX_COMMAND_LENGTH + 1];
    char* params[MAX_NUMBER_OF_PARAMS + 1];

    int cmdCount = 0;

    while(1) {
        // Print command prompt
        char* username = getenv("USER");
        printf("%s@shell %d> ", username, ++cmdCount);

        // Read command from standard input, exit on Ctrl+D
        if(fgets(cmd, sizeof(cmd), stdin) == NULL) break;

        // Remove trailing newline character, if any
        if(cmd[strlen(cmd)-1] == '\n') {
            cmd[strlen(cmd)-1] = '\0';
        }

        // Split cmd into array of parameters
        parseCmd(cmd, params);

        // Exit?
        if(strcmp(params[0], "exit") == 0) break;

        // Execute command
        if(executeCmd(params) == 0) break;
    }

    return 0;
}

// Split cmd into array of parameters
void parseCmd(char* cmd, char** params)
{       
    for(int i = 0; i < MAX_NUMBER_OF_PARAMS; i++) {
        params[i] = strsep(&cmd, " ");
        if(params[i] == NULL) break;
    }
}

int executeCmd(char** params)
{
    // Fork process
    pid_t pid = fork();

    // Error
    if (pid == -1) {
        char* error = strerror(errno);
        printf("fork: %s\n", error);
        return 1;
    }

    // Child process
    else if (pid == 0) {
        // Execute command
        execvp(params[0], params);  

        // Error occurred
        char* error = strerror(errno);
        printf("shell: %s: %s\n", params[0], error);
        return 0;
    }

    // Parent process
    else {
        // Wait for child process to finish
        int childStatus;
        waitpid(pid, &childStatus, 0);
        return 1;
    }
}
share|improve this question

closed as unclear what you're asking by 200_success♦ Oct 24 '14 at 3:34

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question.If this question can be reworded to fit the rules in the help center, please edit the question.
5  
Once you have answers, please don't edit the code any more. What you can and cannot do after receiving answers – Brythan Oct 24 '14 at 2:32
3  
@Henrik You have already been asked to stop modifying the code in the question after it has been answered. Changing the question is unfair to reviewers. I've closed the question. We will consider reopening it if you revert the code to a state before the first answer was posted. – 200_success♦ Oct 24 '14 at 3:40
up vote 7 down vote accepted

Looks pretty nice to me. A few remarks:

  1. There is no need for the malloc - your structures are small enough so they can live on the stack of main. So you could just do:

    int main()
{
    char cmd[100];
    char* params[10];

    ....
}

    then you don't need the free call. It also has the advantage that instead of fgets(cmd, 100, stdin) you can do fgets(cmd, sizeof(cmd), stdin) in case you change the size of cmd you won't have to remember to change it in multiple places.

  2. If your user types in a command with more than 10 parameters you will start overwriting memory. parseCmd should be given a maximum number of commands as parameter.

  3. You should define constants like 100 and 10 as named constants because it increases readability and you can easily change it in one place without having to sift through the entire code. So something like this:

    #define MAX_COMMAND_LENGTH 100
#define MAX_NUMBER_OF_COMMANDS 10

    and use those macros instead of the raw numbers.

share|improve this answer
up vote 5 down vote

  • execvp may fail on a number of reasons, command not found being just one of them (man execve for the list of possible errors. Check errno and call strerr(errno) for a precise error reason and message.

  • Beware that fork may fail too. It is necessary to test pid against -1, and again check errno and call strerr(errno).

share|improve this answer
up vote 4 down vote

This is my biggest complaint about the half-cuddled else:

}

// Parent process
else {

One of the problems with C is that the } at the end of a block after an if may mean that you are done with the if or it may mean that there is an else. The C compiler won't care if you have a hundred lines of whitespace before the else, but people have a reasonable expectation that done means done. Please either cuddle the } else { or at least make it the vertical equivalent:

}
else {
    // Parent process

Separating a closing } from its succeeding else will only lead to tears.

In general, I find your comments to be simply English versions of what your code does. Whomever is reading your code should be able to make that translation without comments. You should save comments for explaining why you are doing things, particularly if what you are doing is clever. Example: 

// Remove trailing newline character, if any

But that should be evident from the code. Why do you remove a trailing newline? Will it break the display later? Interfere with parsing? What? Another example:

    // Read command from standard input, exit on Ctrl+D

I think that this should be 

    // fgets returns NULL on Ctrl+D, so exit the loop on NULL

That tells the reader what you think is happening. In terms of this code review, it might let someone point out that fgets can also return NULL on an error. It also looks like fgets doesn't always return NULL on Ctrl+D. Try entering some input and then hitting Ctrl+D. At least that's what I get from the documentation.

share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.