On Tue, Apr 12, 2016 at 03:15:27PM +0100, Alex Bligh wrote:
> Wouter,
>
> On 12 Apr 2016, at 15:04, Wouter Verhelst <w...@uter.be> wrote:
>
> > On Mon, Apr 11, 2016 at 06:15:38PM +0100, Alex Bligh wrote:
> > [...]
> >> +#ifdef WITH_GNUTLS
> > [...]
> >> +#else
> >> +
> >> + send_reply(opt, *net, NBD_REP_ERR_UNSUP, 0, NULL);
> >
> > NBD_REP_ERR_POLICY (or NBD_REP_ERR_PLATFORM, perhaps).
>
> But this is *without* TLS support compiled in. Surely the correct error
> is NBD_REP_ERR_UNSUP, i.e. the same error as it gives now without
> the TLS code? IE it can NEVER work against this server, unless
> the server's code is changed. It's not a local policy thing.
PLATFORM is "this option is not supported on the platform where it was
compiled". If that platform doesn't have GnuTLS, then you disable
STARTTLS, so it can't work.
Maybe the names were wrong, but the plan was:
INVALID -- client sent something obviously wrong
POLICY -- server admin did something wrong or disabled the option
UNSUP -- server does not have code to handle request
PLATFORM -- server does have code to handle request, but it's not
compiled in for whatever reason (e.g., something required on the
platform is not available)
Obviously the last applies here.
(and just as obviously POLICY doesn't, either -- the "perhaps" line
above is the wrong way round, sorry)
The solution to an INVALID error is "fix the damn client"
The solution to a POLICY error is "fix the damn server config"
The solution to a PLATFORM error is "recompile the server and/or run it
on the right system"
The solution to an UNSUP error is "implement missing functionality"
There's a clear difference between the latter two.
> > You should think of NBD_REP_ERR_INVALID as 4xx errors (i.e., "you're
> > doing it wrong"), and of NBD_REP_ERR_POLICY (or NBD_REP_ERR_PLATFORM) as
> > 5xx errors ("something went wrong on my end").
>
> So nothing went wrong server end. He's running a server without
> TLS built in.
It's a detail, I realize, but that's not what this was meant to do.
> > NBD_REP_ERR_UNSUP really is only meant for "I don't know what the ****
> > you're talking about". It should only be referenced just once in a
> > server (in a "default:" case of a switch statement)
>
> He doesn't know what you're talking about - no TLS!
He does, you've gone into a separate function.
> > [...]
> >> - sock_flags_old = fcntl(net, F_GETFL, 0);
> >> - if (sock_flags_old == -1) {
> >> - msg(LOG_ERR, "Failed to get socket flags");
> >> - goto handler_err;
> >> - }
> >> -
> >> - sock_flags_new = sock_flags_old & ~O_NONBLOCK;
> >> - if (sock_flags_new != sock_flags_old &&
> >> - fcntl(net, F_SETFL, sock_flags_new) == -1) {
> >> - msg(LOG_ERR, "Failed to set socket to blocking mode");
> >> - goto handler_err;
> >> - }
> >> + if (set_nonblocking(client->net, 0) < 0) {
> >> + msg(LOG_ERR, "Failed to set socket to blocking mode");
> >> + goto handler_err;
> >> + }
> >
> > Some whitespace errors there.
>
> Couldn't see them, but reindented it for v4.
:)
> >> if (set_peername(net, client)) {
> >> msg(LOG_ERR, "Failed to set peername");
> >> @@ -2241,11 +2375,15 @@ handle_modern_connection(GArray *const servers,
> >> const int sock)
> >>
> >> msg(LOG_INFO, "Starting to serve");
> >> serveconnection(client);
> >> + close (net);
> >> + if (client->net != net)
> >> + close (client->net);
> >
> > Probably safer to have a block here, rather than a single line?
>
> You mean around "close (client->net)"? Sure, will do for v4. Is there
> some guideline you use? (CodingStyle is remarkably tolerant).
CodingStyle is also remarkably ancient ;-)
I should probably update or ditch it. Some other time.
--
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
people in the world who think they really understand all of its rules,
and pretty much all of them are just lying to themselves too.
-- #debian-devel, OFTC, 2016-02-12
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general
