3 ms·
What the parent said still applies in your case. You just make a wrapper function that does the cleanup. I have used this pattern many times: static int foo_
by derf_ 11y ago
What the parent said still applies in your case. You just make a wrapper function that does the cleanup. I have used this pattern many times:
static int foo_impl(int arg, int **resource1, int **resource2) {
*resource1 = (int *)malloc(sizeof(**resource1));
if (*resource1 == NULL) return EXIT_FAILURE;
/* Do something with resource1 (omitted)... */
*resource2 = (int *)malloc(sizeof(**resource2));
if (*resource2 == NULL) return EXIT_FAILURE;
/* Do something with resource1 and resource2 (e.g.,
stick them in a global hash table as key/value pairs,
or whatever, omitted)... */
return EXIT_SUCCESS;
}
int foo(int arg) {
int *resource1;
int *resource2;
int ret;
resource1 = resource2 = NULL;
ret = foo_impl(arg, &resource1, &resource2);
if (ret != EXIT_SUCCESS) {
free(resource2);
free(resource1);
}
return ret;
}
The advantages are that it makes it very explicit which resources need to be cleaned up (making it easier to review to be sure you haven't forgotten one, or forgotten to initialize one, because they're in a nice list), and it's harder to screw up the control flow. You can't get out of the function without passing by the clean-up code, and you can't accidentally fall into the clean-up code without explicitly returning an error.
- luso_brazilian 11y agoI know it is just an example to illustrate the point but this seems to have a bug in it: resource2 won't be initialized if the function fail to allocate resource1 so resource2 will be null when the code enters the deallocation condition. A way to fix it would be to return different failures for every allocation and to use a switch without a break to clean up, something like: switch (ret) { case EXIT_SUCCESS: free(resource2); case EXIT_FAILURE2: free(resource1); case EXIT_FAILURE1: break; } EDIT: added potential fix to the code. Sorry if there is any bug in the fix, my C is a little bit ... rusty. EDIT2: fixed a bug in the fix. C is hard, let's go shopping. EDIT3: the explanation for the bug was also wrong. It is not a memory leak for but freeing a null pointer. Fixed too.
- deleted 11y ago[deleted]
- protopete 11y agoPassing a null pointer to free() is perfectly safe, and in this case simplifies the cleanup code. Although the concern of a memory leak is valid. I would have expected the resources to be free'd regardless of the result.
- luso_brazilian 11y agoOf course, you are right. I seem to have carried the belief that freeing a null pointer was as bad as freeing one twice. And now I know. Seems to be a common misconception [1], thanks for pointing out, that makes the GGP code correct in all counts and my switch redundant. [1] https://news.ycombinator.com/item?id=8844031 https://news.ycombinator.com/item?id=8844031
- TheLoneWolfling 11y agoExcept of course that many languages seem to be trying to remove fallthroughs from switches too...
- malka 11y agoOr you could simply free resource1 and resource2 even if the allocation failed. free(NULL) is defined to do nothing.
- deleted 11y ago[deleted]