2
\$\begingroup\$

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.

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks. I appreciate your feedback and will edit the code and post it. \$\endgroup\$
    – jarr
    Jan 18, 2016 at 0:52
  • \$\begingroup\$ @jarr Okay, just make sure not to do that in the same post. \$\endgroup\$
    – SirPython
    Jan 18, 2016 at 1:01
  • \$\begingroup\$ I have seen some posts where people re-edit to add new updates. I guess that's against the rules? So the usual way is to keep the post as it is, keeping the suggestion to yourself or create a new one with the updates, right? \$\endgroup\$
    – jarr
    Jan 18, 2016 at 1:32
  • \$\begingroup\$ @jarr Right you are! Thanks for checking the rules, first! :) \$\endgroup\$
    – SirPython
    Jan 18, 2016 at 1:36
  • \$\begingroup\$ Question. Point 1, I was like "that completely makes sense", if I am just using one struct at a time and want to stick with it, then just allocate once and memset to zero for the iterations. When I was writing this I was thinking of making this struct as a node which will be part of a job linked list. But point taken. Point 2, taken. Very creative and simple. Point 3, you're right, do "EXIT_FAILURE"?. Point 4, would adding a "static" would also make it better? \$\endgroup\$
    – jarr
    Jan 18, 2016 at 16:32
2
\$\begingroup\$
  • 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.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks. I appreciate your feedback and will edit the code and post it. \$\endgroup\$
    – jarr
    Jan 18, 2016 at 0:52
  • \$\begingroup\$ vnp, I have a question. Point 1, taken but what about the errno variable? Is it still common in practice to save the value before and after a signal handler? Also Point 3, is it about the program ignoring SIGINT and SIGQUIT? \$\endgroup\$
    – jarr
    Jan 18, 2016 at 16:11
  • \$\begingroup\$ vnp, what about the call to error_msg() from the signal handler? I guess that one is also unsafe right? \$\endgroup\$
    – jarr
    Jan 18, 2016 at 17:48
  • \$\begingroup\$ @jarr Since it calls printf it is automatically unsafe. Re errno I suppose you should - if the handler is prone to change it. \$\endgroup\$
    – vnp
    Jan 18, 2016 at 17:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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