Just Want an opinion

So I was trying to make a very primitive kind-of-shell and I ended ip doing the follow. I just want an opinion
This is my header where I declare the available funtions and make an array of structs where each struct has the name of the function to call the number of arguments it needs and a pointer to the function to call

Code:

---

#ifndef UTILITIES_H
#define UTILITIES_H

                /* ========================        FUNCTIONS      ======================== */
               
int my_mkfs(int argc, char **argv);

int my_mount(int argc, char **argv);

int my_unmount(int argc, char **argv);

int my_quit(int argc, char **argv);

int my_help(int argc, char **argv);

int my_ls(int argc, char **argv);

int my_cp(int argc, char **argv);

int my_mv(int argc, char **argv);

int my_rm(int argc, char **argv);

int my_df(int argc, char **argv);

int my_cat(int argc, char **argv);

int my_echo(int argc, char **argv);

                /* ======================== GLOBALS,STRUCTS,CONSTS ======================== */
               
//The following are mostly to simplify things for me as this is my first time trying something of this sort
//restriction of mine: each argument can have a length of 16 characters - max length of filenames of my filesystem
        #define ARG_LENGTH 16
// restriction of mine: at most there can be 8 arguments
        #define MAX_ARGS 8
/*
  My struct where I store pointers to functions. As you can see it has the name of the function it calls, the number
  of arguments it takes (including the command name - I thought of it like the arguments of main in c) and the
  pointer to the function it calls which are declared above and are implemented in other files. They all have the
  same format, return type int, arguments an int (number of arguments) and a char ** the arguments themseleves
*/
struct cmd_det{
        const char *name;
        int no_args;
        int (*func_to_call) (int argc, char **argv);
};
extern const struct cmd_det function_index[];
extern int shell_running;

#endif

---

And here is the shell implementation

Code:

---

#include <stdio.h>        //printf()
#include <stdlib.h>        //malloc(),
#include <string.h>        //strcmp(),
#include "utilities.h"


int break_cmd_line(char *line, char **argumentsR);
void clear_buffers(char *buf1, char **buf2);

int shell_running;        //state of shell - when running will be 1 and when it turns 0 it will exit
//Here I create the array of all the function pointers
const struct cmd_det function_index[] = {
        {"my_mkfs",3,&my_mkfs},       
        {"my_mount",1,&my_mount},
        {"my_unmount",1,&my_unmount},
        {"my_quit",1,&my_quit},
        {"my_help",1,&my_help},
        {"my_ls",1,&my_ls},
        {"my_cp",3,&my_cp},
        {"my_mv",3,&my_mv},
        {"my_rm",2,&my_rm},
        {"my_df",1,&my_df},
        {"my_cat",2,&my_cat},
        {"my_echo",3,&my_echo},
        {NULL,-1,NULL}
};

int main ()       
{       
        char cmd_line[ARG_LENGTH * MAX_ARGS]; //This is where I read the command typed by the user
        char arguments[MAX_ARGS][ARG_LENGTH];          //In here after breaking the above string I store each argument
        int args;                  //number of argument found in the cmd_line above
        int i,found;       
       
        shell_running = 1;
//This is the shell. Basically an endless while loop - what did i tell ya; primitve
        do       
        {
                clear_buffers(cmd_line,arguments);
                printf("os280fs> ");
//Here it gets the line from the user
                if ( !fgets(cmd_line, ARG_LENGTH*MAX_ARGS , stdin) ) break;
/*
  My function that breaks it. Again I considered using sscaf to break it but I think it's behaviour is unspecified
  when the format has more parts than found. I thought of using it like this sscanf(cmd_line,"%s %s %s %s...",arg1,
  arg2, ..., argMAX_ARGS) with the maximum arguments possible but like I said I didnt like it and to be honest I
  am little prejudices against the scanf family
*/
                args = break_cmd_line(cmd_line, arguments);
// If there are arguments try to find the aproppriate function
                if ( args != 0 )
                {
                        i = 0;
                        found = 0;
//iterate the array of structs to find the function to call
                        while (function_index[i].name != NULL )
                        {
//compare names
                                if ( strcmp(arguments[0],function_index[i].name) == 0 )
//compare number of arguments
                                        if ( args == function_index[i].no_args )
                                        {
/*
  call the function from the pointer inside the struct with arguments the number ofarguments (args) returned by my
  function and the arguments themselves, basically the pointer to the 2D array created inside my function where I
  broke the cmd_line
*/
                                                (*function_index[i].func_to_call) (args,arguments);
//This is nothing, just to know when a function wasnt fount and print a message
                                                found = 1;
                                                break;//no point to continue searching
                                        }
                                i++;       
                        }
                        if ( !found )        printf("No such command available\n") ;
                }
        }while ( shell_running );              //if my_quit is called shell_running changes to 0 and exits
        return 0;
}

//My function to break the cmd_line

int break_cmd_line(char *line, char **argumentsR)       
{
        char *iter;
        int i,j;
       
        i = 0;
        j = 0;
        iter = line;

//if there are leading whitespaces ignore them
        while ( *iter == ' ' || *iter == '\t' )
                iter++;
//And then if it's followed by new line the exit. Basically the user typed whitespace
        if ( *iter == '\n' )
                return 0;
        else
//but if not find the arfuments
        {
                int set = 0;        //this is used to know when there is another argument following whitespace
                while ( *iter != '\n' )
                {
//if set - 1 then there was found whitespace and then not a newline which means that a new argument follows
                        if ( set ) { i++; set = 0; } //and so go to a new row
//Copy the argument per character to the arguments table
                        if ( *iter!= ' ' && *iter != '\t')
                        {
                                argumentsR[i][j] = *iter;
                                j++;
                                iter++;
                        }
                        else
//whitespace is found
                        {
                                set = 1;
                                j = 0;//new row so start from the beginning when copying the argument
//eat up all the whitespace
                                while ( *iter == ' ' || *iter == '\t' )
                                        iter++;
                        }
                }
        }               
        return i+1; //return the number of arguments - +1 because arrays start from 0
}

void clear_buffers(char *buf1, char **buf2)
{
        int i;
       
        memset( buf1, 0, ARG_LENGTH * MAX_ARGS );
        for ( i = 0; i < MAX_ARGS; i++ )
                memset( buf2[i], 0, ARG_LENGTH );
}

---

If you have an editor then it is probably easier for you to read the code so I attached the corresponding files

Code:

---

        for (i=0; i<MAX_ARGS; i++)
                *(arguments+i) = (char *) malloc( ARG_LENGTH * sizeof(char) );

---

I prefer the style of:

Code:

---

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

---

It doesn't make any difference in what code is generated, but it is shorter to write, and is, in my mind, easier to read and understand. This is of course a personal opinion.

Code:

---

int shell_runnning;        //state of shell - when running will be 1 and when it turns 0 it will exit
char *cmd_line;              //This is where I read the command typed by th user to break it up later in pieces
char** arguments;        //In here after breaking the above string I store each argument       
int args;                          //number of argument found in the cmd_line above

---

Surely none of these need to be global?

Code:

---

//Here I create the array of all the function pointers
struct cmd_det function_index[] = {
        {"my_mkfs",3,&my_mkfs},       
        {"my_mount",1,&my_mount},
        {"my_unmount",1,&my_unmount},
        {"my_quit",1,&my_quit},
        {"my_help",1,&my_help},
        {"my_ls",1,&my_ls},
        {"my_cp",3,&my_cp},
        {"my_mv",3,&my_mv},
        {"my_rm",2,&my_rm},
        {"my_df",1,&my_df},
        {"my_cat",2,&my_cat},
        {"my_echo",3,&my_echo}
};

---

If you ever include this twice, your compiler will complain.

Declare it extern in the header-file (if needed), and then define it in one source-file.

Same applies to any other global variables - but it's better to have as few globals as possible - but the above table is one that doesn't make much difference if it's global or not. If you are not changing the table [I doubt you are, right?], then making it const will prevent stupid mistakes.

Code:

---

        char name[ARG_LENGTH];
        int no_args;
        int (*func_to_call) (int, char **);

---

If you change name to const char *, you will save some space - and avoid accidentally changing the name - you either get a compiler error or a access violation, depending on circumstances.

Code:

---

                    //This is nothing, just to know when a function wasnt fount and print a message
                                                i = CUR_UTIL + 1;

---

Use a flag and break, rather than changing the loop variable in the middle of the loop.

If you do the above change to name, I'd suggest you remove CUR_UTIL alltogether, and use a name == NULL to see if you got to the end of the list - that makes it fewer places to change if you add another command.

If you prefer to do it another way, you can #define CUR_UTIL (sizeof(function_index)/sizeof(function_index[0])).

--
Mats

thanks matsp, you ve made some interesting points

On the subject of external declaring, I am aware it and I tried it but I had some problems. Basically what you say is create the table inside the shell.c for instance and just declare it in the header like so extern struct cmd_det function_index[] right?

Anyway I want to hear critisism. I want to see where I went wrong, what I could do different... It's my first time trying to do a flat file system and I know this is a very little part but I want to hear opinions. I will probably ask for advise later on as well on the more serious issues as FAT and directory table and so on

Something else I thought of on the way to work today:

Code:

---

        cmd_line = (char *) malloc( ARG_LENGTH * MAX_ARGS * sizeof(char) );

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

---

Do you really need to allocate this on the stack (that is, are you expecting this to run on a machine with very little stack-space) - otherwise, 128 bytes or 64 bytes aren't really big chunks to put as local variables on the stack, and it saves one level of indirection for the code.

Oh, and don't cast calls to malloc() - it may hide problems.

Could you not use strtok() in your break_cmd_line()? It would save a lot of hassle with looping and skipping over spaces/tabs.

You can probably get rid of this

Code:

---

        //And then if it's followed by new line the exit. Basically the user typed whitespace
        if ( *iter == '\n' )
                return 0;

---

if you do some clever re-arranging of the updating of i.

And I expect you can use strcpy() to copy the string if you use strtok().

--
Mats

Do not remove parameter names

Well I thought of using strtok of course but strtok returns pointers to the start of the tokens right? So that means the first token will have all the line the second all the command line except the first argument and so on. Or am I wrong

As for the malloc allocation I had already changed the cmd_line to an array but when I declare the arguments as char arguments[MAX_ARGS][ARG_LENGTH] I get a warning when I pass it as an argument in break_cmd_line and *(function_index[i].function_to_call) (args,arguments) which both take as second argument a char **. Cant I pass a 2D array as a char ** or is it char *. I know that a 1D array can pass it's name as char * in a function but what about a 2D array

strtok finds the token you tell it to search for and replaces it with '\0', essentially truncating the string.
So if the string is
Hello\nWorld!
And you call strtok to find \n, the first "string" it will return will look like
Hello
And the data in your buffer will look like
Hello\0World

It is destructive, so you do have to be careful. But it works.

Ahh, thanks for clearing it to me. It had seem strange to me if it worked like I thought it did.

True, you still need a list of pointers to each argument, so that you can pass it as char **.

So, you have

Code:

---

char *arguments[MAX_ARGS];
char argstrings[MAX_ARGS][ARG_LENGTH];

---

and then use a loop to assign arguments to argstrings.

However, the other option is to simply use the existing cmd_line string (together with strtok()) - as it places the NUL character at the end of each string.

Your command-line break then becoems:

Code:

---

int split_args(char *cmd_line, char *arguments[], int maxargs)
{
    static const char *seps = " \t\n";
    int argc = 0;
    for(;;)
    {
        char *arg = strtok(cmd_line, seps);
        if (!arg)  // Nothing else in the string. Done!
              return argc;
        arguments[count++] = arg;
    }
}

---

--
Mats

Wow, thanks Mats... I still got a long way to go... :(
Thanks for all the help guys

If I may comment. I think this is correcter (is there such word ! )

Code:

---

int split_cmd(char *cmd_linePtr, char **argumentsPtr)       
{
        static const char *seps = " \t\n";
        int no_arg = 0;
       
        char *arg = strtok(cmd_linePtr, seps);       
        for (;;)
        {               
                if ( !arg ) //Nothing else in the string. Done!
                        return no_arg;
                argumentsPtr[no_arg++] = arg;
                arg = strtok(NULL, seps);       
        }
}

---

I think that subsequent calls need a NULL pointer for the first argument and the saved pointer from before is used

Quote:

---

Originally Posted by Phoenix_Rebirth

If I may comment. I think this is correcter (is there such word ! )

Code:

---

int split_cmd(char *cmd_linePtr, char **argumentsPtr)       
{
        static const char *seps = " \t\n";
        int no_arg = 0;
       
        char *arg = strtok(cmd_linePtr, seps);       
        for (;;)
        {               
                if ( !arg ) //Nothing else in the string. Done!
                        return no_arg;
                argumentsPtr[no_arg++] = arg;
                arg = strtok(NULL, seps);       
        }
}

---

I think that subsequent calls need a NULL pointer for the first argument and the saved pointer from before is used

---

Yes, of course. Much better. Sorry for the bad coding.

But comparing to your oriignal code, this is much simpler, don't you think...

--
Mats

I believe it's called "more correct".
But the design looks okay, except perhaps the problem that the function has no idea how big argumentsPtr is, nor takes any note of its size, causing it to be prone to buffer overflows.

Quote:

---

Originally Posted by Elysia

I believe it's called "more correct".
But the design looks okay, except perhaps the problem that the function has no idea how big argumentsPtr is, nor takes any note of its size, causing it to be prone to buffer overflows.

---

I did actually put a maxargs in the function, but then never got round to adding the check for it. Good point.

--
Mats