Re: [Nbd] [PATCHv5 1/7] Add GnuTLS infrastructure

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

On Tue, May 03, 2016 at 02:14:09PM +0100, Alex Bligh wrote:
> diff --git a/configure.ac b/configure.ac
> index 7806f65..204667f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -114,6 +114,21 @@ AC_CHECK_SIZEOF(unsigned long long int)
>  AC_STRUCT_DIRENT_D_TYPE
>  AC_CHECK_FUNCS([llseek alarm gethostbyname inet_ntoa memset socket strerror strstr mkstemp fdatasync])
>  HAVE_FL_PH=no
> +AC_ARG_ENABLE(
> +  [gnutls],
> +  [AS_HELP_STRING([--enable-gnutls],[Build support for GnuTLS])],
> +  [
> +    if test "x$enableval" = "xyes"]; then
> +       AC_CHECK_LIB([gnutls], [gnutls_init], , AC_MSG_ERROR(no GnuTLS library found))

What happened to using pkg-config?

[...]
> +  if (socksetnonblock (cryptfd, 0) < 0)
> +    {
> +      errout (s, "Could not turn on blocking: %m");
> +      goto error;
> +    }

Why not reuse the function for the same purpose in nbd-server.c? We can
obviously move it elsewhere if needs be, but it seems wrong to implement
the same thing twice in one codebase.

[...]
> +  while ((!plainEOF || !cryptEOF) && !quit (s))
> +    {
> +      struct timeval timeout;
> +      int result;
> +      int selecterrno;
> +      int wait = TRUE;
> +
> +      FD_ZERO (&readfds);
> +      FD_ZERO (&writefds);
> +
> +      size_t buffered = gnutls_record_check_pending (s->session);
> +      if (buffered)
> +	wait = FALSE;		/* do not wait for select to return if we have buffered data */
> +
> +      if (plainEOF)
> +	{
> +	  /* plain text end has closed, but me may still have
> +	   * data yet to write to the crypt end */
> +	  if (bufIsEmpty (plainToCrypt) && !tls_wr_interrupted)
> +	    {
> +	      cryptEOF = TRUE;
> +	      break;
> +	    }
> +	}
> +      else
> +	{
> +	  if (!bufIsEmpty (cryptToPlain))
> +	    FD_SET (plainfd, &writefds);
> +	  if (!bufIsOverHWM (plainToCrypt))
> +	    FD_SET (plainfd, &readfds);
> +	}
> +
> +      if (cryptEOF)
> +	{
> +	  /* crypt end has closed, but me way still have data to
> +	   * write from the crypt buffer */
> +	  if (bufIsEmpty (cryptToPlain) && !buffered)
> +	    {
> +	      plainEOF = TRUE;
> +	      break;
> +	    }
> +	}
> +      else
> +	{
> +	  if (!bufIsEmpty (plainToCrypt) || tls_wr_interrupted)
> +	    FD_SET (cryptfd, &writefds);
> +	  if (!bufIsOverHWM (cryptToPlain))
> +	    FD_SET (cryptfd, &readfds);
> +	}

This is, essentially, twice the same code. I've developed an allergy to
that. Can use a function or a macro or something of the sorts instead of
copy/pasting lines of code?

Also, I can't help but think this is not the most efficient way to do
things, although I'm not awake enough yet to come up with a better way
:-)

[...]
> +      /* Repeat select whilst EINTR happens */
> +      do
> +	{
> +	  timeout.tv_sec = wait ? 1 : 0;
> +	  timeout.tv_usec = 0;
> +	  result = select (maxfd, &readfds, &writefds, NULL, &timeout);
> +
> +	  selecterrno = errno;
> +	}
> +      while ((result == -1) && (selecterrno == EINTR) && !quit (s));

Why timeout after a second? Just don't specify a timeout at all, and let
select() block until there's data. Otherwise you wake up the CPU for no
good reason.

> +      if (quit (s))
> +	break;

What does this do? The name "quit" is an imperative, which would imply
that the process would end. Only it doesn't, apparently, because you
have an if() and a break; statement around it. This is confusing.

(also, indentation is *totally* wrong, because someone swapped tabs and
spaces. Here's why GNU indentation rules suck :-)

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