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