fgets prompt in while loop "skips"

I was just testing the CLI for my application, and I noticed that fgets isn't waiting for a response all the time. It's a good bit more complex than this, but here's a patchwork example:

Code:

---

void start(struct outline_interface *this){
        unsigned short length = 31;
        char *command = (char *)NULL, *arguments = (char *)NULL;
       
        char *space;
        unsigned short valid;
       
        while(1){
                printf("Enter a command. > ")
                fgets(command, (int)length, stdin);
                /* [putting everything after the first space out of 'command' and into 'arguments'] */
               
                if(strcmp("make", command) == 0){
                        char response[4];
                        printf("No outline with this name is currently in memory; do you want to load it? (yes, no)");
                        fgets(response, 4, stdin);
                        if(strcmp(response, "yes") == 0)
                                this->load(this, arguments); /* performs no user I/O */
                        printf("These messages may be avoided by using the otherwise identical \"load\" command to create/load outlines.");
                }
                /* [checking for exit conditions] */
        }
}

---

When I run it, I reply to two prompts and then one "skips," taking an empty string as input:

Code:

---

Enter a command. > make Test
No outline with this name is currently in memory; do you want to load it? (no, yes) > yes
These messages may be avoided by using the otherwise identical "load" command to create/load outlines.
[formatted user input]
Enter a command. > Invalid input.  Type "help" for a list of accepted commands.
Enter a command. >

---

What could cause that? Do I need to system("pause")?

Thanks!

Alright, I got the text in my post as it should be...

Check the return value of fgets(). It could be returning NULL, but you are processing the buffer anyways.

Code:

---

fgets(response, 4, stdin);
                if(strcmp(response, "yes") == 0)
                        this->load(this, arguments); /* performs no I/O */

---

Also, keep in mind that this code will leave the '\n' character in the input buffer. That means the next time you call fgets(), you will just receive an empty string with a newline character in it.

fgets() only reads as much as it can, and one reason it stops reading is that the limit you pass as the second argument is reached. So you have this:

Code:

---

fgets(response, 4, stdin);

---

This means fgets() will read at most three characters, with the fourth byte being used only to store the null character. So you type in "yes" and hit enter. fgets() reads the "yes", and since that's the limit, it stores those four characters in "response"; but you hit enter after typing "yes", so there's still a newline that fgets() ignored. The next call to fgets() sees that newline and reads only that (being that fgets() was designed to stop reading when it sees a newline). Thus it appears as though fgets() is being skipped, but it's not; it's just reading the rest of the input.

This is an issue regardless of whether you limit the buffer to 4 bytes or 4000; the user can always type more. The quick and dirty way to fix the problem is to make your response buffer a lot larger and just ignore any junk the user types in (or do with it what you will). That's somewhat of a band-aid, but there's always going to be some issue or other when you're reading input in C.

Incidentally, the casts you are using are not necessary. While they don't break the code, casts are usually used as a way to get around C's type system, and not to reaffirm valid code. After all, you aren't writing:

Code:

---

unsigned short length = (unsigned short)31;

---

and that's really no different than:

Code:

---

char *command = (char *)NULL;

---

31 (an int) can be converted to an unsigned short just as well as NULL can be converted to a char *. No need to try to help C do it.

Quote:

---

Originally Posted by cas

This is an issue regardless of whether you limit the buffer to 4 bytes or 4000; the user can always type more. The quick and dirty way to fix the problem is to make your response buffer a lot larger and just ignore any junk the user types in (or do with it what you will). That's somewhat of a band-aid, but there's always going to be some issue or other when you're reading input in C.

---

You could always, of course, use scanf(), which limits depend on the variable you are using as a buffer, rather than having a pre-destined limit.

Oh... As I was reading I first thought the 4 was inaccurate but looking at my code it's not. Here's the code which the 4 took the place of, with the intent of finding how much I should ask fgets for:

Code:

---

        /* Find the size of the largest option. */
        for(i = 0; i < length; i++)
                size = MAX(size, strlen(items[i]));
        size++;

---

I was thinking I could just make it size += 2, but do I really need to worry about some goof typing "lihsgoiwrbghv"?

Quote:

---

but do I really need to worry about some goof typing "lihsgoiwrbghv"?

---

That's up to you. How robust do you want your application to be?

I apparently need a bit of enlightenment... fgets prevents buffer overflows for me, and I've allowed enough space for all valid input. How is treating nonsense as sensible robust?

EDIT: Oh, and re casting: Looking back at the 3rd C commandment, I find that I remembered it incorrectly.

Quote:

---

How is treating nonsense as sensible robust?

---

That's not what I said or implied. Being able to handle nonsense input without disrupting your application is robust. If your buffer size is the max expected input size, that's fine. But not accounting for the fact that a user can input more characters than this expected size is not robust. If your call to fgets() does not end with a newline character, then you should read from the input buffer until a newline character is found.

Code:

---

while ((ch = getchar()) != '\n' && ch != EOF);

---

Yeah, that was a sloppy question, so I apologize. Preparation isn't exactly treatment, but the point I was trying to get across is that it doesn't matter much if someone abuses a program and sees it hiccup. Or maybe it does and I need enlightenment...

But would the given code expect another character from the user if no newline were still in the buffer?

Thanks for the help.

Quote:

---

Originally Posted by abraham2119

You could always, of course, use scanf(), which limits depend on the variable you are using as a buffer, rather than having a pre-destined limit.

---

I'm not sure I understand your statement, but scanf() is not better than fgets() in this regard. Either you give no size to scanf() and it can overflow, or you give it a size and it acts similar to fgets(). There's no standard C function that stores an unknown amount of data. You can look to something like GNU's getline() (among many others) for "unlimited" data input, but then, you still are limited by memory.

Quote:

---

But would the given code expect another character from the user if no newline were still in the buffer?

---

That's why I said "If your call to fgets() does not end with a newline character..."

Input from the user will always end with a newline character. If your call to fgets() doesn't return a newline character as the last byte (aside from the null terminator), then there is still a newline character in the input buffer.

Alright, I reverted the other bugfix to test this one, but the output is the same as it was at the beginning... Is my condition giving a bad "false" result?

Code:

---

        fgets(input, (int)length, stdin);
        if(strlen(input) == length && input[length - 1] != '\n'){
                while ((ch = getchar()) != '\n' && ch != EOF)
                        /* Clean out the buffer. */;
        }

---

EDIT: Never mind; I needed to subtract 1 from length both times. Thanks again!

Quote:

---

Originally Posted by Jesdisciple

EDIT: Oh, and re casting: Looking back at the 3rd C commandment, I find that I remembered it incorrectly.

---

While a lot of the commandments still apply, they originate from pre-ANSI days (the version here makes that pretty clear); consequently, the third commandment is, for the most part, obsolete.

As it is stated at the above URL, "Thou shalt cast all function arguments to the expected type if they are not of that type already, even when thou art convinced that this is unnecessary, lest they take cruel vengeance upon thee when thou least expect it." This made sense back before we all had ANSI compilers, because C worked like this:

Code:

---

#include <stdio.h>
void f();
int main(void)
{
  f(5);
  f(5LL);
}
void f(v)
unsigned long long v;
{
  printf("%lld\n", v);
}

---

OK, I'm mixing C99 (void return type, long long) and K&R C, which is an abomination, but I'm doing it for the sake of illustration.

Note the declaration for the function f(): it uses an empty set of parentheses, instead of (long long). The empty parentheses mean "unspecified arguments", so C has no idea how the function is set up; that's up to you to get right. Pre-ANSI C had no prototypes.

What happens, then, is that (after something called the default promotions), the values are passed as the types that they are. This means in the f(5) call above, 5 is passed as an int. This is, of course, wrong, because f() takes a long long; but the compiler can't know that. So an int is passed where a long long is expected. With gcc I get 577750908939534341 printed out; with clang I get -5188481073805262843; with klcc I get -4636931200538116091; and with Intel's compiler I get 5. What do you know, sometimes broken code gives the right value.

The second call, f(5LL), is correct (it's the same as doing f( (long long)5 ), but nicer looking). This is because 5LL is a long long, which gets passed as a long long, and so is the proper type.

On the issue of NULL, the commandments linked above say: "Also, contrary to the beliefs common among the more backward inhabitants of the Polluted Eastern Marshes, `NULL' does not have a pointer type, and must be cast to the correct type whenever it is used as a function argument."

The same applies here. NULL in ANSI C is a null pointer constant, and everywhere a pointer is expected, it provides a null pointer. You can pass it to a function expecting a char*, and it will be passed as a char* (assuming you provided a prototype, of course, and there is no reason not to).

Basically, prototypes essentially render the third commandment worthless. Prototypes are great and should always be used.

There is one caveat, though! Some C functions (such as printf() ) can take a variable number of arguments. For such variable arguments, the K&R semantics apply! Therefore, even though you have a prototype for printf(), be careful:

Code:

---

printf("%lld\n", 5); /* BAD because 5 is not a long long */
printf("%lld\n", 5LL); /* OK */
printf("%p\n", NULL); /* BAD because NULL is not necessarily a void* */
printf("%p\n", (void *)NULL); /* OK */

---

Apart from this little quirk, prototypes mean that arguments are properly converted before functions are called.

Quote:

---

Originally Posted by Jesdisciple

Alright, I reverted the other bugfix to test this one, but the output is the same as it was at the beginning... Is my condition giving a bad "false" result?

Code:

---

        fgets(input, (int)length, stdin);
        if(strlen(input) == length && input[length - 1] != '\n'){
                while ((ch = getchar()) != '\n' && ch != EOF)
                        /* Clean out the buffer. */;
        }

---

EDIT: Never mind; I needed to subtract 1 from length both times. Thanks again!

---

There is no reason to check if strlen(input) == length - justcheck if input[strlen(input)-1] != '\n' - it's not really important to only do this when you have strlen(input) == length-1, since the expensive part of the check is to do strlen - so we may just as well simply compare the end of the string with newline. It makes it ONE check, rather than two, making the code simpler.

I personally would probably just set a max line length of some rather large number, say 256 [assuming you do not have some really long input expected, in which case a larger number is appropriate]. If you do that, you can probably ignore checking for newline in the input string [although you probably will, one way or another deal with the fact that there is a newline at the end of the string - however, that is a slightly different matter].

--
Mats

The reason I didn't do that is that the string may be shorter than the char *. For example, say I had a third option "maybe." Then I need to ask fgets for 7 characters, and the response might be "no":
[n][o][\n][\0][?][?][?]
And then the next-to-last character is probably not a newline, and the user is confused that the program is hanging on an empty line...

EDIT: DUH! I'm sorry, you're right and I'm contradicting myself...

EDIT2: ... And I forgot to reply to the rest of your message... I know modern computers aren't hurting for RAM, but I prefer to only ask for what I'll use.