On Wed, May 04, 2016 at 10:22:34AM +0100, Alex Bligh wrote: > > On 4 May 2016, at 05:43, Wouter Verhelst <[email protected]> wrote: > > > 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? > > I said that I didn't want to introduce a dependency on pkg-config when none > currently existed, and I don't think I heard back.
Ah, that's possible :-) I think that's not something you should be worried about. pkg-config is pretty universal these days; also, it makes it (must) easier on the user to point to an alternative implementation of particular things. > But no great shakes. > > There's a simple answer to most of the stuff below: > > The code is not nbd code. It's code from: > https://github.com/abligh/tlsproxy > which is an MIT licensed TLS proxy written as an example for > GnuTLS (not specifically for nbd). I know that, but you're suggesting it be merged into the nbd repository. Once you do that, it becomes nbd code, and we'd rightfully be expected to maintain it here as well. > It's a byte-for-byte copy of the files, and I don't particularly fancy > maintaining two code bases. Then don't, but essentially you're saying I can now *also* not make changes to these files once it's merged into the nbd repository. Sorry, but that's not going to fly. If at some point I think nbd needs to do something different and it requires changes to some of the files that are part of tlsproxy and are in the nbd repository, I'm not going to hold back just because it's "external" code. If you make it a library (on which nbd is then going to depend) then okay, we could do that. I don't think this code is particularly useful as a library, but it's your call. At any rate, I do not want to limit the nbd TLS implementation to the capabilities of some random unrelated code that just happens to implement something close to what we want. Your tlsproxy is useful, and yes, it makes sense to use it for the client, but I am absolutely not convinced it's the right (long-term) approach to TLS in the server. > > [...] > >> + 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. > > Because I neither want to expose that from tlsproxy, nor (for obvious > reasons) have tlsproxy call nbd. Then make something common that both use? This is the sort of minor useful stuff that would be well served by a file containing some utility functions. > > [...] > >> + /* 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. > > In an ideal world, you would be right. However, you are presuming the > crypto library never has any bugs where something happens where it's > need for FDs being set writeable or readable is wrong (or > more specifically changes during the call). Painful experiences > with OpenSSL suggest doing the above is a very good idea, as it > swaps the possibility of a permanent lockup for a once per second > extra run through the select loop. Cases I found that failed on > some versions of OpenSSL revolved around some form of key renegotiation > (which happens after an hour or so) under heavy load. Mm, right, makes sense I suppose. [...] > >> + 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. > > Quit is a function. Perhaps it should be called 'shouldquit' but that's > what it's called in tlsproxy and I don't particularly want to change it. > > > (also, indentation is *totally* wrong, because someone swapped tabs and > > spaces. Here's why GNU indentation rules suck :-) > > Indentation I think you will find is totally right, because it > was run through GNU's 'indent' program. Unless something really odd > has happened! Point being, the if has six spaces, the break has a tab. The result is that (in this mail) the break indends less than the if. In your reply, it probably won't anymore. -- < 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
