Re: [Nbd] Starting over?

[Wouter Verhelst <w@...112...>]

[still needed to reply to this]

On Tue, May 05, 2015 at 10:58:45PM +0100, Alex Bligh wrote:
> 
> On 5 May 2015, at 21:36, Tuomas Räsänen <tuomasjjrasanen@...1261...> wrote:
> 
> >> 
> >> Also, the lack of other people helping out with the source base have
> >> held me back in that area; but perhaps this lack of interest is mainly
> >> due to the fact that the code is so badly readable...?
> > 
> > Actually, the bad smell around the code was one of the reasons I got interested
> > in helping out about two years ago. Careful refactoring, step-by-step, is quite
> > satisfactory, am I the only village idiot getting kicks from fixing smelly
> > C-code (and wearing black leather programming gloves)?
> 
> +1. As someone who has had the occasional burst of fixing up the code
> in the past, I don't think it's unfixable. There's bits of it that are
> a bit smelly, but most of those are related to the protocol - some
> to back compatibility, but some to flaws in the present day protocol
> (e.g. how do you report an error halfway through a large read unless
> the read is RAM buffered first).
> 
> There's some restructuring that would be nice to do - for instance
> the negotiation stuff (last time I looked at it) was opaque in how
> it worked and a state machine implementation would be cleaner. And
> it would be nice to separate this from the mainloop to give a choice
> of backends (AIO etc.), and give a choice of forking vs threading
> vs whatnot.
> 
> But I've seen far worse, and I'm not convinced a 'nuke from orbit'
> approach is really necessary unless you're going to so something
> truly radical like incorporate it into the kernel (and no, I see
> no need for that).

Right, let's keep working with the current code base then. I *had* been
dabbling at a better design, but it wasn't very far yet and both of your
comments have ensured it's pretty much dead now :-)

What I think is the main issue with nbd-server, though, is the fact that
it uses the stack as a state machine. That is, we read out one request
from the network (e.g., a read request), call a function to handle it
(expread), which checks if we do copy-on-write and calls rawexpread,
which does a seek and reads data into a buffer, and then we unwind the
stack and eventually (in mainloop()) we send the reply to the client.

Instead, it should be refactored so that it reads a request into a
buffer, and returns to mainloop if we block for I/O, so that we can
handle multiple requests at the same time.

I did start work on that a while back (that's what the io_transaction
branch contains), but the result there is a) even uglier than the main
branch, and b) somewhat outdated now, too, to the extent that it would
probably take quite some effort to be brought up to speed as well.

Because of the ugliness I didn't really finish it, but maybe I should
just whip it into shape and deal with the fallout...

-- 
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26