Re: [Nbd] [PATCHv2 5/6] Add TLS support to server

[Alex Bligh <alex@...872...>]

Wouter,

On 12 Apr 2016, at 15:04, Wouter Verhelst <w@...112...> 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