Re: [Nbd] NBD server terminates on SIGPIPE during negotiation
Tuomas,
> #1 Set the default action of SIGPIPE to Ign.
>
> This is simplest to do, but not the right thing in my opinion. It
> would probably work and fix this specific issue, but in my opinion the
> current way of pushing through the negotation procedure no matter what
> happens seems a bit flawed.
Last time I looked at the negotiation process, its state machine
was truly impenetrable. From memory (only) it also suffered from
problems where the data to be read to get to the next state
was only partially ready, or the write partially blocked.
This means that one new connection can effectively DoS another.
I then, feeling rather haughty, went to write a negotiator all
of my own in a truly non-blocking way (this was for an nbd-speaking
alternative to nbd). It turns out to be less than simple, though
I succeeded. I would be happy to share the code but it relies on
some different buffering and list libraries and is probably
not directly incorporatable.
However, ignoring SIGPIPE is almost certainly the right thing to
do. Anything relying on SIGPIPE is basically saying "my error
handling is for the process to die". This may of course require
looking a little closer at how non-blocking I/O is done, particularly
on the write side. But it is not the only solution.
> #2 Move the negotation procedure to the child process.
>
> This would be quite clean way to ensure that catastrophic failure in
> one connection would not affect other clients. But it definitely would
> not remove the need for more robust error handling in general. And could
> it have some nasty unexpected consequences? It seems that not, but someone
> who knows the complete codebase should verify this.
I would suggest this would be a definite incremental improvement.
What it would prevent (without some IPC) one connection being able to
depend on another connection's state. In general this is no bad thing
and IIRC it's not used at the moment, but it might be useful to ensure
one disk isn't mounted by 2 clients simultaneously. I'd suggest this
would be better handled by IPC or just a lock on the disks.
> #3 Fix the error handling.
>
> Fix the error handling in negotiate() to handle all failure situations
> and abort the process if nothing can be done. I think this is something
> which should be done anyways. The current way of walking through the
> procedure no matter if it fails or not is a bit ugly.
+1
> #4 The combination: #1 + #2 + #3
>
> At this point, I'd would probably go with combining at least #1 and
> #3. I'm not sure about the #2, but if it makes sense too, then I don't
> see any problem in combining all three.
>
> Thoughts, ideas, corrections?
IMHO this is the real solution. And I'd do it in the order #2, #3, #1.
That will allow you to fix it without a major rewrite of the negotiator,
but to be honest that needs doing too.
--
Alex Bligh
Reply to: