Thread: Array of Strings

  1. #1
    Registered User
    Join Date
    Sep 2011
    Posts
    9

    Array of Strings

    Hi everyone.

    I'm really running into a wall here. I must be missing something trivial. Here's my faulty code:
    Code:
    char **argvNew;
    char *token;
    
    token = strtok (cmd, " ");
    	while (token != NULL) { // Copy the arguments into argvNew with the exception of the 0th element
                    	argvNew[i] = token;
    			token = strtok (NULL, " "); 
    			i++;
    }
    Whenever I run it I'm getting a bus error. What am I doing wrong here? Also, it would be optimal if I could make an array of strings with a maximum string length of 128 characters and a maximum number of strings of 8. How can I achieve this?

    Thanks!

  2. #2
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    Turn the warnings up on your compiler, or pay attention to them. Filling in the gaps of defining i and cmd, I get:
    Code:
    $ make tok
    gcc -Wall -g  -lcurl -lpthread -lm  tok.c   -o tok
    tok.c: In function ‘main’:
    tok.c:13: warning: ‘argvNew’ may be used uninitialized in this function
    Which points to the fact that argvNew doesn't contain anything yet. That is, you intend for it to point to a list of char *'s to hold the tokens, but that list of char *'s doesn't exist yet. Try defining argvNew to have a certain amount of char *'s, say 10:
    Code:
    #define MAX_TOKENS    10
    char *argv[MAX_TOKENS];
    ...

  3. #3
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    The bus error is because **argvNew does not have any memory assigned to it.

    If you are using strtok(), you can get away with assigning from *token without copying, because by the standard, strtok() replaces delimiters with '\0', the null terminator. Ie, it breaks the input into separate strings. That means what you need is an array of char pointers:

    Code:
    char *argvNew[8];
    Will work with your existing code, assigning pointers to the beginning of each arg -- but add a check here to be safe:

    Code:
    while (token != NULL && i < 8)
    If instead you want to copy the strings for whatever reason (again, not necessary in this case!), you can declare a 2 dimensional matrix like:
    Code:
    char argvNew[8][128];
    However, you do then have to strcpy() from *token, you cannot use "=".
    Last edited by MK27; 09-07-2011 at 01:08 PM.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  4. #4
    Registered User
    Join Date
    Sep 2011
    Posts
    9
    Thanks for the reply. When I make that change, I still get a bus error when running through GDB.

    EDIT: I got it working through your advice. Thanks a lot; I'm sure I'll be back again
    Last edited by ThePestilent; 09-07-2011 at 01:11 PM.

  5. #5
    Registered User
    Join Date
    Sep 2011
    Posts
    9
    Told you so.

    Here's my full code:
    Code:
    #include "shell2.h"
    
    int main (int argc, char **argv)
    {
    	char *argvNew[8];
    	char *cmd; // Command that is read
    	int pid; // Holds PID after fork
    	int i, j, background;
    	i = 0;
    	int *status; // Used to loop
    	char *token; // Temporarily holds each token
    	struct timeval t1, t2;
    	struct rusage usage;
    
    	while (1) {
    		cmd = readline("$"); 
    		/*if (cmd[127] != '\0') {
    			printf ("Error: Too many characters, try again.\n");
    			goto end;
    		}*/
    		
    		if (strcmp (cmd, "exit") == 0)	// End if exit or EOF is used as input
    			exit(0);
    		else if (cmd[0] == EOF)
    			exit(0);
    
    		if (cmd[strlen(cmd)-1] == '&') {
    			cmd[strlen(cmd)-1] = '\0';
    			background == 1;
    		}
    			
    		token = strtok (cmd, " ");
    		while (token != NULL && i < 8) { // Copy the arguments into argvNew with the exception of the 0th element
    			argvNew[i] = token;
    			token = strtok (NULL, " "); 
    			i++;
    		}
    
    		if (strcmp (argvNew[0], "cd") == 0) {
    			chdir(argvNew[1]);
    			goto end;
    		}
    
    		gettimeofday(&t1, NULL);
    		pid = fork();
    		if (pid < 0) {
    			printf ("Error forking!\n");
    			exit(1);
    		}
    		else if (pid == 0) { // If child, execute the commands
    			int status = execvp (argvNew[0], argvNew);
    			if (status == -1) {
    				printf ("execvp error!\n");
    				exit (1);
    			}
    			exit (0);
    		}
    		else {
    			if (background == 1) 
    				wait3(status, WNOHANG, &usage);
    			else {
    				wait (status); 
    				gettimeofday(&t2, NULL);
    				getrusage(RUSAGE_CHILDREN, &usage);
    				printstats(usage, t1, t2);
    			}
    		}
    end: 
    		cmd = NULL;
    		for (j = 0; j < i; j++)
    			argvNew[j] = NULL;
    		i = j = 0;
    	}
    }
    Every time I run execvp(), I get an error returned. Looking at the argvNew, it appears to have the correct values:
    Code:
    Starting program: /home/devin/Dropbox/My Documents/School/CS 3013/Assignment 1/shell2 shell2
    $ls /usr
    execvp error!
    
    Breakpoint 5, main (argc=2, argv=0x7fffffffe2a8) at shell2.c:50
    50            else if (pid == 0) { // If child, execute the commands
    6: argvNew = {0x615ef0 "ls", 0x615ef3 "/usr", 
      0xc2 <Address 0xc2 out of bounds>, 0x7fffffffe17e "", 0x7fffffffe17f "", 
      0x1 <Address 0x1 out of bounds>, 0x0, 
      0x7ffff7661748 "H\205\300u[L\211\351L\211\342D\211\376\211\357\350\023\376\377\377H\205\300uF\213t$\bL\211\351L\211\342\211\357\350\375\375\377\377H\205\300u0D9t$\fv\tA\203\306\001\353\210\353\001\220\201", <incomplete sequence \355\277>}

  6. #6
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    The last element of the array submitted to execvp() must be a NULL pointer so that it knows when to stop. Try:

    Code:
    char *argvNew[9] = {0};  // assigns zero (NULL) to all elements.
    Then as long as you are sure not to assign more than the first 8 pointers, you will always have a NULL after the last assignment.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  7. #7
    Registered User
    Join Date
    Sep 2011
    Posts
    9
    Thank you for the suggestion. Would it not be correct to use "\0" as null?

  8. #8
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    Quote Originally Posted by ThePestilent View Post
    Thank you for the suggestion. Would it not be correct to use "\0" as null?
    You have to be careful. "\0" is a string literal containing the nul character (actually, it contains two, the first being the one you specified, the second being the implicit one that the compiler puts at the end of all string literals). It has a real, valid address, is not NULL, and is not correct. '\0' (note single quotes) is a character literal for ASCII nul (zero). NULL is a void * to address 0. Sure, the last two both have a zero-ness to them, but it doesn't read as nicely when you go assigning things of one type to another type. Use '\0' for characters and string termination, use NULL for pointers.

    Quote Originally Posted by MK27 View Post
    The last element of the array submitted to execvp() must be a NULL pointer so that it knows when to stop. Try:

    Code:
    char *argvNew[9] = {0};  // assigns zero (NULL) to all elements.
    Then as long as you are sure not to assign more than the first 8 pointers, you will always have a NULL after the last assignment.
    This solution would be fine if the program only ran once. But since it's in a while loop, it's very possible that the first command run has, e.g., 4 command line parameters, then the second has 2 parameters. The second command would not be properly null terminated. You would need to memset argvNew to all zeros after each command is exec'ed. The other option (IMO, the best way), is to explicitly set the pointer after the last parameter to NULL:
    Code:
    token = strtok (cmd, " ");
    while (token != NULL && i < 8) { // Copy the arguments into argvNew with the exception of the 0th element
        argvNew[i] = token;
        token = strtok (NULL, " "); 
        i++;
    }
    argvNew[i] = NULL;  // this is all you have to add

  9. #9
    Registered User
    Join Date
    Sep 2011
    Posts
    9
    Okay that makes sense. Thanks for that.

    While I'm here, I'd appreciate some guidance on the last piece. I'm to fork off each command that's read in, and if the string ends in "&" then it's considered a background process. I'm supposed to make an array of jobs for each background process:
    Code:
    struct process {
    	int pid;
    	int num;
    	char *name;
    }; typedef struct process job;
    typedef struct process *jobPtr;
    How do you suppose I should do this? It can have a fixed size and doesn't need to be as dynamic as a linked list, but each time a job terminates I have to display the statistics for it using my printstats() function.

  10. #10
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    For now, I would implement it with a fixed array. Give yourself a nice set of functions (an API) that you can use to insert, remove, stop, start and print jobs, and use that. If you decide to change to a linked list later, you don't have to change everything, just the code behind the API. Your parent process (the shell) will get a signal from a child (forked process/job) when it terminates. Look into the waitpid and the WNOHANG option.

    It also wouldn't hurt to see a good implementation like Bash's job control. You can read some tutorials on it and even look at the source code, since it's open source. Sure, it's a lot of heavy reading, but would be a great lesson.

  11. #11
    Registered User
    Join Date
    Sep 2011
    Posts
    9
    Alright, and how should I keep the array updated? For example, if the second job in the list finishes then I'll have to bump the other entries up.

    EDIT: Think this would do?

    Code:
    jobPtr *jobarr = NULL;
    
    void addjob (jobPtr j, int numjobs) {
    	jobarr[numjobs] = j;
    }
    
    void removejob (int pid) {
    	int i, j;
    	for (i = 0; i < numjobs; i++) {
    		if (jobarr[i].pid == pid) {
    			for (j = i; jobarr[j] != NULL; j++) 
    				jobarr[j] = jobarr[j+1];
    		}
    	}
    }
    Last edited by ThePestilent; 09-07-2011 at 03:28 PM.

  12. #12
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    That's exactly the type of interface I was expecting. But keep it as small and simple as possible for testing, say 5 or 10 jobs max. Nothing says you have to keep the jobs contiguous and in the lowest possible indexes, especially since the indexes don't have significance to your jobs. Just put them in the first slot you can find and do a lazy delete when you're done. Set pid and/or num to -1 to signify an empty job slot. When you need to find where to add the job, just scan from 0 to the end for the first slot that has a -1 pid/num, and put it in there. Removing just becomes scanning the array for the pid, calling printstats, then setting the pid/num to -1 and freeing the name.

  13. #13
    Registered User
    Join Date
    Sep 2011
    Posts
    9
    Alright, it seems to be working like this:
    Code:
    jobPtr *jobarr = NULL;
    int numjobs = 0;
    
    void addjob (jobPtr j, int numjobs) {
    	jobarr[numjobs] = j;
    	numjobs++;
    	
    }
    
    jobPtr removejob (int pid) {
    	int i, j;
    	jobPtr finished;
    	for (i = 0; i < numjobs; i++) {
    		if (jobarr[i]->pid == pid) {
    			finished = jobarr[i];
    			for (j = i; jobarr[j] != NULL; j++) 
    				jobarr[j] = jobarr[j+1];
    		}
    	}
    	numjobs--;
    	return finished;
    }
    I'm just going to leave it as-is so I don't break anything

    Now, I've never used wait3() before. Is the implementation I have above in post #5 correct? And once something does complete, how am I notified of that and how will I be able to use my printstat() function?

    Thanks again!

    EDIT: Actually, where in that code should I add and remove jobs?
    Last edited by ThePestilent; 09-07-2011 at 04:00 PM.

  14. #14
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    No, you don't use wait3 correctly. You need to pay attention to it's return value (a PID for a child process). Check if that pid is tied to a background job, and if so, remove that job and print stats. You will be notified by way of the status variable you pass to wait3 (note, status should be an int, not int *, and you should pass in &status). You then check the status with one of the macros, like WIFEXITED. Read the man page of waitpid for details and an example on how to use it. You will remove jobs when wait3 notifies you a child has exited and that child was in your job list.

  15. #15
    ATH0 quzah's Avatar
    Join Date
    Oct 2001
    Posts
    14,826
    Quote Originally Posted by ThePestilent View Post
    Actually, where in that code should I add and remove jobs?
    Add it where ever it is that you need to add and remove jobs. If you understand what you have there, then you should know where you need to put the sections for adding and removing. You add when you need a new job, and you remove where you are done with a job. I know, crazy right?


    Quzah.
    Hope is the first step on the road to disappointment.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. array of strings
    By Thegame16 in forum C Programming
    Replies: 1
    Last Post: 11-24-2010, 05:54 PM
  2. Array of Strings
    By bradsulli in forum C++ Programming
    Replies: 4
    Last Post: 09-16-2009, 02:10 PM
  3. Swapping strings in an array of strings
    By dannyzimbabwe in forum C Programming
    Replies: 3
    Last Post: 03-03-2009, 12:28 PM
  4. Replies: 2
    Last Post: 04-27-2008, 03:39 AM
  5. malloc() strings VS array strings
    By Kleid-0 in forum C Programming
    Replies: 5
    Last Post: 01-10-2005, 10:26 PM