1

I'm coding a shell currently and for some reason I can't get my printenv function to work. When a command is not given, it works. When two arguments are given, it also works. However, when one argument is given it does not work and prints nothing.

The code is as follows:

else if (strcmp(args[0], "printenv")==0){
        /* Previously: if (args[1] == NULL && args[0] != NULL){ */
        if (argc == 1){
            int i = 0;
            while (envp[i] != NULL){
                printf("%s\n", envp[i++]);
            }
        }
        /* Previously: else if (args[2] == NULL && args[1] != NULL){ */
        else if (argc == 2){
            char *env;
            while (args[1] = *argv++){
                env = getenv(args[1]);
                if (env != NULL){
                    printf("%s", env);              
                }           
            }
            free(env);
        }
        else {
            fprintf(stderr, "%s: Too many arguments\n", args[0]);
        }   

    }
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • 2
    BTW, don't free `env` ... you're not suppose to modify the pointer returned from `getenv`. – Jason Oct 01 '12 at 03:02
  • And why are you not using argc to determine the number of args - if I remember correctly there is no requirement to NULL terminate the list – Adrian Cornish Oct 01 '12 at 03:03
  • According to this loop,everything is dependent on the content of args[] which you haven't shown us regarding its population. Further, we have no idea whether you've advanced argv or not prior to entry into this code, but the fact that the sample you provided starts with 'else' makes the mind wonder. Please provide more code, ideally a mock-down that exhibits the problem. – WhozCraig Oct 01 '12 at 03:08
  • I have an argvlist that takes in my commandline and strtoks so that args[0] is the command, args[1] etc are the arguments to the command. –  Oct 01 '12 at 04:05
  • Again, why do you not check then number of args passed? – Adrian Cornish Oct 01 '12 at 04:20
  • I understand that you can check if argc = 1, else if argc = 2, else. You can also do it this way though and it works the same exact way. If you really would like me to change it to argc, I will, but either way works. –  Oct 01 '12 at 04:31

1 Answers1

2
else if (strcmp(args[0], "printenv")==0){
        if (args[1] == NULL && args[0] != NULL){

I stopped reading here. The strcmp call having already determined that *args[0]=='p', why are you comparing args[0] to NULL?


Now, that that's cleared-up. I think you don't need this loop at all:

        while (args[1] = *argv++){
            env = getenv(args[1]);
            if (env != NULL){
                printf("%s", env);              
            }           
        }

Just the loop body:

            env = getenv(args[1]);
            if (env != NULL){
                printf("%s", env);              
            }

And as @Jason comments, you should not free (or otherwise modify) the pointer returned by getenv (nor the data to which it points).

luser droog
  • 18,988
  • 3
  • 53
  • 105
  • That's not in my actual code by I put it on here because apparently the other people above were having trouble understanding my logic of why I was comparing args[1] == NULL and such. The if and else statements both work. I just can't get the else if part to work when an argument is given to printenv. Not sure how to go about it at all. –  Oct 01 '12 at 04:22
  • @Requiem Ok. I can understand that. Then it's this part `while (args[1] = *argv++)` that looks screwy. Having just ensured that it's not NULL, you overwrite it? Is `argv` the normal second parameter to `main`? – luser droog Oct 01 '12 at 04:31
  • Yes, argv is the second parameter. int sh(int argc, char **argv, char **envp) is what I'm working with. I'm not sure what you're meaning by overwriting it though? –  Oct 01 '12 at 04:36
  • Keep in mind that I've only been programming in the unix environment for about 2 weeks now so bear with me, lolz. –  Oct 01 '12 at 04:39
  • When should I free the pointer then? I'm going to have to free the pointer at some pointer right? –  Oct 01 '12 at 04:44
  • No worries. `argv` contains the command line that invoked your shell program itself. There shouldn't be much need to work with it after you've entered your central loop. – luser droog Oct 01 '12 at 04:44
  • No! Don't free it at all. It's *not yours*, you're just *looking at it*. – luser droog Oct 01 '12 at 04:45