Parsing and Simple Shell in C

Question

I wrote this simple shell in C for a university assignment on operating systems (My guess is that it's just the beginning and the next assignments will add to this code).

The program prints the prompt to the user, receives an input and try to execute it. The program ends when the input is 'done', and prints some statistics.

• cd command is not supported
• For some reason (maybe the real shell does that too?) if getpwuid or getcwd fails we've been told to print the prompt with NULL so I didn't check for failure.
• About casting mallocs, I know that it's not necessary

my concerns are:

1. Should the main be broken into more functions?
2. Is the idea behind parseString good?
3. About freeArr - In this case I can know for sure that the array will end by a NULL, is it still bad practice to free that way? should I pass the array size to each functions instead?
4. Any other suggestion will be welcomed

Here is the code:

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

#define SENTENCE_LEN 511
#define PATH_MAX 512

void printPrompt();
char *getUserName();
int numOfWords(const char sentence[]);
void parseString(char sentence[], char** parsedStr);
void freeArr(char** parsedStr);
void exeCommand(char** command);
void donePrint(int numOfCommands, int lengthOfCommands);

int main() {

char sentence[SENTENCE_LEN];
char** parsedStr;
int numOfCommands = 0, lengthOfCommands = 0, parsedStrLen;

while(1)
{
    printPrompt();
    if(fgets(sentence , SENTENCE_LEN, stdin) != NULL) {
        parsedStrLen = numOfWords(sentence);
        lengthOfCommands += (int) (strlen(sentence) - 1);
        numOfCommands++;
        if(parsedStrLen > 0) {
            parsedStr = (char **) malloc((parsedStrLen + 1) * sizeof(char*));
            if (parsedStr == NULL) {
                fprintf(stderr, "malloc failed");
                exit(1);
            }
            parsedStr[parsedStrLen] = NULL;
            parseString(sentence, parsedStr); //allocates the words in the array
            if(strcmp(parsedStr[0], "done") == 0 && parsedStrLen == 1)
            {
                donePrint(numOfCommands, lengthOfCommands);
                freeArr(parsedStr);
                break;
            }
            pid_t id;
            id = fork();
            if (id < 0) {
                perror("ERR");
                freeArr(parsedStr);
                exit(1);
            }
            else if (id == 0) {
                exeCommand(parsedStr);
                freeArr(parsedStr);
            }
            else {
                wait(NULL);
                freeArr(parsedStr);
            }
        }
    }
}
return 0;
    }


//Prints prompt in the following format user@current dir>, if getUserName or getcwd fails -
// print NULL in their place
void printPrompt()
{
    char* userName = getUserName();
    char currentDir[PATH_MAX];

    getcwd(currentDir, sizeof (currentDir));
    printf("%s@%s>", userName, currentDir);
}

//Returns the user name as a char*, if getpwuid fails - return NULL
char *getUserName()
{
    uid_t uid = geteuid();
    struct passwd *pw = getpwuid(uid);
    if (pw)
        return pw->pw_name;
    else {
        return NULL;
    }
}

//receives a sentence and returns the number of words in it, ignoring blank spaces
int numOfWords(const char sentence[] )
{
    int i = 0, wordCounter = 0;
    while(sentence[i] != '\n')
    {
        if(sentence[i] != ' ' && (sentence[i+1] == ' ' || sentence[i+1] == '\n'))
            wordCounter++;
        i++;
    }
    return wordCounter;
}

/*receives a sentence as a char[] and a char**
 *assign dynamically the sentence words into the char**
 */
void parseString(char sentence[], char** parsedStr) {
    char tmpWord[SENTENCE_LEN];
    int tmpIndex = 0, parsedIndex = 0;
    for (int i = 0; i < strlen(sentence); i++) {
        if (sentence[i] != ' ' && sentence[i] != '\n') {
            tmpWord[tmpIndex] = sentence[i];
            tmpIndex++;
        }
            //End of word
        else if ((sentence[i] == ' ' || sentence[i] == '\n') && tmpIndex > 0) {
            tmpWord[tmpIndex] = '\0';
            parsedStr[parsedIndex] = (char *) malloc((strlen(tmpWord)) + 1);
            if (parsedStr[parsedIndex] == NULL) {
                freeArr(parsedStr);
                fprintf(stderr, "malloc failed");
                exit(1);
            }
            strcpy(parsedStr[parsedIndex], tmpWord);
            parsedIndex++;
            tmpIndex = 0;
        }
    }
}

//Receives an array and free each cell from 0 to the first NULL
void freeArr(char** parsedStr) //---should maybe get the array size although?
{
    int i =0;
    while(parsedStr[i])
        free(parsedStr[i++]);
    free(parsedStr);
}


// Receives a command to execute as an array** and execute it using execvp, cd not supported
void exeCommand(char** command)
{
    if(strcmp(command[0], "cd") == 0) {
        printf("command not supported (YET)\n");
        freeArr(command);
        exit(0);
    }
    else if(execvp(command[0], command) != -1)
    {
        freeArr(command);
        exit(0);
    }
    else {
        perror("command not found");
        freeArr(command);
        exit(1);
    }
}

//Prints the total num of commands, the total length of commands and the average command length
void donePrint(int numOfCommands, int lengthOfCommands) {
    printf("Num of commands: %d\n", numOfCommands);
    printf("Total length of all commands: %d\n", lengthOfCommands);
    printf("Average length of all commands: %f\n", (double) (lengthOfCommands) / numOfCommands);
    printf("See you Next time !\n");
}

Answer 1

Answers to your questions

Should the main be broken into more functions?

Yes. Keep it as high level as possible. Basically, it should look like:

int main() {
   char sentence[...];

   while (fgets(sentence, sizeof sentence, stdin) != NULL) {
       processSentence(sentence);
   }
}

Where processSentence() would do everything necessary to process a single "sentence".

Is the idea behind parseString good?

It's one way of doing it. You could also consider modifying sentence in place, changing the byte after each word to a NUL-byte, and just storing pointers into the original string in the array parsedStr. This avoids having to call malloc().

Another possible improvement is to avoid scanning the input twice, once to count words, and another to build parsedStr. Instead, do one pass, and use realloc() to grow the array parsedStr() as necessary while scanning the input.

About freeArr - In this case I can know for sure that the array will end by a NULL, is it still bad practice to free that way? should I pass the array size to each functions instead?

Since you know the size you could indeed pass the array size to the function and use that instead. It will probably generate slightly more efficient machine code. But if you avoid calling malloc() for each word as suggested above, you avoid this issue as well.

Any other suggestion will be welcomed

See below.

Naming things

The name "sentence" is a bit strange, we normally call this a "command line", although it is also just called "command". The words on a command line are called "arguments". The first one is usually also what we call the "command", although it does not have to be. So I would recommend the following name changes:

• sentence -> commandline
• parsedStr -> arguments or args
• numOfCommands -> numArguments or nargs

Also try to use nouns for variables, and verbs for functions. This is mostly what you did already, but I would suggest these changes:

• numOfWords() -> countArguments()
• donePrint() -> printStatistics()

Avoid forward declarations

If you put main() at the bottom of your source file, you don't need the forward declarations at the start of the program anymore. This avoids having to repeat yourself, and also avoids potential mistakes.

Make functions static when possible

If a function is only used within the same source file, you should mark it as static. This prevents these functions from polluting the global namespace (which might become important if your program gets split into multiple source files), and this might help the compiler generate better code.

Use more const

I see you already used const for the parameter of numOfWords(), but you can also use it for the parameter sentence in parseString(), and command in exeCommand().

Avoid hardcoding maximum sizes

You hardcode the length of the input to 511 bytes, and paths to 512 bytes. But what if I need to run a command that is longer than 511 bytes? What will happen in your code is that the first 510 bytes will be parsed as one command line, then the next 510 bytes will be parsed as another command line, and so on. This might results in unexpected and potentially dangerous behavior. Consider using a dynamically allocated buffer for the command line instead, and grow it with realloc() if necessary.

The same goes for the buffer for getcwd(). What will happen if the buffer is not long enough? The manpage says that it will return NULL and set errno to ERANGE, but it doesn't tell you if the contents of the buffer will be valid, and it is better to assume it is not. Again, use a dynamically allocated buffer, and resize it if necessary as the documentation suggests.

Error messages

Your error messages are not very consistent. First, you use printf(...), fprintf(stderr, ...) and perror(). Try to be more consistent. perror() is a good choice whenever errno has been set, which includes the case when malloc() fails. For all other cases, use fprintf() to print to stderr.

Also avoid making assumptions about errors: if execvp() returns an error, it can either be because the command was not found, or it could not be executed (maybe it didn't have the right file permissions to execute), or there was not enough memory to load the program, and so on.

Also be careful when checking whether a function returned an error. The manpage for fork() says that on error, -1 is returned, but it does not say that all negative numbers are errors.