Re: [Nbd] [PATCH 2/5] nbd-server: fix open_modern() to report errors

[Tuomas Jorma Juhani Räsänen <tuomasjjrasanen@...1261...>]

On Sat, Jan 12, 2013 at 12:42:41PM +0100, Wouter Verhelst wrote:
> 
> This having to repeatedly close or free or initialize a particular value
> creates a lot of clutter, which makes it harder to follow what's really
> going on. It's also a possible source of future bugs; when we add
> another variable, or change how we need to deinitialize it, or something
> similar, we'd need to ensure that each and every one of these if blocks
> is updated; if we miss one, we introduce a possible leak.
> 

Yes, fully agreed, see below.

> Instead, I'd prefer if you could use goto here. This is about the only
> situation where goto is a good idea, but then it's a really good idea:
> 
.
.
.
> This keeps the deinitialization code together (so updating how to
> deinitialize something is done in one central location). It also keeps
> the if blocks much shorter, making the code easier to read.
> 
> (obviously the goto labels would use some descriptive name instead, but
> you got that, right? ;-)
> 

And that is exactly how I am used to deal with local deallocation
(like I've done in libm210 and libsuinput for example). But the reason
why the goto-pattern was not used here, was that I just somehow
thought that the patch would be clearer to reason about at this stage
without the goto change and then it would have been the responsibility
of yet another patch to clean the internal dellocation. It's perhaps a
bit silly, but I try to construct patches very carefully to contain
only the bare minimum. Here, the bare minimum was the error reporting
change.

But anyways, I'll include the goto-fix to that and resend all three.

I'm glad to find that we share the idea/vision (or at least quite a
lot of it) of how source code should look like or how it should be
written.

-- 
Tuomas