Shell-like program C

Question

The code below is supposed to work as a shell. It has the previous and next option, a history like feature, the exit and the execute command.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#define BUFFER_SIZE 256
#define HISTORY_LENGTH 128

int executeCommand(char*cmd)
{
    return(!strcmp(cmd,"e\n"));
}

int exitCommand(char*cmd)
{
    return (!strcmp(cmd,"exit\n"));
}

int previousCommand(char*cmd)
{
    return (!strcmp(cmd,"p\n"));
}

int nextCommand(char*cmd)
{
    return (!strcmp(cmd,"n\n"));
}

void execute(char *line)
{
    line[strlen(line)-1]='\0';
    char **arguments;
    char* temp;
    int i=0;
    arguments=(char**)malloc(sizeof(char)*10);
    temp=strtok(line," ");
    arguments[i]=malloc(strlen(temp)*sizeof(char));
    if(arguments[i]!=NULL)
    {
        strcpy(arguments[i],temp);
        i++;
    }
    else
    {
        printf("Out of memory");
    }
    while(temp!=NULL)
    {
        temp=strtok(NULL," ");
        if(temp==NULL){
            arguments[i]=NULL;
        }
        else{
            arguments[i]=malloc(strlen(temp)*sizeof(char));
            if(arguments[i]!=NULL)
            {
                strcpy(arguments[i],temp);
                i++;
            }
        }
    }
    printf("%s  ",arguments[0]);
    printf("%s  ",arguments[1]);
    printf("%s  ",arguments[2]);
    execvp(arguments[0],arguments);
}

int main(int argc, char*argV[]) {
    int i;
    char *cmd=(char*)malloc(sizeof(char)*BUFFER_SIZE);
    char **history=NULL;
    int historylength=0;
    int currentCommand=0;
    history=(char**)malloc(sizeof(char)*BUFFER_SIZE);
    do{
        fgets(cmd,BUFFER_SIZE-1,stdin);
        if(exitCommand(cmd))
            break;
        else
            if(previousCommand(cmd))
            {
                if(currentCommand>0)
                    printf("%s",history[--currentCommand]);
                else if(currentCommand==0)
                {
                    currentCommand=historylength;
                    printf("%s",history[--currentCommand]);
                }
            }
            else
                if(nextCommand(cmd))
                {
                    if(currentCommand<historylength)
                        printf("%s",history[currentCommand++]);
                }
                else
                    if(executeCommand(cmd))
                    {
                        execute(history[--currentCommand]);
                    }
                    else
                    {
                        history[historylength]=malloc(strlen(cmd)*sizeof(char));
                        if(history[historylength]!=NULL)
                        {
                            strcpy(history[historylength],cmd);
                            currentCommand=++historylength;
                        }
                        else
                        {
                            printf("Out of memory");
                            break;
                        }
                    }

    } while(1);

    free(cmd);

    for(i=0;i<historylength;i++)
        free(history[i]);
    free(history);

    return 0;
}

I want to make this work for the function cat. I type in the e cat main.c and I expect it to execute the cat command but it's not doing anything, what am I doing wrong here? I'm not a pro at this so I appreciate all the help.

Since you are new to C, one thing that can help with your readability is to use "else if" instead of an else block with an if in the block. It would reduce your nesting levels and make your code more readable. — RonaldBarzell, Dec 10 '12 at 21:45
I thing you should introduce some better indentation for your code because your `if/ else if` construction is extremely unreadable. Also when you decompose your logic into smaller pieces it is easier to find a bug. — Kylo, Dec 10 '12 at 21:48

score 3 · Accepted Answer · edited May 23 '17 at 10:34

3

This is incorrect:

arguments=(char**)malloc(sizeof(char)*10);

as arguments is a char**, so the code needs to allocate sizeof(char*). Change to:

arguments = malloc(10 * sizeof(*arguments));

Same mistake for history also. Additionally, see Do I cast the result of malloc?

There is one char less than required being allocated for the char* as strcpy() writes a terminating null character. Change:

arguments[i]=malloc(strlen(temp)*sizeof(char));

to:

arguments[i] = malloc(strlen(temp) + 1);

sizeof(char) is guaranteed to be 1 and can be omitted from the size calculation.

Prevent i from exceeding the bounds of the memory allocated for arguments. As the code currently stands there is no protection to prevent i from exceeding 9 (0 to 9 are the valid values for i as arguments was allocated 10 elements).

edited May 23 '17 at 10:34

Community

1
1

answered Dec 10 '12 at 21:44

hmjd

120,187
20
207
252

By preventing you mean stating in the code that $i< args$ or just make sure not to type in more than 10? – Max Bummer Dec 10 '12 at 21:58
Add `i < 10` to the `while` loop terminating condition. – hmjd Dec 10 '12 at 21:59
I've made all the changes you suggested but when it comes to the cat command, it stays the same. What should I do to make it execute it? – Max Bummer Dec 10 '12 at 22:17

Shell-like program C

1 Answers1