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

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

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).

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").

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)

[...]
> -        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.

>          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?

[...]

-- 
< 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