Wouter,
On 12 Apr 2016, at 15:04, Wouter Verhelst <[email protected]> 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.
> 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.
> 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!
> [...]
>> - 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).
--
Alex Bligh
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/nbd-general
