1

I've been struggling on this problem all day now, and looking at similar examples hasn't gotten me too far, so I'm hoping you can help! I'm working on the programming assignment 1 at the end of CH 3 of Operating Systems Concepts if anyone wanted to know the context.

So the problem is to essentially create a command prompt in c that allows users to input a command, fork and execute it, and save the command in history. The user can enter the command 'history' to see the 10 most recent commands printed out. The book instructed me to store the current command as a char pointer array of arguments, and I would execute the current one using execvp(args[0], args). My professor added other requirements to this, so having each argument individually accessible like this will be useful for those parts as well.

I decided to store the history of commands in a similar fashion using an array of char pointers. So for example if the first command was ls -la and the second command entered was cd.. we would have history[0] = "ls -la" and history[1] = "cd..". I'm really struggling getting this to work, and I'm fairly certain I'm screwing up pointers somewhere, but I just can't figure it out.

In main I can print the first word in the first command (so just ls for ls -la) using arg_history[0] but really can't figure out printing the whole thing. But I know the data's there and I verify it when I add it in (via add_history function) and it's correct! Even worse when I pass it to the get_history function made for printing the history, it prints a bunch of gibberish. I would greatly appreciate any help in understanding why it's doing this! Right now I have a hunch it's something to do with passing pointers incorrectly between functions, but based on what I've been looking at I can't spot the problem!

/**
 * Simple shell interface program.
 *
 * Operating System Concepts - Ninth Edition
 * Copyright John Wiley & Sons - 2013
 */

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>



#define MAX_LINE        80 /* 80 chars per line, per command */
#define HIST_LENGTH     10

void get_input(char *args[], int *num_args, char *history[], int *hist_index);
void get_history(char *history[], int hist_index);
void add_history(char *history[], char *added_command, int *hist_index);

int main(void)
{
    char *args[MAX_LINE/2 + 1]; /* command line (of 80) has max of 40 arguments */
    char *arg_history[HIST_LENGTH];

    int num_args;
    int hist_index;

    int should_run = 1;
    int i;

    while (should_run){   
        printf("osh>");
        fflush(stdout);

        get_input(args, &num_args, arg_history, &hist_index);

        //printf("%s\n", arg_history[0]); //incorrectly prints up to the first space
        //printf("%s\n", args[0]) //prints the correct arg from the last command (eg. for 'ls -la' it prints ls for args[0] and -la for args[1])

        if (strcmp(args[0], "history") == 0) {
            get_history(arg_history, hist_index);
        }        
    }

    return 0;
}

void get_input(char *args[], int *num_args, char *history[], int *hist_index) {
    char input[MAX_LINE];
    char *arg;

    fgets(input, MAX_LINE, stdin);
    input[strlen(input) - 1] = NULL; // To remove new line character - the compiler doesn't like how I'm doing this

    add_history(history, input, hist_index);

    arg = strtok(input, " ");

    *num_args = 0;
    while(arg != NULL) {
        args[*num_args] = arg;
        *num_args = *num_args + 1;
        arg = strtok(NULL, " ");
    }
}

void get_history(char *history[], int hist_index) {
    int i;
    for (i = 0; i < HIST_LENGTH; i++) {
        printf("%d %s\n",  hist_index, *history);
        // prints gibberish
        hist_index = hist_index - 1;
        if (hist_index < 1) {
            break;
        }
    }
}

void add_history(char *history[], char *added_command, int *hist_index) {
    int i;
    for (i = HIST_LENGTH-1; i > 0; i--) {
            history[i] = history[i-1];
    }

    history[0] = added_command;
    *hist_index = *hist_index + 1;
    //printf("%s\n", history[0]); prints correctly
}

Update: I made the changes suggested by some of the solutions including moving the pointer to input out of the function (I put it in main) and using strcpy for the add_history function. The reason I was having an issue using this earlier was because I'm rotating the items 'up' through the array, but I was accessing uninitialized locations before history was full with all 10 elements. While I was now able to print the arg_history[0] from main, I was still having problems printing anything else (eg. arg_history[1]). But more importantly, I couldn't print from the get_historyfunction which is what I actually needed to solve. After closer inspection I realized hist_index is never given a value before it's used to access the array. Thanks for the help everyone.

Improve this question
2
  • i guess strtok(input, " ") broke your input line. you may try to print out input before strtok() and after strtok() to see what you got. Commented Feb 3, 2017 at 3:15
  • After calling get_input if you print out args[] in main it functions correctly. The issue is accessing the data in arg_history Commented Feb 3, 2017 at 4:00

2 Answers 2

Reset to default
3 
input[strlen(input) - 1] = NULL; // To remove new line character - the compiler doesn't like how I'm doing this

Of course it doesn't. There are many things wrong with this. Imagine if strlen(input) is 0, for example, then strlen(input) - 1 is -1, and you're accessing the -1th item of the array... not to mention NULL is a pointer, not a character value. You probably meant input[strlen(input) - 1] = '\0';, but a safer solution would be:

input[strcspn(input, "\n")] = '\0';

history[0] = added_command;
*hist_index = *hist_index + 1;
//printf("%s\n", history[0]); prints correctly

This prints correctly because the pointer value added_command, which you assign to history[0] and which points into input in get_command is still alive. Once get_command returns, the object that pointer points to no longer exists, and so the history[0] pointer also doesn't exist.

You should know you need to use strcpy to assign strings by now, if you're reading a book (such as K&R2E). Before you do that, you need to create a new object of suitable size (e.g. using malloc)...

This is a common problem for people who aren't reading a book... Which book are you reading?

    printf("%d %s\n",  hist_index, *history);
    // prints gibberish

Well, yes, it prints gibberish because the object that *history once pointed to, before get_command returned, was destroyed when get_command returned. A book would teach you this.

See also Returning an array using C for a similar explanation...

Improve this answer
Sign up to request clarification or add additional context in comments.

9 Comments

Thanks for the reply! That first section was very useful, I need to learn to read my compiler better. I knew I was something along those lines but I didn't quite understand. For the second half I'm following your train of thought, but I'm a bit skeptical that once get_command returns the pointer is gone, because the print statement following where it returns prints the entire string up to the space. If there's no space it'll print the entirety of which ever command in history I point it to. If there is a space it'll point up to that.
Perhaps you could ask another question about the warning message, such as "What does ... mean?" or "How do I decipher ...?", filling in the blanks with a part you don't understand...
Regarding your skepticism, consider that the pointer points into stack space... until you call another function (to reuse the stack space), that stack space won't be overwritten. Once the stack space is overwritten, this is where the garbage comes from. This is technically undefined behaviour, something to be avoided.
I'm going to move the input pointer out of the function and into main, but I'm still not sure you're 100% correct. And there's no need to be condescending, in an earlier unsuccessful version I was using srtcpy, but as I said I've been working on this all day so it's gone though some changes. Thank you for pointing that out though. I'm reading the book I mentioned in the original post, and have read other C books in the past, but I am very rusty.
Well regarding the error you cleared it up for me. I didn't realize NULL was a pointer, which now makes sense what the compiler meant referring to it as void *. You helped me and cleared it up; I don't see what the point of helping is if you're going to be rude about it.
|
0

Here are some description of strtok(). Because you just put the pointer of input to your history list instead of putting a copy, you'd only print out the first word.

char *strtok(char *str, const char *delim)
Parameters
str -- The contents of this string are modified and broken into smaller strings (tokens).

delim -- This is the C string containing the delimiters. These may vary from one call to another.
Improve this answer

2 Comments

Thanks for the help! I agree I need to copy the copy a new pointer instead of the one directly from input, but I'm confused how strtok impacts this? I only use strtok on arg[] after it's already put in the history list.
@PhilBaldoni you need to copy the whole text of input, not just the pointer itself. what you put in history is just the address of the starting point of the input text. but you passed the same address to strtok which broke up the text at that address (namely, putting '\0' at spaces between words), so when you tried print out the text at the saved address, you just got the first word. one way to solve the issue, is to add char copy[MAX_LINE]; then do strcpy(copy, input); after you got the input, then do arg = strtok(copy, " "); instead of arg = strtok(input, " ");

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.