file_read block

By vashek on Sep 10th, 2009 7:26 pm

Hi All,

this is my first C code in my professional life. I wanted to write a common function to retrieve file content. I do not know whether below code is upto the mark or not.

any Suggestions/modifications/tips are greatly appreciated.

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

/*
	Function Name: get_file_content
	Description: fills content of the file to given buffer.
	Parameters: file to be read(in), Mode(in), destination buffer(out)
	Return Type: pointer to content (SUCCESS), otherwise NULL
*/
char* get_file_content (char *file_name, char *file_mode, char *file_content)
{
	FILE *fp;
	long file_size;	
	size_t bytes_read;
	
	//open file
	if (!(fp = fopen (file_name, file_mode))) {
		printf ("*** Unable to open file ***\n");	
		return NULL;
	}
	
	//get file size
	fseek (fp, 0, SEEK_END);
	file_size = ftell (fp);
	rewind (fp);
	printf("*** size = %ld ***\n", file_size);

	//check memory
	file_content = (char *) realloc (file_content, sizeof(char) * file_size);
	if (file_content == NULL) {
		printf("*** memory insufficient ***");
		fclose (fp);
		return NULL;
	}

	//get content
	bytes_read = fread (file_content, sizeof(char), file_size, fp);
	if (bytes_read != file_size) {
		printf("*** read failed ***");
		fclose (fp);
		return NULL;
	}

	//put EOS
	file_content[bytes_read-1] = '\0';	

	//clean	
	fclose (fp);

	//success
	return file_content;
}


int main (int argC, char *argV[])
{
	int	status = -1;
	char *file_content = (char *) malloc (2);
	if ((file_content = get_file_content ("metadata.txt", "r", file_content)) == NULL){
		status = 1;	
		goto FREE_BLOCK;
	}
		
	printf("\n#%s#\n", file_content);


	FREE_BLOCK:
		free (file_content);

	return 0;
}

Regards,
Vashek

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


/*
	Function Name: get_file_content
	Description: fills content of the file to given buffer.
	Parameters: file to be read(in), Mode(in), destination buffer(out)
	Return Type: pointer to content (SUCCESS), otherwise NULL
*/
char* get_file_content (char *file_name, char *file_mode, char *file_content)
{
	FILE *fp;
	long file_size;	
	size_t bytes_read;
	
	//open file
	if (!(fp = fopen (file_name, file_mode))) {
		printf ("*** Unable to open file ***\n");	
		return NULL;
	}
	
	//get file size
	fseek (fp, 0, SEEK_END);
	file_size = ftell (fp);
	rewind (fp);
	printf("*** size = %ld ***\n", file_size);

	//check memory
	file_content = (char *) realloc (file_content, sizeof(char) * file_size);
	if (file_content == NULL) {
		printf("*** memory insufficient ***");
		fclose (fp);
		return NULL;
	}

	//get content
	bytes_read = fread (file_content, sizeof(char), file_size, fp);
	if (bytes_read != file_size) {
		printf("*** read failed ***");
		fclose (fp);
		return NULL;
	}

	//put EOS
	file_content[bytes_read-1] = '\0';	

	//clean	
	fclose (fp);

	//success
	return file_content;
}


int main (int argC, char *argV[])
{
	int	status = -1;
	char *file_content = (char *) malloc (2);
	if ((file_content = get_file_content ("metadata.txt", "r", file_content)) == NULL){
		status = 1;	
		goto FREE_BLOCK;
	}
		
	printf("\n#%s#\n", file_content);


	FREE_BLOCK:
		free (file_content);

	return 0;
}

2 Years Ago

Show Comments

> file_content = (char *) realloc (file_content, sizeof(char) * file_size);
Is this C or C++?
If it is C, then you don't need the cast.

However, what happens if realloc fails? The problem is, you just trashed your pointer with NULL, thus making it impossible to free a memory leak (if that was your only pointer).

Also, if realloc MOVES the memory block, you've only updated your local copy of the pointer (the one in main still points to the original (and now freed) block).

The whole 3rd parameter is a waste of effort. You may as well have a local pointer, initialise it with malloc and return it.

> file_content[bytes_read-1] = '\0';
What if filesize was in fact zero?
Here you're off into the weeds somewhere, getting bogged down by a segfault no doubt.
Even if the file length is non-zero, you trash the last char of the file.

Also, what if someone tries to load in say a DVD image file - will you handle that well?

What if they call the function with a mode of "w" ?

Salem

Posting Sage

Team Colleague

11,531 posts since Dec 2005

Reputation Points: 5,862

Solved Threads: 953

2 Years Ago

I am assuming you are reading an ASCII file. Do not overwrite the last byte in the file. Get file length. Allocated buffer of size+1. Read file of size into the buffer. Set terminator (that extra byte you appended) buffer[size]=0;

It'll handle the empty file scenario! But you really should have a length check for zero, and not read the zero bytes!

Or handle an empty file as a special kind of error!

I personally prefer returning bool and passing the address of the pointer as an argument along with an address of a length count just so I can have both whether I chose to ignore the length count or not! Then it doesn't matter if the file is ASCII or binary, you can use the same loader for all file loads into memory!

wildgoose

Practically a Posting Shark

896 posts since Jun 2009

Reputation Points: 546

Solved Threads: 99

This article has been dead for over three months

You