Basic program design, passing pointer to a function

Hi,
The programs below try to read a file and print its contents to a block of 72 chars wide. The point of the programs is for me to learn not to cram everything into main(). What I'd like it to do is the following in pseudo code:

Code:

/*
func1()
  open text file

func2()
  make '\0' terminated line
  return pointer of line to main

main()
  func1()
  loop func2 until EOF
 */

The first kludge I came up with is this:

Code:

// readfile1.c

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

static FILE *open_file(char *file, char *mode)
{
	FILE *fp = fopen(file, mode);
	if (fp == NULL) {
		perror("fopen: ");
		exit(EXIT_FAILURE);
	}
	return fp;
}

char *read_line(int line_length, FILE *txt)
{
	int i, character;
	char *line;
	line = (char *)malloc(line_length);
	FILE *text;

	for (i = 0; i < (line_length - 1); i++) {
		if ((character = fgetc(text)) == EOF) {
			perror("fgetc: ");
			exit(EXIT_SUCCESS);
		}
		line[i] = character;
	}
	line[line_length - 1] = '\0';

	return(line);
}

int main(int argc, char *argv[])
{
	int length = 73;
	FILE *open;
	open = open_file(argv[1], "r");

	int i;
	// This test loop is a place holder for a loop_until_EOF.
	// It segfaults at i = 1.
	for (i = 0; i < 4; i++) printf("%s\n", read_line(length, open));

	return(EXIT_SUCCESS);
}

Code:

heras@vhbh:~/code/c$ ./read1 ~/code/text

Open text file for reading. The stream is positioned at the beginning
Segmentation fault (core dumped)

The malloc in the readline() funtion is probably a bad idea because it would require a seperate line = (char *)malloc(line_length); buffer for each line that is read from the source file. And to free() them (I'm not at that point yet) might be cumbersome. Is any of this true?
Then I tried to move the malloc to main() and pass line_pointer to read_line() hoping read_line() might write to it. This is not working at all:

Code:

//readfile2.c

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

static FILE *open_file(char *, char *);
char *read_line(int, FILE *, char *);

int main(int argc, char *argv[])
{
	FILE *open;
	open = open_file(argv[1], "r");

	int i;
	int length = 73;
	char *line_pointer;
	line_pointer = (char *)malloc(length);
	for (i = 0; i < 4; i++) {
		printf("%s\n", read_line(length, open, line_pointer));
	}
	return(EXIT_SUCCESS);
}

static FILE *open_file(char *file, char *mode)
{
	FILE *fp = fopen(file, mode);
	if (fp == NULL) {
		perror("fopen");
		exit(EXIT_FAILURE);
	}
	return(fp);
}

char *read_line(int line_length, FILE *txt, char *lp)
{
	int i;
	char *character = lp;
	FILE *text;

	for (i = 0; i < (line_length - 1); i++) {
		*character++ = fgetc(text);
	}
	return(lp);
}

Code:

heras@vhbh:~/code/c$ ./read2 ~/code/text
Segmentation fault (core dumped)

Code:

(gdb) b main
Breakpoint 1 at 0x80484a5: file readfile2.c, line 12.
(gdb) r ~/code/text
Starting program: /home/heras/code/c/read2 ~/code/text

Breakpoint 1, main (argc=2, argv=0xbfcea004) at readfile2.c:12
12              open = open_file(argv[1], "r");
(gdb) n
15              int length = 73;
(gdb)
17              line_pointer = (char *)malloc(length);
(gdb)
18              for (i = 0; i < 4; i++) {
(gdb)
19                      printf("%s\n", read_line(length, open, line_pointer));
(gdb)

Program received signal SIGSEGV, Segmentation fault.
0xb7edb46d in getc () from /lib/tls/i686/cmov/libc.so.6
(gdb)
Single stepping until exit from function getc,
which has no line number information.

Program terminated with signal SIGSEGV, Segmentation fault.
The program no longer exists.

I have no clear question, by now I don't know what I'm doing anymore. Any guidance or hints on how to construct such a program would be greatly appreciated.

Two more minor / unimportant issues:
- printing a line is off by 2. It prints 70 characters but I don't see why
- if I get this to work at all, to reuse it in a socket/network program with as little modification as possible

Thanks,
heras

Code:

char *read_line(int line_length, FILE *txt)
...
	FILE *text;

	for (i = 0; i < (line_length - 1); i++) {
		if ((character = fgetc(text)) == EOF) {
			perror("fgetc: ");

You are using the uninitialized local "text" instead of the argument txt - so you are using a "rubbish" file descriptor, and that causes the seg fault.

--
Mats

Code:

char *read_line(int line_length, FILE *txt, char *lp)
{
	int i;
	char *character = lp;
	FILE *text;

	for (i = 0; i < (line_length - 1); i++) {
		*character++ = fgetc(text);
	}
        *character = '\0';  /* A good idea to terminate the string */
	return(lp);
}

You would be much better to store what fgetc returns to an int.
That way you can test whether fgetc returned EOF, and break out of the for loop.
Also for starters, don't use assignment and pointer increment at the same time.
Break the statements to verify your logic of code flow.
My point is that your pointer named 'character' should be assigned after being incremented,
if there was a character read.

Thanks matsp! That was an easier fix than I thought. xuftugulus, thanks for catching those bugs, I'll clean up the loop with proper checking.

The off by 2 thing turns out to be related to the source file (perhaps a carriage return), it prints 72 char lines just fine.

Yay! It now does what I want, sort of. I have some questions though.

- gcc -Wall doesn't care whether read_line() is int or char. I thought EOF is int, no?
- Is there a point in freeing memory just before exiting?
- Is there a nicer way to do the end = EOF; return (end); thingy?
- Any comments on beginner badness are most welcome.

Thanks,
heras

Code:

/*
        readfile2.3.c
*/

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <ctype.h>

static FILE *open_file(char *, char *);
int read_line(int, FILE *, char *);

int main(int argc, char *argv[])
{
    if (argc != 2) {
        fprintf(stderr, "%s usage: <address>\n", argv[0]);
        exit (EXIT_FAILURE);
    }

    FILE *open;
    open = open_file(argv[1], "r");

    int line_length = 73;
    char *line_pointer;
    line_pointer = (char *)malloc(line_length);
    while (read_line(line_length, open, line_pointer) != EOF) {
        printf("%s\n", line_pointer); //FIXME: does not print last line
    }
    free(line_pointer);
    return (EXIT_SUCCESS);
}

static FILE *open_file(char *file, char *mode)
{
    FILE *fp = fopen(file, mode);
    if (fp == NULL) {
        perror("fopen");
        exit(EXIT_FAILURE);
    }
    return (fp);
}

int read_line(int line_length, FILE *file, char *line_pointer)
{
    int i = 0, end = 0, c;
    while ((c = fgetc(file) ) != EOF) {
        if ((isalnum(c) != 0 || ispunct(c) != 0 || c == ' ')) {
            *line_pointer = c;
            line_pointer++;
        }
        if (i == (line_length - 2)) {
            break;
        }
        i++;
    }
    *line_pointer = '\0';
    if (c == EOF) {
        end = EOF;
    }
    return (end);
}

Code:

heras@vhbh:~/code/c$ ./read ~/code/text
 Open text file for reading. The stream is positioned at the beginning
of the file. r+ Open for reading and writing. The stream is positioned
 at the beginning of the file. w Truncate file to zero length or creat
e text file for writing. The stream is positioned at the beginning of th
e file. w+ Open for reading and writing. The file is created if it doe
s not exist, otherwise it is truncated. The stream is positioned at the
beginning of the file. a Open for appending (writing at end of file).
The file is created if it does not exist. The stream is positioned at th
e end of the file. a+ Open for reading and appending (writing at end o
f file). The file is created if it does not exist. The initial file posi
tion for reading is at the beginning of the file, but output is always a
heras@vhbh:~/code/c$ Yay! :D

> - gcc -Wall doesn't care whether read_line() is int or char. I thought EOF is int, no?

EOF is defined as -1, which in signed char is indistinguishable from the char of all ones (255 == -1). Character number 255 is not commonly used, but if you read a binary file it may well occur.

> - Is there a point in freeing memory just before exiting?
Yes, neatness and completeness.

> - Is there a nicer way to do the end = EOF; return (end); thingy?
See below.

> - Any comments on beginner badness are most welcome.
See below


There is a function called "open()" [in Linux at the very least - recent MS libraries call this _open()]. So don't call your FILE * variable open - you'll get into trouble sooner or later if you do.

You do not check if fopen() is successfull.

I would re-order the arguments to read_line(), such that you pass linelenght and linebuffer next to each other, with the FILE * argument either at the front or the back of the list.

You can do the return statement in several different ways. Here's a few options:

Code:

return (c == EOF)?EOF:0;
// or simpler, since you are checking for exactly EOF:
return c; 
// or:
if (c == EOF) return EOF; 
else return 0;

The above solutions replace the "end" variable and any code related to setting the end variable.

Why is open_file static, but read_line isn't?

--
Mats

Originally Posted by matsp

You do not check if fopen() is successfull.

So checking if (fp == NULL) is not enough? When I enter a non-existent file name, the program quits with "fopen: No such file or directory". Perhaps this would be better? (I don't know if this compiles but something along these lines):

Code:

static FILE *open_file(char *file, char *mode)
{
    if ((FILE *fp = fopen(file, mode)) == -1) {
        perror("fopen");
        exit(EXIT_FAILURE);
    }
    return (fp);
}

return c;

Hehe! I'm definitely not the sharpest tool in the box.

Why is open_file static, but read_line isn't?

I'm going to think about that because I actually don't know.

Thanks a lot for your feedback.
Cheers,
heras

Note that simply calling exit from a function is discouraged. The function should generally return success or failure and the caller should take appropriate action.

Originally Posted by heras

So checking if (fp == NULL) is not enough? When I enter a non-existent file name, the program quits with "fopen: No such file or directory". Perhaps this would be better? (I don't know if this compiles but something along these lines):

Code:

static FILE *open_file(char *file, char *mode)
{
    if ((FILE *fp = fopen(file, mode)) == -1) {
        perror("fopen");
        exit(EXIT_FAILURE);
    }
    return (fp);
}

Sorry, I wasn't paying attention - that's fine as it is. Although it is often better to return failure to the calling code if it's not a completely catastrophic failure.

Re: static, Do you understand what static actually does?

--
Mats

The function should generally return success or failure and the caller should take appropriate action.

Although it is often better to return failure to the calling code if it's not a completely catastrophic failure.

Thanks, I'll try to move that stuff to main().

Do you understand what static actually does?

I think it makes sure that the source of whatever kind cannot be altered. Or was that const ... But I haven't yet rtfm of either static or const.

edit:

In computer programming, static variables data value persists for the life of the running process and typically have a broader scope than other variables. Their values may be set at run-time or may change subtly during program execution.

Reads somewhat like global variables. But anywho, I'm of to the documentation.

static on variables make them act like globals except with more limited scope (if they are global in the file, they will only be visible within the file, if the static is in a function then the variable is only visible in the function - so no other function can "mess" with the variable).

But for functions, it has the same meaning as static on a file-global variable: it hides the function from other modules in the project. Since this is a single-file-project, it makes little difference here - I was just curious as to why you decided to make one static and the other one not.

--
Mats

Originally Posted by matsp

Since this is a single-file-project, it makes little difference here - I was just curious as to why you decided to make one static and the other one not.

Thanks for the explanation, I think I understand
Actually, the next thing I wanted to try is putting the functions in separate files, so by then it may be more relevant to consider this.

And after asking twice, I notice that I still haven't answered your question, sorry:

I was just curious as to why you decided to make one static and the other one not.

My implementation comes from this FAQ without fully understanding it.

I think I understand

Turns out I'm wrong. The below code produces an "interesting" compiler output:

Code:

heras@vhbh:~/code/c/readfile$ gcc -Wall readfile.c open_file.c read_line.c -o read
open_file.h:1: warning: ‘open_file’ used but never defined
open_file.c:7: warning: ‘open_file’ defined but not used
/tmp/ccMsUG3J.o: In function `main':
readfile.c:(.text+0x61): undefined reference to `open_file'
collect2: ld returned 1 exit status

Code:

/*
		readfile2.3.c
*/

#include <stdio.h>
#include <stdlib.h>
#include "open_file.h"
#include "read_line.h"

int main(int argc, char *argv[])
{
	if (argc != 2) {
		fprintf(stderr, "%s usage: <address>\n", argv[0]);
		exit (EXIT_FAILURE);
	}

	FILE *open_fd;
	open_fd = open_file(argv[1], "r");

	int line_length = 73;
	char *line_pointer;
	line_pointer = (char *)malloc(line_length);
	while (read_line(open_fd, line_length, line_pointer) != EOF) {
		printf("%s\n", line_pointer); //FIXME: does not print last line
	}
	free(line_pointer);
	return (EXIT_SUCCESS);
}

open_file.h

Code:

static FILE *open_file(char *, char *);

open_file.c

Code:

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include "open_file.h"

static FILE *open_file(char *file, char *mode)
{
	FILE *fp = fopen(file, mode);
	if (fp == NULL) {
		perror("fopen");
		exit (EXIT_FAILURE); //FIXME: return failure
	}
	return (fp);
}

read_line.h

Code:

int read_line(FILE *, int, char *);

read_line.c

Code:

#include <stdio.h>
#include <ctype.h>
#include "read_line.h"

//FIXME: static int read_line() ?
int read_line(FILE *file, int line_length, char *line_pointer)
{
	int i = 0, c;
	while ((c = fgetc(file) ) != EOF) {
		if (isalnum(c) != 0 || ispunct(c) != 0 || c == ' ') {
			*line_pointer = c;
			line_pointer++;
		}
		if (i == (line_length - 2)) {
			break;
		}
		i++;
	}
	*line_pointer = '\0';

	return (c);
}

Removing static from both open_file.h and open_file.c makes the "interesting" part go away.
Note1 that some of the badness has not been fixed, but it will be.
Note2 that matsp rules (together with all the other helpfull folks in these parts).

ps:I don't have a question about this code, it's just a follow-up.

That is EXACTLY what the expected effect of static gives - it hides the function from other compilation units (that is, other .c files).

--
Mats

See how the linker complained it couldn't find the function? Indeed, that's because it was hidden. So you get a linking error. And if you remove it... then it works.