A Sane generic cleanup?

Question

I would like to learn a generic cleanup approach which applies to a scenario like the following. Please remember that this is just a -=SAMPLE=-.

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

int main(void)
{
  unsigned char *uchar1, *uchar2, *uchar3;

  if ((uchar1 = malloc(sizeof(*uchar1) * 10)) == NULL) {
    fprintf(stderr, "Error: malloc(uchar1);\n");

    return 1;
  }

  if ((uchar2 = malloc(sizeof(*uchar2) * 10)) == NULL) {
    fprintf(stderr, "Error: malloc(uchar2);\n");

    free(uchar1);
    return 1;
  }

  if ((uchar3 = malloc(sizeof(*uchar3) * 10)) == NULL) {
    fprintf(stderr, "Error: malloc(uchar3);\n");

    free(uchar1);
    free(uchar2);
    return 1;
  }

  /* do something */

  free(uchar1);
  free(uchar2);
  free(uchar3);
  return 0;
}

Be careful or you'll start to reinvent C++ (specifically, destructors). — Jerry Coffin, Aug 02 '10 at 17:04
possible duplicate of [How to avoid long chain of free's (or deletes) after every error check in C?](http://stackoverflow.com/questions/3339946/how-to-avoid-long-chain-of-frees-or-deletes-after-every-error-check-in-c) — caf, Aug 02 '10 at 23:49

torak · Accepted Answer · 2010-08-02T20:49:02.143

5

I think I know what you're getting at. Somehing like the following can make it easier to ensure that you correctly manage resources. I beleive that it is one case where goto needn't be considered harmful.

Edited to reflected Summy0001's excellent observarion and fix return of wrong value.

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

int main(void) 
{ 
  unsigned char *uchar1 = NULL;
  unsigned char *uchar2 = NULL;
  unsigned char *uchar3 = NULL; 
  int result = 0;

  if ((uchar1 = malloc(sizeof(*uchar1) * 10)) == NULL) { 
    fprintf(stderr, "Error: malloc(uchar1);\n");
    result = 1;
    goto CLEANUP0;
  } 

  if ((uchar2 = malloc(sizeof(*uchar2) * 10)) == NULL) { 
    fprintf(stderr, "Error: malloc(uchar2);\n"); 
    result = 1;
    goto CLEANUP1;
  } 

  if ((uchar3 = malloc(sizeof(*uchar3) * 10)) == NULL) { 
    fprintf(stderr, "Error: malloc(uchar3);\n"); 
    result = 1;
    goto CLEANUP2;
  } 

  /* do something */ 


  free(uchar3);
CLEANUP2:
  free(uchar2);
CLEANUP1:
  free(uchar1);
CLEANUP0:
  return result; 
}

edited Aug 02 '10 at 20:49

answered Aug 02 '10 at 16:45

torak

5,684
21
25

Yes, this is what I'm trying to resolve - but the cleanup should be done properly. (which means, calling cleanup routines of routines that been previously called, not ALL of the cleanup routines). How would you suggest doing that? toggle indicator for each and every routine? – Doori Bar Aug 02 '10 at 16:50
As I've shown, initialise things to invalid values. Then in the CLEANUP code check the value. If the value is still invalid then the resource doesn't need to be released. On the other hand, if it is valid it does need to be released. For resources where an invalid value isn't known you can use a separate boolean value to track if a resource has been acquired. – torak Aug 02 '10 at 16:57
@Doori: obviously, you do not have only single label. I personally (for sake of example) would have used several labels: one per every goto/free(). – Dummy00001 Aug 02 '10 at 19:10
@Dummy00001: Yup, that's what I did :) – Doori Bar Aug 02 '10 at 19:21
@Dummy00001 & @Doori: That's probably actually a better solution, given that it also handles the tracking of what has / hasn't been allocated. – torak Aug 02 '10 at 19:23
I'm actually thinking of using a 1 label of 'cleanups', with a switch() ... that way all the gotos goto 'cleanups', and prior to each step - use a counter++; which orients the cleanup phase. – Doori Bar Aug 02 '10 at 20:21
@Doori: Counters suck for the purpose. Plain gotos - with properly named labels - are self documenting. With a counter (== poor imitation of a FSM) one has to think every time "what the count is after that line?" – Dummy00001 Aug 02 '10 at 22:28

A Sane generic cleanup?

1 Answers1