LinuxQuestions.org

LinuxQuestions.org (/questions/)
-   Programming (https://www.linuxquestions.org/questions/programming-9/)
-   -   Problem with C program (https://www.linuxquestions.org/questions/programming-9/problem-with-c-program-273320/)

exvor 01-03-2005 11:40 AM
Problem with C program
 
This is a problem im doing for the ctutorial questions. It asked me to write a program that takes input from the user and removes all the spaces and replaces them with newlines. I did this to solve the problem but im getting one issue.

It displayes the error "Error Reading file for input" this is the error i told it to put if the open command return with a null.

problem is with the this part "temp = fopen ("temp.str","w");" in theory acording to my tutorial this should find the file if it exists remove its contents and open it for writeing.

problem is thats not what is occuring. its just apending the file not removing the current contents.

even if the file does not exist i still get the message. mentiond above when trying to read the file.

all and all the program works correctly ?

so do i just need to abandon my quest with high level file routines and go low level or am i doing something wrong ?

here is the code

Code:
/* program takes input from user and strips */
/* All of the spaces out and replaces them with newlines */


#include <stdio.h>

char *typed;
int errchk;
int SIZE = 100;

void gettyped(char*);
void striprint(char*);
 


int main()
{
    typed = (char *) malloc (SIZE);
   
    gettyped(typed);
    striprint(typed);
   
   
    return 0;
}

void gettyped(char *typed)
{
    /* This function gets values from the user */
   
    errchk = getline(&typed,&SIZE,stdin);
        if (errchk == -1)
            {
                puts("Invalid input.");
            }
}

void striprint(char *typed)
{
    FILE *temp;
    char *typedread;
    int err2chk;
    typedread = (char*) malloc(100);
   
  /* Opens a file for tempoary holding space */     
   
    temp = fopen ("temp.str","w");
    if (temp == NULL)
        {
            puts ("Error opening temporary file space");
        }
    fprintf(temp,typed);
    errchk = 0;
   
    errchk = fclose (temp); /* This action closes the file to write it */
    if (errchk == -1)
        {
            puts ("Failed to close file");
        }
   
    /* we need to reopen the file to parse it for the strings */
   
    temp = fopen ("temp.str","r");
      if (temp == NULL);
        {
            puts("Error Reading file for input");
        }
    /* file should be opened at this point */
    /* I probably should have made this a seperate function and may */
    /* do that later if i find it eazier to maintain */
    err2chk = 0;
    /* The below code does the actual printing and adding a new line */
    do
    {
   
    err2chk = fscanf (temp,"%s",typedread);
    if (err2chk != -1)
        {
        printf("%s\n",typedread);
        }
    }
 while (err2chk != -1);
 errchk = fclose (temp);
 if (errchk == -1)
  {
    puts("error closeing file");
    }
}

itsme86 01-03-2005 12:52 PM
What's the getline() function?

Hko 01-03-2005 12:56 PM
Quote:
It displayes the error "Error Reading file for input" this is the error i told it to put if the open command return with a null.
Yes, it will allways print that error, even when everything is OK. This s because you need to remove the ; after the "if"-statement (see commented code below)

Quote:
problem is with the this part "temp = fopen ("temp.str","w");" in theory acording to my tutorial this should find the file if it exists remove its contents and open it for writeing.
This works OK on my computer. Try fixing the other mistakes (see comments in the code below, and then try again.

But why are you writing the input to a file and then read it back in? Once the input from the user is read into the buffer (the char *typed is the buffer) you can parse it for spaces from that buffer! Why first writing it to a file and then reading it into another buffer before parsing it?

The getline() function is an advanced version of fgets(), and does some memory-fiddling if the buffer is not large enough to hold the data read. You are not using or checking for this, and I think you don't understand what its memory-fiddling feature is about (you have a mistake in your code caused by this, see comment below). Also getline() is a GNU-only function (it's not a commonly used function), which means your program will not compiled and run correctly on UNIX-like OS's that have not the GNU libc installed. In other words: it's not portable.

I'd recommend fgets() instead of getline(), unless you have special reasons (which you don't in this case).

One more thing:
When you compile with "-Wall" you get all compiler warnings. This will give you more clues about mistakes.

Code:
/*
 * program takes input from user and strips
 * All of the spaces out and replaces them with newlines
 */

/* #define added for getline() (see it man page)
 * Must go before #include <stdio.h>
 */
#define _GNU_SOURCE

#include <stdio.h>

/* Header added for malloc() and free() */
#include <stdlib.h>

char *typed;
int errchk;
int SIZE = 100;

void gettyped(char *);
void striprint(char *);

int main()
{
    typed = (char *) malloc(SIZE);

    gettyped(typed);
    striprint(typed);

    /* Make a good habit of freeing malloc()ed memory.
    * While it does not matter really in this program,
    * not freeing malloc() memory is a popular way of
    * introducing bugs, especially memory-leaks..
    */
    free(typed);

    return 0;
}

void gettyped(char *typed)
{
    /* This function gets values from the user  */

    /* It would have been better if you declared SIZE of type "size_t"
    * instead of int if you want it to use it here in getline().
    * If you compile with gcc's --Wall option ("Warnings-all") you get
    * a warning about this.
    *
    * Better though to use fgets() instead of getline(). You can then leave
    * it an int.
    */
    errchk = getline(&typed,  &SIZE, stdin);

    if (errchk == -1) {
        puts("Invalid input.");
    }
}

void striprint(char *typed)
{
    FILE *temp;
    char *typedread;
    int err2chk;

    typedread = (char *) malloc(100); /* Better use SIZE here instead of 100 */

    /* Opens a file for tempoary holding space */

    temp = fopen("temp.str", "w");
    if (temp == NULL) {
        puts("Error opening temporary file space");
    }
    fprintf(temp, typed);
    errchk = 0;

    errchk = fclose(temp);  /* This action closes the file to write it */
    if (errchk == -1) {
        puts("Failed to close file");
    }

    /* we need to reopen the file to parse it for the strings */
    /* You could have opened it for reading AND writing instead, and "rewind"
    * it here to start reading from the start by calling rewind() or fseek().
    * (see the man pages of those functions for more info)
    */

    temp = fopen("temp.str", "r");
    if (temp == NULL) ; /* <-- Remove the ';' here or the "if" won't work, so
                              error will print allways without reason! */
    {
        puts("Error Reading file for input");
    }
    /* file should be opened at this point  */
    /* I probably should have made this a seperate function and may */
    /* do that later if i find it eazier to maintain */
    err2chk = 0;
    /* The below code does the actual printing and adding a new line */
    do {

        err2chk = fscanf(temp, "%s", typedread);
        if (err2chk != -1) {
            printf("%s\n", typedread);
        }
    } while (err2chk != -1);
    errchk = fclose(temp);
    if (errchk == -1) {
        puts("error closeing file");
    }
    free(typedread); /* Again, a good habit */
}

exvor 01-03-2005 01:13 PM
Yea seams to me im not totaly grasping the whole sizeof thing yet. Im just learning c and this is the first problem that was semi advanced that i have tried to tackle. The tutorial praises getline that is why i tried to use it instead of other methods. It doesent even mention fgets.


as for why i did not use the buffer im still a little in the fog on buffers. And file postion. I figured i would need to make a file because im using file postion to determine where the next word typed is.

Is there a better way to replace a character in a string of undetermined length rather then the model that i have created ??

Hivemind 01-03-2005 01:27 PM
And remove the casts of malloc()'s return value:

Code:
typed = (char *) malloc (SIZE);
should be
Code:
typed =  malloc (SIZE);
The code compiles without the cast as long as you have the proper header (stdlib.h) included. If you don't have stdlib.h included (or a header that includes it), a compilation error is generated. If you cast what malloc() returns an error indicating that the proper header hasn't been included won't appear and you're left with a compilable, but non-working program.

I wonder why people insist on casting...must be alot of crappy tutorials out there. I know you have to cast in C++ but in C++ you should use new/delete because malloc/free doesn't understand constructors.

exvor 01-03-2005 01:35 PM
Yea stupid tutorial never even explained what the (char * ) even was just that it needed to be in front of malloc.


I dident see why i needed to put it there either ???

exvor 01-03-2005 01:37 PM
The header and the ; in front of the if statement was my own bad :P and i think i tracked my less on the file problem down. Its less thats messed up not my data file that was created :P


STUPID LESS BUFFER !!

Hko 01-03-2005 01:51 PM
Quote:
Originally posted by exvor
The tutorial praises getline that is why i tried to use it instead of other methods. It doesent even mention fgets.
OK. I had never heard of getline() before. Now that I read its man page, I realize it can be really useful. May be better to stick with it during the tutorial. But realize it can also be very tricky: Your program has a hard to understand-find-and-solve bug only because you use getline(). (this bug will only appear when the user type more than 100 characters). It's too far fetched to try explain this here, while you're still "in the fog" about buffers. To solve this, just remove the gettyped() function and put everyting that was in gettyped() into main(). Like this:
Code:
int main()
{
    typed = (char *) malloc(SIZE);
    errchk = getline(&typed, &SIZE, stdin);
    if (errchk == -1) {
        puts("Invalid input.");
    }
    striprint(typed);
    free(typed);
    return 0;
}

void striprint(char *typed)
{
  /* ..... left out code ......*/
}
And important in this case: use SIZE instead of 100 everywhere the buffer size is needed.

Quote:
I figured i would need to make a file because im using file postion to determine where the next word typed is.
Better use an variable (int) as an index to the buffer-chars for that. Reading files is slow compared to memory reading (not to mention first writing the file)

Quote:
Is there a better way to replace a character in a string of undetermined length rather then the model that i have created ??
Yes. By using the buffer. Much easier, faster & shorter. You can replace your striprint() function with just this:
Code:
void striprint(char *typed)
{
    int i;

    for (i = 0; i < SIZE; ++i) {
        if (typed[i] == ' ') {
            typed[i] = '\n';
        }
    }
    printf(typed);
}
You said: "...in a string of undetermined length". This is where getline() comes in really handy. When you really have no idea how long the input will be, and you don't want to put a limit on it, getline() can save a lot of troubles and opportunities to put bugs in a program. On the other hand getline() opens up a few bug-opportunities itself, but this is definately more managable if you understand how it works. BTW Note that this getline stuff only applies to C. Other, higher level, languages (java, python, perl, php...) have easier solutions. AFAIK even C++-strings make this GNU-getline() a non-option (when using C++ that is).

jlliagre 01-03-2005 02:46 PM
Quote:
I wonder why people insist on casting...must be alot of crappy tutorials out there.
Hmm, or perhaps some crappy comments ...

"Man malloc" or "grep malloc /usr/include/stdlib.h" will show you that malloc returns a void*, so it must be casted to the destination pointer type, unless it is a void*.
Not doing it will trigger a C compiler warning.

itsme86 01-03-2005 03:00 PM
Quote:
Originally posted by jlliagre
Hmm, or perhaps some crappy comments ...

"Man malloc" or "grep malloc /usr/include/stdlib.h" will show you that malloc returns a void*, so it must be casted to the destination pointer type, unless it is a void*.
Not doing it will trigger a C compiler warning.
That applies to C++ only, not C. A void * pointer can be assigned to any other type of pointer type without the need for a cast. You can see it in action here:
Code:
itsme@dreams:~/C$ cat nowarn.c
#include <stdio.h>

void *func(void)
{
  void *ptr;

  return ptr;
}

int main(void)
{
  int *num;

  num = func();

  return 0;
}
itsme@dreams:~/C$ gcc -Wall -ansi -pedantic nowarn.c -o nowarn
itsme@dreams:~/C$

exvor 01-03-2005 03:06 PM
LOL this is so much eazier

Code:
void striprint(char *typed)
{
    int i;

    for (i = 0; i < SIZE; ++i) {
        if (typed[i] == ' ') {
            typed[i] = '\n';
        }
    }
    printf(typed);
}

silly me forgetting strings are arrays :tisk:

itsme86 01-03-2005 03:14 PM
Or you could just do it on-the-fly while printing the output:
Code:
{
  int i;

  for(i = 0;typed[i];++i)
    putchar(typed[i] == ' ' ? '\n' : typed[i]);
}

exvor 01-03-2005 03:40 PM
Actually theres a problem with doing this way. What happens is because the size of the buffer is 100 it asumes the string is 100 so it loops untill it gets to 100 rather then stopping at the end of the input. Maybe this is why i dident do it this way?

exvor 01-03-2005 03:46 PM
upon setting malloc to 1 i get some very strange results.

exvor 01-03-2005 03:54 PM
never mind i wasent minding wehre the } are



it seams to work ok but i wonder if you make malloc too big would it continue to loop untill it got to 100

exvor 01-03-2005 03:58 PM
Never mind forgot getline sets the EOF at the end of the input no matter what SIZE is set to so even if it does loop 100 times printf will still only print out the data contained in the string.
Wow answering my own questions now

Hko 01-03-2005 04:41 PM
Quote:
Originally posted by exvor
Actually theres a problem with doing this way. What happens is because the size of the buffer is 100 it asumes the string is 100 so it loops untill it gets to 100 rather then stopping at the end of the input. Maybe this is why i dident do it this way?
Hmm,.. that's what'swrong with my code...
itsme86's code handles this right.

So a "patched" version of mine then:
Code:
void striprint(char *typed)
{
    int i;

    for (i = 0; typed[i]; ++i) {
        if (typed[i] == ' ') {
            typed[i] = '\n';
        }
    }
    printf(typed);
}

itsme86 01-03-2005 05:02 PM
This is the way I'd do it:
Code:
#include <stdio.h>

int main(void)
{
  char typed[100];
  int i;

  fgets(typed, sizeof(typed), stdin);
  for(i = 0;typed[i];++i)
    putchar(typed[i] == ' ' ? '\n' : typed[i]);

  return 0;
}
I don't recommend using Hko's method of using the string that the user typed as the format string for printf(). It's too big of a vulnerability if someone knows what they're doing. For instance, look at the difference between ours. nl uses mine and nlbad uses Hko's (I type the %d%s%s%d on the first line for each program. The next line is the result):
Code:
itsme@dreams:~/C$ ./nl
%d%s%s%d
%d%s%s%d
Code:
itsme@dreams:~/C$ ./nlbad
%d%s%s%d
3fgets(null)-1073743656
A fix would be to replace printf(typed); with printf("%s", typed);

exvor 01-03-2005 05:13 PM
As i mentioned before nothig was wrong with it i just wasent thinking clearly :)


Also checked the Tutorial on fgets and it was there it was listed under a depreciated function.

Hko 01-03-2005 05:15 PM
Oops,..
I stand corrected.

Hko 01-03-2005 05:17 PM
Quote:
Originally posted by exvor
Also checked the Tutorial on fgets and it was there it was listed under a depreciated function.
fgets() deprecated? Or are you purhaps talking about gets()? (without 'f').
gets() is wrong. fgets() is OK.

exvor 01-03-2005 05:18 PM
TRY a null character on this program of yours and see if it breaks fgets supposedly this is why fgets is not supposed to be used anymore.

itsme86 01-03-2005 05:24 PM
Quote:
Originally posted by exvor
TRY a null character on this program of yours and see if it breaks fgets supposedly this is why fgets is not supposed to be used anymore.
What do you mean "try a null character"? What is supposed to be the replacement for fgets()? Can you cite the source of this information that it's deprecated?

EDIT: After some research, it seems that only crasseux.com seems to dog on fgets. And yeah, fgets() will store a null byte in the string if the user enters one somehow, but I don't see how that in any way can be dangerous. It will just cut the string off short. fgets() will work with any compiler and getline() won't, since it's not a standard function.

There's not really any problem with fgets() anyway. In fact, an argument can be made that getline() is the buggy function since it's not accurately reading input. It fails to copy null bytes from the input and fgets() doesn't have that problem. Anyway, I don't know who these crasseux.com people are, but I wouldn't listen to them too much.

Hko 01-03-2005 05:44 PM
Tried it.
Doesn't break.

itsme86 01-03-2005 05:50 PM
Okay, look at this:
Code:
itsme@dreams:~/C$ cat badfgets.c
#include <stdio.h>

int main(void)
{
  char buf[4096] = { 'a', 'b', 'c', '\0', 'd', 'e', 'f', '\n', '\0' };
  FILE *fp;

  fp = fopen("slimy.txt", "w");
  fputs(buf, fp);
  fclose(fp);

  fp = fopen("slimy.txt", "r");
  fgets(buf, sizeof(buf), fp);
  fclose(fp);

  puts(buf);
  return 0;
}
When I run it I get:
Code:
itsme@dreams:~/C$ ./badfgets
abc
itsme@dreams:~/C$
So you start to think that fgets() is broken, right? Wrong! Look at the contents of the file:
Code:
itsme@dreams:~/C$ cat slimy.txt
abcitsme@dreams:~/C$
So you can see that even fputs() didn't dump the whole thing. The string looked like it ended at the first null byte to fputs(). But that's just my point. There's nothing wrong with fgets(). That's the way strings were designed to work.

Hko 01-03-2005 06:10 PM
..and getline() won't do any different.

exvor 01-04-2005 10:20 AM
This is getting fustrating with c programming tutorials. Why does everyone has a diffrent idea on what is right and wrong. Not that im angry at you im not i greatly apreciate your help its these tutorials everyone has THERE way of learning. This is the the best one i can find thats why im trying to learn it. Why cant the authors stick to facts and keep there option out of a so called traning manual.


By the way if i can NOT use getline anywhere I would be estatic not to. It does seam buggy and harder to do some simple things.


thanks guys

Hko 01-04-2005 11:51 AM
C is not an easy programming language, it's a low-level programming language. Arguably the second lowest-level one (assembly being the lowest).

In C you need to take care of a lot of memory-handling/fiddling yourself. For non-trivial program this can become difficult and is the most common cause of bugs in even quite mature C-programs (buffer overflow security holes, memory leaks,..). Also these kind of bugs are known for being hard to track down.

Even in your simple program this problem with memory-handling in C (buffer malloc's and such) appears. It can be done easier, but then there is a limit to the buffer size. And you need to make sure that the buffer will not be written to past the end of the buffer (same for reading, but that is not as harmful as writing), no matter what weird input the user feeds the program.

You can make the buffer fixed-size and use fgets(). But what if the buffer is fixed at 100 bytes, and the user enters 2000? To be on the safe side, you may think of making the buffer 10000 bytes big. But then again, what if the user enters 15000 characters? Well you can make a HUGE buffer of 10 Mb. But now your program uses quite a lot of memory, while your user may only enter 10 characters. For your simple program this is not a real problem, but the same problem applies to software like web/ftp/mailservers, browser, etc. which need many more buffers of different sizes. Just making all buffers huge, in order to be able to store all expected data, is not the solution, because you'd need a lot of memory, just to run apache. If there is enough memory just run apache, then you may have a problem running another program with huge buffers at the same time.

Getline() is a solution to this (for a small part). It enables the buffer to grow automatically if the data does not fit. This seams handy for newbies to C: If used in the right way you can read huge and small user-inputs without thinking about the memory buffer stuff. But it also can introduce problems when not used in the right way. These problems will be more difficult to understand for a newbie.

I guess authors of tutorials (and other people) have to deal with the memory-issue of C almost from the start. So they think of solutions, or ways to get around it in teaching C to newbie's, like using getline() for example. It wouldn't be my choice, but I'm not a teacher. It's a matter of taste, I suppose.

If you do not want to deal with this memory thing of C, you can choose a different (high-level) language, like Java, Python, PHP, Perl. All of these do not have this issue with memory-buffers. Their interpreters/compiler include code to handle all that, so the programmer is not bothered with that.

If you do want to learn C, just continue to learn/try. In the end, a low-level language like C will hlp you understand better how computers work under the hood.

Quote:
By the way if i can NOT use getline anywhere I would be estatic not to. It does seam buggy and harder to do some simple things.
I suppose it's not buggy by itself. It can even prevent certain mistakes, but it introduces other ways to make mistakes.

Here's a version of you program that uses fgets() with a fixed buffer. Just change the "100" in the #define to change the buffer size, and recompile.
Code:
#include <stdio.h>

#define SIZE 100

int main()
{
    int  i;
    char typed[SIZE];

    if (fgets(typed, SIZE, stdin) == NULL) {
        perror("Reading input");
        return 1;
    }

    for (i = 0; typed[i] != '\0'; ++i) {
        if (typed[i] == ' ') {
            putchar('\n');
        } else {
            putchar(typed[i]);
        }
    }

    return 0;
}


All times are GMT -5. The time now is 05:25 AM.