From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Lars Ingebrigtsen Newsgroups: gmane.emacs.bugs Subject: bug#22493: 25.1.50; open-gnutls-stream doesn't respect :nowait, so the connections are synchronous Date: Wed, 03 Feb 2016 12:10:21 +1100 Message-ID: <87a8nizjia.fsf@gnus.org> References: <87mvrnzpge.fsf@gnus.org> <878u37zndq.fsf@gnus.org> <83r3gzwhg8.fsf@gnu.org> <87fuxebrsy.fsf@gnus.org> <878u36fung.fsf@gnus.org> <8360y93fka.fsf@gnu.org> <87wpqpwd8p.fsf@gnus.org> <83d1sh14is.fsf@gnu.org> <87egcx13kc.fsf@gnus.org> <83bn801d06.fsf@gnu.org> <87bn7znazd.fsf@gnus.org> <83lh73ytxo.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1454461889 11638 80.91.229.3 (3 Feb 2016 01:11:29 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 3 Feb 2016 01:11:29 +0000 (UTC) Cc: 22493@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Wed Feb 03 02:11:18 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aQlyV-0007s8-RM for geb-bug-gnu-emacs@m.gmane.org; Wed, 03 Feb 2016 02:11:16 +0100 Original-Received: from localhost ([::1]:60188 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQlyR-00056o-IJ for geb-bug-gnu-emacs@m.gmane.org; Tue, 02 Feb 2016 20:11:11 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:58408) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQlyM-00056W-Vd for bug-gnu-emacs@gnu.org; Tue, 02 Feb 2016 20:11:08 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQlyI-0002dv-Ts for bug-gnu-emacs@gnu.org; Tue, 02 Feb 2016 20:11:06 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:48693) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQlyI-0002dr-Pl for bug-gnu-emacs@gnu.org; Tue, 02 Feb 2016 20:11:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84) (envelope-from ) id 1aQlyI-0002PO-Ey for bug-gnu-emacs@gnu.org; Tue, 02 Feb 2016 20:11:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Lars Ingebrigtsen Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 03 Feb 2016 01:11:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 22493 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 22493-submit@debbugs.gnu.org id=B22493.14544618539243 (code B ref 22493); Wed, 03 Feb 2016 01:11:02 +0000 Original-Received: (at 22493) by debbugs.gnu.org; 3 Feb 2016 01:10:53 +0000 Original-Received: from localhost ([127.0.0.1]:57282 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aQly8-0002P1-Vh for submit@debbugs.gnu.org; Tue, 02 Feb 2016 20:10:53 -0500 Original-Received: from hermes.netfonds.no ([80.91.224.195]:45722) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aQly6-0002Os-Iy for 22493@debbugs.gnu.org; Tue, 02 Feb 2016 20:10:51 -0500 Original-Received: from cpe-60-225-211-161.nsw.bigpond.net.au ([60.225.211.161] helo=mouse) by hermes.netfonds.no with esmtpsa (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.72) (envelope-from ) id 1aQlxj-00008x-D5; Wed, 03 Feb 2016 02:10:28 +0100 In-Reply-To: <83lh73ytxo.fsf@gnu.org> (Eli Zaretskii's message of "Tue, 02 Feb 2016 18:10:27 +0200") User-Agent: Gnus/5.130014 (Ma Gnus v0.14) Emacs/25.1.50 (gnu/linux) X-MailScanner-ID: 1aQlxj-00008x-D5 MailScanner-NULL-Check: 1455066628.15333@fM7J22Wc5klT5KIQzNtEgQ X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:112312 Archived-At: Eli Zaretskii writes: >> Sure. But the wait is much shorter. > > If it really is, then I must be missing something, because I don't see > how it could. Can you show some timings, comparing the "old" and the > "new" methods for making a GnuTLS connection? You don't see how taking the DNS resolution out of the "stop everything Emacs is doing" bit makes Emacs stop everything it's doing for a shorter while? I think I must be misunderstanding you... > And if somehow it indeed is shorter, the same effect can be achieved > by running the GnuTLS negotiation from an idle timer, something that > would most probably avoid most or all of the complexities on the C > level. Have you considered this alternative? Sure, but we were talking about the DNS resolution... > This will have to be optional, since Emacs might not become idle for a > prolonged time. IOW, only certain applications will want to benefit > from such a background handshake, some will need to wait for its > completion. So if you make this be a background thing by default, you > might break existing users that don't expect the negotiation to return > before it's completed or failed. Sure. The optional part here is that the user says :nowait when they don't want to wait, so I'm not sure I'm getting your objection here, either. > Thanks for working on this. Please allow me some review comments > based on what's on the branch now. > > +@item :tls-parameters > +When opening a TLS connection, this should be where the first element > +is the TLS type, > > This should tell what values are acceptable for TYPE, since no other > text around this one hints on that. Ok, fixed. > and the remaining elements should form a keyword list > +acceptable for @code{gnutls-boot}. The TLS connection will then be > +negotiated after completing the connection to the host. > > This should tell how to obtain those parameters. It's currently obtained from `gnutls-negotiate' with a :return-keywords parameter, but I think it would make sense to refactor that so that there's a separate function that computes the parameter. I'll fix that. > -@defun open-gnutls-stream name buffer host service > +@defun open-gnutls-stream name buffer host service &optional nowait > This function creates a buffer connected to a specific @var{host} and > @var{service} (port number or service name). The parameters and their > syntax are the same as those given to @code{open-network-stream} > > The meaning of 'nowait' should be explicitly described, since relying > on "the same as those given to 'open-gnutls-stream'" hides some > important information about the handshake in this case. Ok, fixed. > -(defun open-gnutls-stream (name buffer host service) > +(defun open-gnutls-stream (name buffer host service &optional nowait) > "Open a SSL/TLS connection for a service to a host. > Returns a subprocess-object to represent the connection. > Input and output work as for subprocesses; `delete-process' closes it. > @@ -109,6 +109,8 @@ BUFFER is the buffer (or `buffer-name') to associate with the process. > Third arg is name of the host to connect to, or its IP address. > Fourth arg SERVICE is name of the service desired, or an integer > specifying a port number to connect to. > +Fifth arg NOWAIT (which is optional) means that the socket should > +be opened asynchronously. > > This last sentence should say more explicitly what happens in that > case. Done. > It must be omitted, a number, or nil; if omitted or nil it > -defaults to GNUTLS_VERIFY_ALLOW_X509_V1_CA_CRT." > +defaults to GNUTLS_VERIFY_ALLOW_X509_V1_CA_CRT. > + > +If RETURN-KEYWORDS, don't connect to anything, but just return > +the computed parameters that we otherwise would be calling > +gnutls-boot with. The return value will be a list where the > +first element is the TLS type, and the rest of the list consists > +of the keywords." > > It's confusing to have a function named "open-gnutls-stream" that > doesn't really open any streams. So I'd rather have > open-gnutls-stream refactored into 2 functions, with the part that > computes the parameters made a separate function that can be called > directly. Then both open-gnutls-stream and the async code could call > that new function. Yup. :-) > +:tls-parameters is a list that should be supplied if you're > +opening a TLS connection. The first element is the TLS type, and > +the remaining elements should be a keyword list accepted by > +gnutls-boot." > > Please mention here how to obtain that keyword list. Will do. > Please add a comment saying when GETADDRINFO_A_LIBS is non-empty. > > +static void > +boot_error (struct Lisp_Process *p, const char *m, ...) > +{ > + va_list ap; > + va_start (ap, m); > + if (p->is_non_blocking_client) > + pset_status (p, Qfailed); > + else > + verror (m, ap); > > AFAIU, here we are losing the error information, which I think we > shouldn't. Why not keep it around and let the sentinel use it if it > wants to? Oh yeah, that's true. When I did that change I didn't know that a status could be a list that returned more error info. Fixed now. > +#ifdef HAVE_GNUTLS > + /* The TLS connection hasn't been set up yet, so we can't write > + anything on the socket. */ > + if (!NILP (p->gnutls_boot_parameters)) > + return; > +#endif > > I don't think this is a good idea: you are silently returning here > without doing anything; the callers of send_process won't expect > that. I think this should signal an error instead. It's perfectly fine to say (setq proc (make-network-process ... :nowait t :tls-parameters ...)) (process-send-string proc) It's not an error, and the idle loop will try resending it until it succeeds. (Which will only happen after DNS resolution and TLS negotiation has failed.) If the user has requested a TLS socket, then it's nonsensical to try sending data on it before it's completed the negotiation. > One final comment: I think this change will need addition of tests to > the test suite, to exercise the affected APIs, so we could make sure > we don't introduce any bugs in the existing functionalities. Unfortunately, there are virtually no network tests in the suite. At least I couldn't find any. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no