Re: The problem is with cleanups which should never be executed, unless an error happens.

Posted Dec 29, 2014 18:14 UTC (Mon) by cesarb (subscriber, #6266)
In reply to: Re: The problem is with cleanups which should *never* be executed, unless an error happens. by ldo
Parent article: The "too small to fail" memory-allocation rule

> If the second thing subsumes the first thing (so cleaning up the second thing automatically cleans up the first thing), then you can set the first thing to NULL at this point, to avoid a double cleanup.

You still are in the "cleanup" mentality. But the problem with filesystems is not "cleanup", it's "rollback". There's nothing to subsume the relevant code, and it subsumes nothing else; it's really something which executes only on failure.

> So when you get to the end, you simply execute all the relevant cleanups unconditionally, and the ones with NULL arguments turn into no-ops.

That doesn't help with the situation in question, where the memory allocation routines never return "failure". The relevant error-recovery code would always get passed NULL, and so the branch which is called when it receives a non-NULL never gets tested.

Sure, you *called* the code unconditionally, but it doesn't change the fact that it *executes* conditionally. Whether the branch point is outside or inside it is immaterial. For instance, it's well-known that the standard "free(void *ptr)" function does nothing when called with a NULL argument, but that's because it begins with a conditional branch: "if (!ptr) return;". If it were always called with a NULL pointer, the most complex part of its code would never be exercised.

Re: You still are in the "cleanup" mentality.

Posted Dec 30, 2014 20:34 UTC (Tue) by ldo (guest, #40946) [Link] (2 responses)

That’s right. You keep insisting that “cleanup” and “rollback” are entirely different, whereas they are really two aspects of the same thing, and can be treated as such.

> That doesn't help with the situation in question, where the memory
> allocation routines never return "failure".

Returning NULL from an allocation request is a failure.

> Sure, you *called* the code unconditionally, but it doesn't change
> the fact that it *executes* conditionally.

Do you know what “abstraction” means?

Re: You still are in the "cleanup" mentality.

Posted Dec 30, 2014 22:33 UTC (Tue) by cesarb (subscriber, #6266) [Link] (1 responses)

> > That doesn't help with the situation in question, where the memory allocation routines never return "failure".
> Returning NULL from an allocation request is a failure.

Please, reread the article this comment thread is attached to.

The whole issue is that, under certain conditions, the allocation requests were *never* returning NULL, even when they should!

> > Sure, you *called* the code unconditionally, but it doesn't change the fact that it *executes* conditionally.
> Do you know what “abstraction” means?

Please, reread the article this comment thread is attached to.

Abstraction or not, it doesn't change the fact that, since the allocation requests were *never* returning NULL, even when they should, the error handling code was *never* being executed. It doesn't matter whether it has been abstracted away or not, untested code is untested code.

Re: requests were never returning NULL, even when they should!

Posted Dec 31, 2014 21:00 UTC (Wed) by ldo (guest, #40946) [Link]

Precisely the point. And changing them to return NULL is fraught, because the error-handling paths in the callers are quite likely riddled with omissions where they should be dealing with this case. And finding and fixing those omissions is hard, because the error-handling paths are so complex.

Which is why I am advocating simpler error-handling paths, as in my example.

Re: The problem is with cleanups which should *never* be executed, unless an error happens.