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

[Laszlo Ersek <lersek@redhat.com>]

On 12/10/15 10:22, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> 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 agree. :)

Thanks
Laszlo

> 
>> 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.
>