Just Want an opinion

This is a discussion on Just Want an opinion within the C Programming forums, part of the General Programming Boards category; So I was trying to make a very primitive kind-of-shell and I ended ip doing the follow. I just want ...

  1. #1
    Registered User Phoenix_Rebirth's Avatar
    Join Date
    Dec 2005
    Location
    Cyprus
    Posts
    68
    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
    Attached Files Attached Files
    Last edited by Phoenix_Rebirth; 01-29-2009 at 06:40 AM.

  2. #2
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  3. #3
    Registered User Phoenix_Rebirth's Avatar
    Join Date
    Dec 2005
    Location
    Cyprus
    Posts
    68
    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
    Last edited by Phoenix_Rebirth; 01-28-2009 at 07:27 PM.

  4. #4
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  5. #5
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    22,985
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  6. #6
    Registered User Phoenix_Rebirth's Avatar
    Join Date
    Dec 2005
    Location
    Cyprus
    Posts
    68
    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
    Last edited by Phoenix_Rebirth; 01-29-2009 at 06:17 AM.

  7. #7
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    22,985
    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.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  8. #8
    Registered User Phoenix_Rebirth's Avatar
    Join Date
    Dec 2005
    Location
    Cyprus
    Posts
    68
    Ahh, thanks for clearing it to me. It had seem strange to me if it worked like I thought it did.

  9. #9
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  10. #10
    Registered User Phoenix_Rebirth's Avatar
    Join Date
    Dec 2005
    Location
    Cyprus
    Posts
    68
    Wow, thanks Mats... I still got a long way to go...
    Thanks for all the help guys

  11. #11
    Registered User Phoenix_Rebirth's Avatar
    Join Date
    Dec 2005
    Location
    Cyprus
    Posts
    68
    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

  12. #12
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by Phoenix_Rebirth View Post
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  13. #13
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    22,985
    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 Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  14. #14
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by Elysia View Post
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 6
    Last Post: 02-27-2009, 04:43 PM
  2. Her opinion, your opinion
    By RoD in forum C++ Programming
    Replies: 4
    Last Post: 12-22-2002, 10:50 AM
  3. Freedom of opinion
    By Shiro in forum A Brief History of Cprogramming.com
    Replies: 1
    Last Post: 02-10-2002, 07:06 AM
  4. opinion about books
    By clement in forum C Programming
    Replies: 7
    Last Post: 09-24-2001, 05:18 PM
  5. your opinion please
    By iain in forum A Brief History of Cprogramming.com
    Replies: 3
    Last Post: 09-10-2001, 06:36 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21