fgets loops many times before exiting for EOF

Question

I am making a simple shell. It also needs to be able to read text files by lines. This is my code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/stat.h>

// Exit when called, with messages
void my_exit() {
    printf("Bye!\n");
    exit(0);
}

int main(void) {

  setvbuf(stdout, NULL, _IONBF, 0);

  // Char array to store the input
  char buff[1024];

  // For the fork
  int fid;

  // Get all the environment variables
  char dir[50];
  getcwd(dir,50);
  char *user = getenv("USER");
  char *host = getenv("HOST");

  // Issue the prompt here.
  printf("%s@%s:%s> ", user, host, dir);

  // If not EOF, then do stuff!
  while (fgets(buff, 1024, stdin) != NULL) {

    // Get rid of the new line character at the end
    // We will need more of these for special slash cases
    int i = strlen(buff) - 1;
    if (buff[i] == '\n') {
      buff[i] = 0;
    }

    // If the text says 'exit', then exit
    if (!strcmp(buff,"exit")) {
      my_exit();
    }

    // Start forking!
    fid = fork();

    // If fid == 0, then we have the child!
    if (fid == 0) {

      // To keep track of the number of arguments in the buff
      int nargs = 0;

      // This is a messy function we'll have to change. For now,
      // it just counts the number of spaces in the buff and adds
      // one. So (ls -a -l) = 3. AKA 2 spaces + 1. Really in the
      // end, we should be counting the number of chunks in between
      // the spaces.
      for (int i = 0; buff[i] != '\0'; i++) {
        if (buff[i] == ' ') nargs ++;
      }

      // Allocate the space for an array of pointers to args the
      // size of the number of args, plus one for the NULL pointer.
      char **args = malloc((sizeof(char*)*(nargs + 2)));

      // Set the last element to NULL
      args[nargs+1] = NULL;

      // Split string into tokens by space
      char *temp = strtok (buff," ");

      // Copy each token into the array of args
      for (int i = 0; temp != NULL; i++) {
        args[i] = malloc (strlen(temp) + 1);
        strcpy(args[i], temp);
        temp = strtok (NULL, " ");
      }

      // Run the arguments with execvp
      if (execvp(args[0], args)) {
        my_exit();
      }
    }

    //  If fid !=0 then we still have the parent... Need to
    //  add specific errors.
    else {
        wait(NULL);
    }

    // Issue the prompt again.
    printf("%s@%s:%s> ", user, host, dir);
  }

  // If fgets == NULL, then exit!
  my_exit();
  return 0;
}

When I run it alone as a shell, it works great. When I run ./myshell < commands.txt, it does not work.

commands.txt is:

ls -l -a
pwd
ls

But the output is:

>Bye!
>Bye!
>Bye!
>Bye!
>Bye!
>Bye!>Bye!
>Bye!
>Bye!
>Bye!

Doesn't even run my commands. Any ideas? I thought my while loop was pretty simple.

Try and print the PID of the process in my_exit() to see who's printing what. — Marco Leogrande, Sep 22 '12 at 23:01
You need at least to flush the output after you print the prompt so it comes out in the right place relative to the command output. — Jim Balter, Sep 22 '12 at 23:21
Printing out the PID, I get all 0's (for each Bye!) except the last one, which is 19147 — user1687558, Sep 22 '12 at 23:43
A few comments not related to your problem. If you're not using `argc` and `argv`, just declare `int main(void)`. The casts on the assignments to `user` and `host` are superfluous, and the assignments could be initializations: `char *user = getenv("USER"); char *host = getenv("HOST");` — Keith Thompson, Sep 22 '12 at 23:51
Please do not vandalize your question. Until you get some answers or comments, it is fine to change things; even after you've got some comments, it is OK to make changes. However, once you've started to get answers, you have to be more careful about making changes. It is not fair to those who've expended effort helping you to completely invalidate their answer by expunging 90% of the code that was visible when they were answering. — Jonathan Leffler, Sep 23 '12 at 03:09

score 3 · Answer 1 · answered Sep 22 '12 at 22:55

3

I don't know if this is the problem, but you (correctly) mention in a comment that you have to allocate "plus one for the NULL pointer" in the *args array.

However, you don't actually set the last pointer in *args to NULL.

execvp() won't like that.

That doesn't explain why there might be a difference between redirected vs. non-redirected input, other than undefined behavior is a bastard.

answered Sep 22 '12 at 22:55

Michael Burr

333,147
50
533
760

Thank you - I've set the last pointer to NULL and the strange behavior is still occurring – user1687558 Sep 22 '12 at 23:31

user1687558 · Answer 2 · 2012-09-23T00:32:34.000

1

Sorry everyone - turns out my text file was in some sort of demented format from Mac's TextEdit GUI. Everything is working great.

I really appreciate all of the helpful responses

edited Sep 23 '12 at 00:32

answered Sep 23 '12 at 00:06

user1687558

83
1
7

Can you elaborate on what was demented about the format, and how the dementia manifested itself? – Jonathan Leffler Sep 23 '12 at 03:11
Sure! I saved the TextEdit file, but it was .rtf - so I just renamed it to .txt ... This resulted in strange characters at the top of the file (about the RTF format), followed by the commands. So when the program ran with the .txt input, it was trying to execute completely strange commands. I found this by trying cat commands.txt... The reason I was using TextEdit in the first place is because I've been SSHing into my Unix Server through my Mac @ home. I wanted to create a text file for testing, so I made it in TextEdit and used CyberDuck to SCP it over. – user1687558 Sep 24 '12 at 02:23

fgets loops many times before exiting for EOF

2 Answers2