Re: [PATCH v5] Add systemd socket launching support

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

Matthew Leach wrote:
> There is no specific reason other than 226 is what is installed on my
> system and what I have tested against.  If you are able to test on an
> earlier version, this number could be reduced.

Thanks. Can you please investigate which is the earliest number for which it's 
plausible that the new systemd code will work? In the meantime I installed the 
attached patch, which lists that as a FIXME and addresses the other problem I 
mentioned along with a few minor style glitches.

[-- Attachment #2: 0001-Minor-fixups-for-external-socket-launching.patch --]
[-- Type: text/x-diff, Size: 8164 bytes --]

From fd379e7ed99557bb4348e215bfc711af2d9360e8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 17 Apr 2016 22:41:14 -0700
Subject: [PATCH] Minor fixups for external socket launching

* configure.ac (HAVE_LIBSYSTEMD): Change earliest version to 222.
* doc/emacs/misc.texi (Emacs Server):
* etc/NEWS: Spelling and doc fixes.
* src/emacs.c (main) [HAVE_LIBSYSTEMD]:
Check for sd_is_socket returning positive, not zero.
* src/process.c (external_sock_fd): Instead of initializing here ...
(init_process_emacs): ... initialize it here, so that it does the
right thing after dump/restore.
(connect_network_socket): Simplify socket_to_use test.
---
 configure.ac        |  5 ++++-
 doc/emacs/misc.texi |  2 +-
 etc/NEWS            | 12 ++++++------
 src/emacs.c         | 26 ++++++++++++--------------
 src/process.c       | 35 ++++++++++++++++-------------------
 5 files changed, 39 insertions(+), 41 deletions(-)

diff --git a/configure.ac b/configure.ac
index 54e9ec5..0f6f650 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2735,7 +2735,10 @@ AC_DEFUN
 
 HAVE_LIBSYSTEMD=no
 if test "${with_libsystemd}" = "yes" ; then
-  EMACS_CHECK_MODULES([LIBSYSTEMD], [libsystemd >= 226],
+  dnl This code has been tested with libsystemd 222 and later.
+  dnl FIXME: Find the earliest version number for which Emacs should work,
+  dnl and change '222' to that number.
+  EMACS_CHECK_MODULES([LIBSYSTEMD], [libsystemd >= 222],
     [HAVE_LIBSYSTEMD=yes], [HAVE_LIBSYSTEMD=no])
   if test "${HAVE_LIBSYSTEMD}" = "yes"; then
     AC_DEFINE(HAVE_LIBSYSTEMD, 1, [Define if using libsystemd.])
diff --git a/doc/emacs/misc.texi b/doc/emacs/misc.texi
index 775cda9..20f2d66 100644
--- a/doc/emacs/misc.texi
+++ b/doc/emacs/misc.texi
@@ -1586,7 +1586,7 @@ Emacs Server
 An external process can invoke the Emacs server when a connection
 event occurs upon a specified socket and pass the socket to the new
 Emacs server process.  An instance of this is @command{systemd}'s
-socket functionaly: the @command{systemd} service creates a socket and
+socket functionality: the @command{systemd} service creates a socket and
 listens for connections on it; when @command{emacsclient} connects to
 it for the first time, @command{systemd} can launch the Emacs server
 and hand over the socket to it for servicing @command{emacsclient}
diff --git a/etc/NEWS b/etc/NEWS
index ecb52c1..2373347 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -32,12 +32,12 @@ the default in developer builds.  As before, use
 '--enable-gcc-warnings' to stop the build if GCC issues warnings.
 
 +++
-** Emacs server now has socket-launching support.  This allows socket
-based activation, where an external process can invoke the Emacs
-server process upon a socket connection event and hand over the socket
-to Emacs.  Emacs will use this socket for servicing emacsclient
-commands.  systemd can make use of this new functionally but can be
-disabled with the configure option '--disable-libsystemd'.
+** The Emacs server now has socket-launching support.  This allows
+socket based activation, where an external process like systemd can
+invoke the Emacs server process upon a socket connection event and
+hand the socket over to Emacs.  Emacs uses this socket to service
+emacsclient commands.  This new functionality can be disabled with the
+configure option '--disable-libsystemd'.
 
 ** New configure option '--disable-build-details' attempts to build an
 Emacs that is more likely to be reproducible; that is, if you build
diff --git a/src/emacs.c b/src/emacs.c
index a51df09..a738bac 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -57,9 +57,9 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #endif
 
 #ifdef HAVE_LIBSYSTEMD
-#include <systemd/sd-daemon.h>
-#include <sys/socket.h>
-#endif /* HAVE_LIBSYSTEMD */
+# include <systemd/sd-daemon.h>
+# include <sys/socket.h>
+#endif
 
 #ifdef HAVE_WINDOW_SYSTEM
 #include TERM_HEADER
@@ -681,9 +681,6 @@ main (int argc, char **argv)
   char dname_arg2[80];
 #endif
   char *ch_to_dir = 0;
-#ifdef HAVE_LIBSYSTEMD
-  int systemd_socket;
-#endif
 
   /* If we use --chdir, this records the original directory.  */
   char *original_pwd = 0;
@@ -1008,16 +1005,17 @@ main (int argc, char **argv)
 	}
 
 #ifdef HAVE_LIBSYSTEMD
-      /* Read the number of sockets passed through by systemd. */
-      systemd_socket = sd_listen_fds(1);
+      /* Read the number of sockets passed through by systemd.  */
+      int systemd_socket = sd_listen_fds (1);
 
       if (systemd_socket > 1)
-        fprintf (stderr, "\nWarning: systemd has passed more than one socket to the Emacs process.\n\
-Try adding 'Accept=false' in the Emacs socket unit file.\n");
-
-      else if (systemd_socket == 1 &&
-               sd_is_socket (SD_LISTEN_FDS_START,
-                             AF_UNSPEC, SOCK_STREAM, 1) >= 0)
+        fprintf (stderr,
+		 ("\n"
+		  "Warning: systemd passed more than one socket to Emacs.\n"
+		  "Try 'Accept=false' in the Emacs socket unit file.\n"));
+      else if (systemd_socket == 1
+	       && (0 < sd_is_socket (SD_LISTEN_FDS_START,
+				     AF_UNSPEC, SOCK_STREAM, 1)))
         set_external_socket_descriptor (SD_LISTEN_FDS_START);
 #endif /* HAVE_LIBSYSTEMD */
 
diff --git a/src/process.c b/src/process.c
index 2dfad66..a222a5b 100644
--- a/src/process.c
+++ b/src/process.c
@@ -267,8 +267,8 @@ static int max_process_desc;
 /* The largest descriptor currently in use for input; -1 if none.  */
 static int max_input_desc;
 
-/* The descriptor  of any sockets passed to Emacs; -1 if none. */
-static int external_sock_fd = -1;
+/* The descriptor of any socket passed to Emacs; -1 if none. */
+static int external_sock_fd;
 
 /* Indexed by descriptor, gives the process (if any) for that descriptor.  */
 static Lisp_Object chan_process[FD_SETSIZE];
@@ -3099,11 +3099,10 @@ connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses,
     {
       socket_to_use = external_sock_fd;
 
-      /* Ensure we don't consume the external socket twice. */
+      /* Ensure we don't consume the external socket twice.  */
       external_sock_fd = -1;
     }
 
-
   /* Do this in case we never enter the while-loop below.  */
   count1 = SPECPDL_INDEX ();
   s = -1;
@@ -3123,15 +3122,15 @@ connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses,
       sa = xmalloc (addrlen);
       conv_lisp_to_sockaddr (family, ip_address, sa, addrlen);
 
-      if (socket_to_use != -1)
-          s = socket_to_use;
-      else
-          s = socket (family, p->socktype | SOCK_CLOEXEC, p->ai_protocol);
-
+      s = socket_to_use;
       if (s < 0)
 	{
-	  xerrno = errno;
-	  continue;
+	  s = socket (family, p->socktype | SOCK_CLOEXEC, p->ai_protocol);
+	  if (s < 0)
+	    {
+	      xerrno = errno;
+	      continue;
+	    }
 	}
 
 #ifdef DATAGRAM_SOCKETS
@@ -3186,11 +3185,9 @@ connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses,
 		  report_file_error ("Cannot set reuse option on server socket", Qnil);
 	      }
 
-          /* If we are passed a socket descriptor, it should be
-             already bound. */
-	  if (socket_to_use == -1)
-	    if (bind (s, sa, addrlen))
-	      report_file_error ("Cannot bind server socket", Qnil);
+          /* If passed a socket descriptor, it should be already bound. */
+	  if (socket_to_use < 0 && bind (s, sa, addrlen) != 0)
+	    report_file_error ("Cannot bind server socket", Qnil);
 
 #ifdef HAVE_GETSOCKNAME
 	  if (p->port == 0)
@@ -7741,9 +7738,9 @@ catch_child_signal (void)
    `:use-external-socket' option.  The fd should have been checked to
    ensure it is a valid socket and is already bound.  */
 void
-set_external_socket_descriptor(int fd)
+set_external_socket_descriptor (int fd)
 {
-    external_sock_fd = fd;
+  external_sock_fd = fd;
 }
 
 \f
@@ -7775,7 +7772,7 @@ init_process_emacs (void)
   FD_ZERO (&non_keyboard_wait_mask);
   FD_ZERO (&non_process_wait_mask);
   FD_ZERO (&write_mask);
-  max_process_desc = max_input_desc = -1;
+  max_process_desc = max_input_desc = external_sock_fd = -1;
   memset (fd_callback_info, 0, sizeof (fd_callback_info));
 
   FD_ZERO (&connect_wait_mask);
-- 
2.5.5