file_read block

vashek 0 385 Views

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;
}
Member Avatar for Salem
Salem 5,138 Posting Sage

> 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" ?

commented: Good advice :) +23
Member Avatar for wildgoose
wildgoose 420 Practically a Posting Shark

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=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!

Edited by wildgoose because: n/a
Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.

Edit Preview