Re: [Qemu-devel] Error handling in realize() methods

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> I've been following this discussion with great interest.
>
> My opinion should not be considered, because I won't be turning my
> opinion into new code, or an agreement to support / maintain code. :)
>
> My opinion is that
> - every single allocation needs to be checked rigorously,
> - any partial construction of a more complex object needs to be rolled
>   back in precise reverse order upon encountering any kind of failure
>   (not just allocation)
> - this should occur regardless of testing coverage (although projects
>   exist (for example, SQLite, IIRC) that use random or systematic
>   malloc() error injection in their test suite, for good coverage)
> - the primary requirements for this to work are:
>   - a clear notion of ownership at any point in the code
>   - a disciplined approach to ownership tracking; for example, a helper
>     callee (responsible for constructing a member of a more complex
>     object) is forbidden from releasing "sibling" resources allocated
>     by the caller
>
> This is possible to do (I'm doing it and enforcing it in OVMF), but it
> takes a lot of discipline, and *historically* the QEMU codebase has
> stunk, whenever I've looked at its ownership tracking during
> construction of objects.

Doing it from the start is one thing.  Laudable in theory, justifiable
in practice for some applications (I've seen it done for a satellite's
avionics software), but in general, life's too short for that kind of
manual error handling.

Retrofitting it into a big & messy existing code base is another thing
entirely.  Believe me, I've done my share of cleaning up stuff.  When
you're tempted to mess with something that works (although you barely
understand how or even why) to make it neater, or easier to maintain, or
handle vanishingly rare errors more gracefully, the one thing you need
to know is when *not* to do it, because the risk of you fscking it up
outweighs whatever you hope to gain.

> I feel that in the last sequence of months (years?) the developer
> discipline and the codebase have improved a *great* deal. Still I cannot
> say how feasible it would be to bring all existent code into conformance
> with the above.

Hah!  We can't even get all the existent qdev init() methods converted
to realize(), which is a *much* simpler task.  And that conveniently
ignores all the code that hasn't even made it to qdev.

We got bigger fish to fry.  In fact, the queue of fish waiting to be
fried goes along the block a couple of times.

> ... As I said, I just wanted to express this opinion as another (not
> really practical) data point. My children utterly hate spinach, so
> Markus's counterpoint is definitely not lost on me.

To anyone advocating creating thousands (or even dozens) of non-trivial
new error paths: come back with a test suite.  We have mountains of
evidence for chronic inability to get error paths right.  In fact, I'm
working on a patch that adds a bunch of FIXMEs for one class of flawed
error recovery.  Just FIXMEs, because I don't dare fixing the bugs
myself without a way to test the fix, and the fishes in the queue are
looking at me accusingly already.

If I may suggest a logo for such a test suite: kids eating spinach with
a smile feel apt.