Basic shell implementation

Question

Here is a small project to implement a basic shell. It is a personal ongoing project to keep system call usage fresh. It currently does background processes and some basic signal handling.

I am actually looking for code feedback in regards to professionalism. I want to know if my code is clear and easy to understand. I know my main function is currently doing too much and that I will handle in the short future.

/*
Description: This is a light implementation of a Linux shell
*/

#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <signal.h>
#include <sys/types.h>
#include <sys/wait.h>

#define MAX_LEN 4096
#define MAX_ARGS 256


struct proc_st {
    int bg;
    int exec;
    int commc;
    int fd_out;
    int fd_in;
    pid_t pid;
    char pbuff[MAX_LEN];
    char *commv[];
};


/* function prototypes */
void error_msg(char *);
void sig_handler(int);
void comm_identi(struct proc_st *, char *);

int main()
{
    struct proc_st *proc;
    char buff[MAX_LEN];
    pid_t pid;
    int status;

    /* initialize signal handlers */
    signal(SIGINT, SIG_IGN);
    signal(SIGQUIT, SIG_IGN);
    signal(SIGCHLD, sig_handler);

    printf("[SHELL]> ");
    for (;;) {

        while (fgets(buff, MAX_LEN, stdin) != NULL) {

            proc = malloc(sizeof(struct proc_st) + MAX_ARGS);

            /*initialize to zero */
            memset(proc, 0, sizeof(struct proc_st));
            proc->exec = 1;
            comm_identi(proc, buff);

            if (proc->exec == 1) {
                if ((pid = fork()) < 0 ) {
                    printf("fork error");
                } else if (pid == 0) { /* child */
                    if (execvp(*(proc->commv), 
                        proc->commv) < 0 )
                        error_msg("could not execute");
                    free(proc);
                }

                /* parent */
                if (proc->bg == 0) {
                    if ((pid = waitpid(pid, &status, 0)) 
                            < 0)
                        error_msg("waitpid error");
                } else
                    printf("PID %d %s\n", pid, proc->pbuff);
            }
            free(proc);
            printf("[SHELL]> ");
        }
    }
    exit(0);
}



void sig_handler(int signal)
{
    pid_t pid;

    while ((pid = waitpid(-1, NULL, WNOHANG | WUNTRACED)) > 0) {
        printf("Child Process [%d]: Terminated", (int)pid);
    }
    if (errno != ECHILD)
        error_msg("waitpid() error");
}

void error_msg(char * msg)
{
    fprintf(stderr, "%s: %s\n", msg, strerror(errno));
    exit(0);
}


void comm_identi(struct proc_st *proc, char *buffer)
{
    char *tmp;

    /* replace newline with null */
    if (buffer[strlen(buffer) - 1] == '\n')
        buffer[strlen(buffer) - 1] = 0;
    strncpy(proc->pbuff,buffer,MAX_LEN);

    tmp = strtok(buffer, " ");
    while (tmp != NULL) {
        proc->commv[(proc->commc)++] = tmp;
        tmp = strtok(NULL, " ");
    }

    proc->commv[proc->commc] = NULL;

    /* check for built-in functions */
    if (proc->commc >= 1) {
        if(!strcmp(proc->commv[0], "exit")) {
            free(proc);
            exit(EXIT_SUCCESS);
        }

        if(!strcmp(proc->commv[0], "cd")) {
            proc->exec = 0;
            if (chdir(proc->commv[1]) != 0)
                error_msg("chdir() error");
        }

        /* check if it is a background process */
        if (*proc->commv[(proc->commc) - 1] == '&') {
            proc->bg = 1;
            proc->commv[(proc->commc) - 1] = NULL;
        }
    }

    return;
}

This code and updates are available in the github repo.

Answer 1

Allocation and freeing

In the while loop of your infinite loop, you are allocating memory for proc only to be completely freeing it by the end of the loop. Why? Considering every loop you reset with memset, why not keep the same allocated memory for the entire time?

Your code would be much faster if you moved the allocation of proc out of both loops, and if you did not free the memory every iteration, too. Also, you seem to be freeing proc in a few other places in the loop; see if you can remove those, too.

---

Refactor built-in functions

Here you are checking for commands that require special attention:

if(!strcmp(proc->commv[0], "exit")) {
    free(proc);
    exit(EXIT_SUCCESS);
}

if(!strcmp(proc->commv[0], "cd")) {
    proc->exec = 0;
    if (chdir(proc->commv[1]) != 0)
        error_msg("chdir() error");
}

What if you need to add another command to this list for whatever reason? Now, you could just add another if, but then your code starts to look a little more messy.

To make your code more flexible here, you could try splitting these built-in functions into a struct which stores the name of the built-in function and a pointer to a function to execute if this is come across.

Then, you could create a function that checks if the command entered is a built-in function. If it is, then the function can easily handle the situation using the built-in function's struct.

Here is a short snippet of what I mean:

struct builtin {
    char *name;
    function pointer;
} builtins = {
    "exit", builtin_exit,
    ...
};

Then, all you need is a function to check through builtins to see if the command entered is a built-in function.

From there, if you ever need to add another built-in function, you simply add another one to the array (and create its respective function), and you don't even have to touch the shell's main code.

---

Exit number

In your error_msg function, you have this:

exit(0);

Well, that doesn't make much sense; if there's an error, the code needs to return a non-zero number to indicate failure. Other than that, the environment in which this program was run will not be able to tell that there was an error.

---

const int or #define

#define MAX_LEN 4096
#define MAX_ARGS 256

It would be better to use a const int here because that will actually define a type for the variable, where #define simply sticks that literal string after the macro name into the code and leaves it up to the compiler to determine a type for it.

Answer 2

• Calling prinf from signal handler is unsafe (see man 7 signal).

• comm_identi parses the command line and sometimes executes the command. It is clear violation of a single responsibility principle. Leave the command execution logic to the main loop.

• The purpose of the surrounding for(;;) loop seems to keep the shell going after fgets() returns NULL. Since there is no attempt to recover (and some conditions, like hitting EOF are not recoverable at all), once NULL is returned the shell ends up in an infinite loop.