Thread: Better way to free resources on error

Is there a better way to handle this situation? Everytime I open a file and allocate memory I check if it is successful. If not then free all files and memory before I return an error.

Code:

int edb_initBlob(edb_blobStore *store, char *path)
{
   store->writer.fp = fopen(path, "rb+");
   if(!store->writer.fp)
      return EDB_BLOB_IOERR;

   store->reader.fp = fopen(path, "rb");
   if(!store->reader.fp) {
      fclose(store->writer.fp);
      return EDB_BLOB_IOERR;
   }

   u32 *blobInf = malloc(sizeof(u32) * 4);
   if(!blobInf)  {
      fclose(store->writer.fp);
      fclose(store->reader.fp);
      return EDB_BLOB_NOMEM;
    }
   size_t ret = fread(blobInf, sizeof(u32), 4, store->reader.fp);
   if(ret != 4)  {
      free(blobInf);
      fclose(store->writer.fp);
      fclose(store->reader.fp);
      return EDB_BLOB_IOERR;
    }
}

I am thinking of using GOTO and put all my free code in one place? Tell me what you think.

In all honesty, the best way is to use RAII, i.e. putting the cleanup in a destructor, but you'll find that you need e.g. C++ for that.

The fact that there just is no "great" solution to the problem in C is one of the reasons to look to other languages for solutions.

Ignoring the memory leak and the fact you're not returning anything in the non-error case plus the potential issues with opening the same file twice, something like this might work :

Code:

int edb_initBlob(edb_blobStore *store, char *path)
{
   int rc = EDB_OK; /* whatever a good rc is */   
   size_t ret = 0;
   u32 *blobInf = NULL;
   store->writer.fp = store->reader.fp = NULL;

   store->writer.fp = fopen(path, "rb+");
   if(store->writer.fp)
   {
      store->reader.fp = fopen(path, "rb");
      if(store->reader.fp) 
      {
         blobInf = malloc(sizeof(u32) * 4);
         if(blobInf)  
         {
	    ret = fread(blobInf, sizeof(u32), 4, store->reader.fp);
	 }
      }
   }

   if (!store->writer.fp || !store->reader.fp)
      rc = EDB_BLOB_IOERR;
   else if (!blobInf)
      rc = EDB_BLOB_NOMEM;
   else if (ret != 4)
      rc = EDB_BLOB_IOERR;

   if (rc != EDB_OK)
   {
      if (store->writer.fp)
	 fclose(store->writer.fp);
      if (store->reader.fp)
	 fclose(store->reader.fp);
      if (blobInf)
	 free (blobInf); /* should be freed regardless of RC to avoid mem leak, but this matches the original code */
   }
   return rc;
}

Not saying it's better or worse, just a different approach that's a bit closer to a C++ RAII feel. Single entry, single exit, clean up on error before leaving.

Originally Posted by iMalc

In all honesty, the best way is to use RAII, i.e. putting the cleanup in a destructor, but you'll find that you need e.g. C++ for that.

The fact that there just is no "great" solution to the problem in C is one of the reasons to look to other languages for solutions.

Still want to use C on this project just checking if there is a better way to handle this. Since there's no better solution I will just leave it like that then.

Thanks for the info.

Originally Posted by mnd22

Still want to use C on this project just checking if there is a better way to handle this. Since there's no better solution I will just leave it like that then.

Well, if you really do want to go that route, then the goto method is an acceptable way of dealing with the problem.

@KCfromNC
Thank you for the tip. I will check how can I use that approach. Sorry I forgot to put the success return code I just pasted a small part of the code.

Another question what are the potential issues on opening the same file twice? I am trying to create two FILE pointers, one for reading and writing for the same file.

@laserlight
Cool I will see how can I use goto on this.

Use an integer to keep track of your "initialization state". For each resource (file, memory, etc) you initialize, increment this counter. Write a cleanup function that uses a switch statement and fall-through to correctly release the resources if something fails. You will need to package up all the resources into a struct so you can pass it into the cleanup function.

This particular issue is THE BIGGEST reason I don't write C code anymore.

Originally Posted by Subsonics

I use goto for situations like this. I have used a macro (not by me) that prints an error message and goto the clean up section like this:

Code:

#define OUT_ON_NULL(p, msg) if(!p) { fprintf( stderr, "%s\n", msg); goto out; }

// then at the clean up

if(pointer) free(pointer);

Lately I have switched to something like the check macro in this link: 21 Exercise 20: Zed's Awesome Debug Macros that also prints error messages if errno is set and also let me test other conditions like:

Code:

check( argc == 2, "Missing argument" );

It's very important to initialize all variables at the top of the function, otherwise you might free() resources that was never allocated.

Wow! Thank you for the link Subsonics. This will help me a lot.

Originally Posted by Subsonics

I use goto for situations like this.

Me too:

Code:

struct whatever *example (args) {

	// validate args
	if (!somecondition) return null;

	struct whatever *rv = malloc(sizeof(struct whatever));

	// do more stuff
	if (!somecondition) goto problem1;

	rv->p = malloc(xxxx);

	// more stuff
	if (!somecondition) goto problem2;

	return rv; // happens if there were no problems

problem2:
	free(rv->p);

problem1:
	free(rv);

	return null;
}

Hopefully it's clear how that works.