Thread: 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

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

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

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.