[PATCH 0/5] Add systemd socket launching support.

[PATCH 0/5] Add systemd socket launching support.

[Matthew Leach]

Systemd has the ability to create a socket, launch a daemon upon
connection to that socket and hand over the socket to the new process
to handle.  In Emacs' case, we can delay the start of the Emacs daemon
until emacsclient is called upon a socket.

These patches can be tested by creating minimal systemd setup.  First,
create two systemd unit files:

emacs.socket
============
[Socket]
ListenStream=/home/foobar/test.socket

[Install]
WantedBy=sockets.target

emacs.service
=============
[Service]
Type=forking
ExecStart=/path/to/emacs --daemon

Once these files have been created, execute:

# systemctl daemon-reload
# systemctl start enacs.socket

The socket will now be created, and when a connection is made, the
Emacs daemon will be started:

# emacsclient -s /home/foobar/test.socket -t

Feedback & comments welcome!

Thanks,
Matt

Matthew Leach (5):
  Check for libsystemd when building Emacs.
  Read the number of sockets passed by systemd.
  Permit systemd-allocated socket file-descriptors to be used.
  Allow the systed socket fd to be retrieved.
  When set, use the systemd socket descriptor.

 configure.ac    | 13 +++++++++++++
 lisp/server.el  |  5 ++++-
 src/Makefile.in |  6 +++++-
 src/emacs.c     | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 src/process.c   | 28 +++++++++++++++++++++-------
 5 files changed, 90 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH 1/5] Check for libsystemd when building Emacs.

[Matthew Leach]

* configure.ac: Add new default-on option systemd and check for
  necessary systemd libraries at configure time.
* src/Makefile.in: Add libsystemd library and C flags to the Emacs
  compilation options.
---
 configure.ac    | 13 +++++++++++++
 src/Makefile.in |  6 +++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index f3846f4..d3628d2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -330,6 +330,7 @@ OPTION_DEFAULT_ON([tiff],[don't compile with TIFF image support])
 OPTION_DEFAULT_ON([gif],[don't compile with GIF image support])
 OPTION_DEFAULT_ON([png],[don't compile with PNG image support])
 OPTION_DEFAULT_ON([rsvg],[don't compile with SVG image support])
+OPTION_DEFAULT_ON([systemd],[don't compile with systemd support])
 OPTION_DEFAULT_OFF([cairo],[compile with Cairo drawing (experimental)])
 OPTION_DEFAULT_ON([xml2],[don't compile with XML parsing support])
 OPTION_DEFAULT_ON([imagemagick],[don't compile with ImageMagick image support])
@@ -2716,6 +2717,18 @@ fi
 AC_SUBST(LIBGNUTLS_LIBS)
 AC_SUBST(LIBGNUTLS_CFLAGS)
 
+HAVE_SYSTEMD=no
+if test "${with_systemd}" = "yes" ; then
+  EMACS_CHECK_MODULES([LIBSYSTEMD], [libsystemd >= 226],
+    [HAVE_SYSTEMD=yes], [HAVE_SYSTEMD=no])
+  if test "${HAVE_SYSTEMD}" = "yes"; then
+    AC_DEFINE(HAVE_SYSTEMD, 1, [Define if using systemd.])
+  fi
+fi
+
+AC_SUBST(LIBSYSTEMD_LIBS)
+AC_SUBST(LIBSYSTEMD_CFLAGS)
+
 NOTIFY_OBJ=
 NOTIFY_SUMMARY=no
 
diff --git a/src/Makefile.in b/src/Makefile.in
index c290a60..fc9360a 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -307,6 +307,9 @@ LIBSELINUX_LIBS = @LIBSELINUX_LIBS@
 LIBGNUTLS_LIBS = @LIBGNUTLS_LIBS@
 LIBGNUTLS_CFLAGS = @LIBGNUTLS_CFLAGS@
 
+LIBSYSTEMD_LIBS = @LIBSYSTEMD_LIBS@
+LIBSYSTEMD_CFLAGS = @LIBSYSTEMD_CFLAGS@
+
 INTERVALS_H = dispextern.h intervals.h composite.h
 
 GETLOADAVG_LIBS = @GETLOADAVG_LIBS@
@@ -372,6 +375,7 @@ ALL_CFLAGS=-Demacs $(MYCPPFLAGS) -I. -I$(srcdir) \
   $(WEBKIT_CFLAGS) \
   $(SETTINGS_CFLAGS) $(FREETYPE_CFLAGS) $(FONTCONFIG_CFLAGS) \
   $(LIBOTF_CFLAGS) $(M17N_FLT_CFLAGS) $(DEPFLAGS) \
+  $(LIBSYSTEMD_CFLAGS) \
   $(LIBGNUTLS_CFLAGS) $(NOTIFY_CFLAGS) $(CAIRO_CFLAGS) \
   $(WARN_CFLAGS) $(WERROR_CFLAGS) $(CFLAGS)
 ALL_OBJC_CFLAGS=$(ALL_CFLAGS) $(GNU_OBJC_CFLAGS)
@@ -489,7 +493,7 @@ LIBES = $(LIBS) $(W32_LIBS) $(LIBS_GNUSTEP) $(LIBX_BASE) $(LIBIMAGE) \
    $(LIBS_TERMCAP) $(GETLOADAVG_LIBS) $(SETTINGS_LIBS) $(LIBSELINUX_LIBS) \
    $(FREETYPE_LIBS) $(FONTCONFIG_LIBS) $(LIBOTF_LIBS) $(M17N_FLT_LIBS) \
    $(LIBGNUTLS_LIBS) $(LIB_PTHREAD) $(GETADDRINFO_A_LIBS) \
-   $(NOTIFY_LIBS) $(LIB_MATH) $(LIBZ) $(LIBMODULES)
+   $(NOTIFY_LIBS) $(LIB_MATH) $(LIBZ) $(LIBMODULES) $(LIBSYSTEMD_LIBS)
 
 $(leimdir)/leim-list.el: bootstrap-emacs$(EXEEXT)
 	$(MAKE) -C ../leim leim-list.el EMACS="$(bootstrap_exe)"
-- 
2.7.4

[PATCH 2/5] Read the number of sockets passed by systemd.

[Matthew Leach]

* src/emacs.c (systemd_socket): New variable for storing the number
  of sockets passed by systemd.
  (main): Call sd_listen_fds to read the number of sockets passed.
  (Fsystemd_socket): New lisp function that returns `t' when systemd
  did pass Emacs a socket.
---
 src/emacs.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/src/emacs.c b/src/emacs.c
index 95d1905..5a87bcd 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -56,6 +56,10 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <binary-io.h>
 #endif
 
+#ifdef HAVE_SYSTEMD
+#include <systemd/sd-daemon.h>
+#endif /* HAVE_SYSTEMD */
+
 #ifdef HAVE_WINDOW_SYSTEM
 #include TERM_HEADER
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -195,6 +199,11 @@ int daemon_pipe[2];
 HANDLE w32_daemon_event;
 #endif
 
+#ifdef HAVE_SYSTEMD
+/* The number of sockets passed by systemd. */
+int systemd_socket = 0;
+#endif /* HAVE_SYSTEMD */
+
 /* Save argv and argc.  */
 char **initial_argv;
 int initial_argc;
@@ -997,6 +1006,15 @@ main (int argc, char **argv)
 	  exit (1);
 	}
 
+#ifdef HAVE_SYSTEMD
+      /* Read the number of sockets passed through by systemd. */
+      systemd_socket = sd_listen_fds(0);
+
+      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");
+#endif
+
 #ifndef DAEMON_MUST_EXEC
 #ifdef USE_GTK
       fprintf (stderr, "\nWarning: due to a long standing Gtk+ bug\nhttp://bugzilla.gnome.org/show_bug.cgi?id=85715\n\
@@ -2335,6 +2353,21 @@ If the daemon was given a name argument, return that name. */)
     return Qnil;
 }
 
+DEFUN ("systemd-socket", Fsystemd_socket, Ssystemd_socket, 0, 0, 0,
+       doc: /* Returns non-nil if systemd passed a socket through.
+When systemd passes a socket through to emacs, return `t'.*/)
+  (void)
+{
+#ifdef HAVE_SYSTEMD
+    if (systemd_socket)
+        return Qt;
+    else
+        return Qnil;
+#else
+    return Qnil;
+#endif
+}
+
 DEFUN ("daemon-initialized", Fdaemon_initialized, Sdaemon_initialized, 0, 0, 0,
        doc: /* Mark the Emacs daemon as being initialized.
 This finishes the daemonization process by doing the other half of detaching
@@ -2403,6 +2436,7 @@ syms_of_emacs (void)
   defsubr (&Sinvocation_name);
   defsubr (&Sinvocation_directory);
   defsubr (&Sdaemonp);
+  defsubr (&Ssystemd_socket);
   defsubr (&Sdaemon_initialized);
 
   DEFVAR_LISP ("command-line-args", Vcommand_line_args,
-- 
2.7.4

[PATCH 3/5] Permit systemd-allocated socket file-descriptors to be used.

[Matthew Leach]

* src/process.c (connect_network_socket): Allow a systemd-allocated
  file-descriptor to be passed, and use it, avoiding the call to
  socket() and bind().
  (Fmake_network_process): Allow users to pass in :systemd-fd on the
  parameter plist to use a systemd fd.
  (wait_reading_process_output): Call socket() & bind() every time.
  (syms_of_process): New symbol.
---
 src/process.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/src/process.c b/src/process.c
index 198e7de..b6bc757 100644
--- a/src/process.c
+++ b/src/process.c
@@ -3075,7 +3075,8 @@ finish_after_tls_connection (Lisp_Object proc)
 #endif
 
 static void
-connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses)
+connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses,
+                        Lisp_Object systemd_fd)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
   ptrdiff_t count1;
@@ -3089,6 +3090,8 @@ connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses)
   struct Lisp_Process *p = XPROCESS (proc);
   Lisp_Object contact = p->childp;
   int optbits = 0;
+  int use_systemd_fd = !NILP (systemd_fd) && INTEGERP (systemd_fd) &&
+      XINT (systemd_fd) != 0;
 
   /* Do this in case we never enter the while-loop below.  */
   count1 = SPECPDL_INDEX ();
@@ -3109,7 +3112,11 @@ connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses)
       sa = xmalloc (addrlen);
       conv_lisp_to_sockaddr (family, ip_address, sa, addrlen);
 
-      s = socket (family, p->socktype | SOCK_CLOEXEC, p->ai_protocol);
+      if (use_systemd_fd)
+          s = XINT (systemd_fd);
+      else
+          s = socket (family, p->socktype | SOCK_CLOEXEC, p->ai_protocol);
+
       if (s < 0)
 	{
 	  xerrno = errno;
@@ -3168,8 +3175,11 @@ connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses)
 		  report_file_error ("Cannot set reuse option on server socket", Qnil);
 	      }
 
-	  if (bind (s, sa, addrlen))
-	    report_file_error ("Cannot bind server socket", Qnil);
+          /* If we are passed in an fd from systemd, it is already
+             bound. */
+	  if (!use_systemd_fd)
+	    if (bind (s, sa, addrlen))
+	      report_file_error ("Cannot bind server socket", Qnil);
 
 #ifdef HAVE_GETSOCKNAME
 	  if (p->port == 0)
@@ -3534,6 +3544,8 @@ The following network options can be specified for this connection:
                       (this is allowed by default for a server process).
 :bindtodevice NAME -- bind to interface NAME.  Using this may require
                       special privileges on some systems.
+:systemd-fd INT    -- use this file-descriptor, passed in through systemd,
+                      and don't call socket() and bind().
 
 Consult the relevant system programmer's manual pages for more
 information on using these options.
@@ -3577,7 +3589,7 @@ usage: (make-network-process &rest ARGS)  */)
 #endif
   EMACS_INT port = 0;
   Lisp_Object tem;
-  Lisp_Object name, buffer, host, service, address;
+  Lisp_Object name, buffer, host, service, address, systemd_fd;
   Lisp_Object filter, sentinel;
   Lisp_Object ip_addresses = Qnil;
   int socktype;
@@ -3618,6 +3630,7 @@ usage: (make-network-process &rest ARGS)  */)
   buffer = Fplist_get (contact, QCbuffer);
   filter = Fplist_get (contact, QCfilter);
   sentinel = Fplist_get (contact, QCsentinel);
+  systemd_fd = Fplist_get (contact, QCsystemd_fd);
 
   CHECK_STRING (name);
 
@@ -3914,7 +3927,7 @@ usage: (make-network-process &rest ARGS)  */)
     }
 #endif
 
-  connect_network_socket (proc, ip_addresses);
+  connect_network_socket (proc, ip_addresses, systemd_fd);
   return proc;
 }
 
@@ -4848,7 +4861,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 		  {
 		    Lisp_Object ip_addresses = check_for_dns (aproc);
 		    if (!NILP (ip_addresses) && !EQ (ip_addresses, Qt))
-		      connect_network_socket (aproc, ip_addresses);
+		      connect_network_socket (aproc, ip_addresses, Qnil);
 		    else
 		      retry_for_async = true;
 		  }
@@ -7837,6 +7850,7 @@ syms_of_process (void)
   DEFSYM (QCserver, ":server");
   DEFSYM (QCnowait, ":nowait");
   DEFSYM (QCsentinel, ":sentinel");
+  DEFSYM (QCsystemd_fd, ":systemd-fd");
   DEFSYM (QCtls_parameters, ":tls-parameters");
   DEFSYM (Qnsm_verify_connection, "nsm-verify-connection");
   DEFSYM (QClog, ":log");
-- 
2.7.4

[PATCH 4/5] Allow the systed socket fd to be retrieved.

[Matthew Leach]

* src/emacs.c (Fsystemd_socket_fd): New.
---
 src/emacs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/emacs.c b/src/emacs.c
index 5a87bcd..da869af 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -2368,6 +2368,18 @@ When systemd passes a socket through to emacs, return `t'.*/)
 #endif
 }
 
+DEFUN ("systemd-socket-fd", Fsystemd_socket_fd, Ssystemd_socket_fd, 0, 0, 0,
+       doc: /* Returns the fd of the socket that systemd will pass through.
+When systemd support is not enabled, return nil.*/)
+  (void)
+{
+#ifdef HAVE_SYSTEMD
+    return make_number (SD_LISTEN_FDS_START);
+#else
+    return Qnil;
+#endif
+}
+
 DEFUN ("daemon-initialized", Fdaemon_initialized, Sdaemon_initialized, 0, 0, 0,
        doc: /* Mark the Emacs daemon as being initialized.
 This finishes the daemonization process by doing the other half of detaching
@@ -2437,6 +2449,7 @@ syms_of_emacs (void)
   defsubr (&Sinvocation_directory);
   defsubr (&Sdaemonp);
   defsubr (&Ssystemd_socket);
+  defsubr (&Ssystemd_socket_fd);
   defsubr (&Sdaemon_initialized);
 
   DEFVAR_LISP ("command-line-args", Vcommand_line_args,
-- 
2.7.4

[PATCH 5/5] When set, use the systemd socket descriptor.

[Matthew Leach]

* lisp/server.el (server-start): Pass in the systemd-fd number when
  calling `make-network-process'.
---
 lisp/server.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/server.el b/lisp/server.el
index 5243820..b3e24b1 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -609,7 +609,9 @@ To force-start a server, do \\[server-force-delete] and then
 	     (t (yes-or-no-p
 		 "The current server still has clients; delete them? "))))
     (let* ((server-dir (if server-use-tcp server-auth-dir server-socket-dir))
-	   (server-file (expand-file-name server-name server-dir)))
+	   (server-file (expand-file-name server-name server-dir))
+           (systemd-fd (when (systemd-socket)
+                         (systemd-socket-fd))))
       (when server-process
 	;; kill it dead!
 	(ignore-errors (delete-process server-process)))
@@ -655,6 +657,7 @@ server or call `\\[server-force-delete]' to forcibly disconnect it."))
 		       :noquery t
 		       :sentinel #'server-sentinel
 		       :filter #'server-process-filter
+		       :systemd-fd systemd-fd
 		       ;; We must receive file names without being decoded.
 		       ;; Those are decoded by server-process-filter according
 		       ;; to file-name-coding-system.  Also don't get
-- 
2.7.4

Re: [PATCH 0/5] Add systemd socket launching support.

[Alan Mackenzie]

Hello, Matthew.

On Sat, Mar 26, 2016 at 09:16:37PM +0000, Matthew Leach wrote:
> Systemd has the ability to create a socket, launch a daemon upon
> connection to that socket and hand over the socket to the new process
> to handle.  In Emacs' case, we can delay the start of the Emacs daemon
> until emacsclient is called upon a socket.

Can we really?  What's the purpose of such a delay?

[ .... ]

> Feedback & comments welcome!

Systemd is a system initialisation program, one of several available.
It is highly controversial, and it is fairly obvious that one of its
aims is to subvert GNU's central position in the GNU/Linux system.  One
of its methods of proliferation is to persuade maintainers to code in
direct dependencies on systemd, thus imposing itself on anybody wishing
to run the program.  (Witness Gnome.) Emacs is a core GNU product, and
should remain equally accessible to all GNU users.

Emacs should not be talking directly to init systems.  There are
perfectly good, portable system level interfaces for talking to sockets,
and Emacs should restrict itself to using them.

Should your patch be accepted, it would lead to an Emacs version
specific to an initialisation system, a degree of fragmentation we've
not had before, as far as I know.

And the idea of making systemd dependency a default for building Emacs
seems somewhat on the far side of acceptability.

My vote is for not putting initialisation system specific code into
Emacs.

> Thanks,
> Matt

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: [PATCH 0/5] Add systemd socket launching support.

[Alexis]

Alan Mackenzie <acm@muc.de> writes:

> There are 
> perfectly good, portable system level interfaces for talking to 
> sockets, and Emacs should restrict itself to using them.

Well, my understanding is that systemd activation sockets are not 
network sockets (or domain sockets, for that matter).


Alexis.

Re: [PATCH 0/5] Add systemd socket launching support.

[Matthew Leach]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Matthew.

Hi Alan,

> On Sat, Mar 26, 2016 at 09:16:37PM +0000, Matthew Leach wrote:
>> Systemd has the ability to create a socket, launch a daemon upon
>> connection to that socket and hand over the socket to the new process
>> to handle.  In Emacs' case, we can delay the start of the Emacs daemon
>> until emacsclient is called upon a socket.
>
> Can we really?  What's the purpose of such a delay?

Currently, I start emacs --daemon upon login of my user and this can
delay the login process by a few seconds as Emacs initialises.  The idea
behind socket activation is that, systemd knows about the socket's
existence and won't unmount any file-systems where the socket resides
until the daemon has shut down, this is causing a race condition on
reboot for me.  Also, allows the start of emacs --daemon to be on-demand
and so there is less of a delay at login.

> Systemd is a system initialisation program, one of several available.
> It is highly controversial, and it is fairly obvious that one of its
> aims is to subvert GNU's central position in the GNU/Linux system.

Systemd is licensed under the GNU Lesser General Public License, [1],
and I don't see the harm in a small amount integration with another free
software package.  I can't comment on the controversies that you have
raised here as I don't know about them - I'm just trying to fix a
problem that exists with Emacs on my system.

> One of its methods of proliferation is to persuade maintainers to code
> in direct dependencies on systemd, thus imposing itself on anybody
> wishing to run the program.  (Witness Gnome.) Emacs is a core GNU
> product, and should remain equally accessible to all GNU users.

The proposed systemd integration can be disabled with --without-systemd
at configure time.  Maybe we could make this option disabled by default
as a compromise?  However, even If Emacs is compiled with systemd
support, this change does not affect the current behaviour of Emacs.

> Emacs should not be talking directly to init systems.  There are
> perfectly good, portable system level interfaces for talking to sockets,
> and Emacs should restrict itself to using them.

Given that Emacs becomes larger every day and can behave as a daemon
process, I feel that giving users more options to control when and how
Emacs starts up is a worthwhile endeavour.

> Should your patch be accepted, it would lead to an Emacs version
> specific to an initialisation system, a degree of fragmentation we've
> not had before, as far as I know.
>
> And the idea of making systemd dependency a default for building Emacs
> seems somewhat on the far side of acceptability.

This patch does *not* make systemd a dependency of Emacs.  Again, this
patch provides a small degree of integration which can easily be
disabled and doesn't affect the current Emacs behaviour if enabled.


[1]: https://freedesktop.org/wiki/Software/systemd/

Thanks,
-- 
Matt

Re: [PATCH 0/5] Add systemd socket launching support.

[Matthew Leach]

Hi Alexis,

Alexis <flexibeast@gmail.com> writes:

> Alan Mackenzie <acm@muc.de> writes:
>
>> There are perfectly good, portable system level interfaces for
>> talking to sockets, and Emacs should restrict itself to using them.
>
> Well, my understanding is that systemd activation sockets are not
> network sockets (or domain sockets, for that matter).

I believe that systemd does create the actual socket requested (whether
it be AF_INET or AF_LOCAL, etc) and once a connection is made upon that
socket, systemd will invoke the requested process and hand over the
socket.  See sd_listen_fds(3) and systemd.socket(5).

Thanks,
-- 
Matt

Re: [PATCH 0/5] Add systemd socket launching support.

[Alexis]

Matthew Leach <matthew@mattleach.net> writes:

> I believe that systemd does create the actual socket requested 
> (whether it be AF_INET or AF_LOCAL, etc) and once a connection 
> is made upon that socket, systemd will invoke the requested 
> process and hand over the socket.  See sd_listen_fds(3) and 
> systemd.socket(5).

Ah okay - thanks!


Alexis.

Re: [PATCH 3/5] Permit systemd-allocated socket file-descriptors to be used.

[Lars Magne Ingebrigtsen]

Having systemd support in Emacs is probably nice, I think, but one
comment on this part of the patch series:

Matthew Leach <matthew@mattleach.net> writes:

> * src/process.c (connect_network_socket): Allow a systemd-allocated
>   file-descriptor to be passed, and use it, avoiding the call to
>   socket() and bind().

[...]

> +connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses,
> +                        Lisp_Object systemd_fd)

[...]

> -      s = socket (family, p->socktype | SOCK_CLOEXEC, p->ai_protocol);
> +      if (use_systemd_fd)
> +          s = XINT (systemd_fd);
> +      else
> +          s = socket (family, p->socktype | SOCK_CLOEXEC, p->ai_protocol);
> +

[...]

> +:systemd-fd INT    -- use this file-descriptor, passed in through systemd,
> +                      and don't call socket() and bind().

There isn't really anything systemd-specific about any of this code (as
far as I can see): It's about calling `make-network-socket' on a file
descriptor that's already been opened.  So I'd just rename this
parameter to :allocated-file-descriptor or something and adjust the
names of the other variables used.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH 3/5] Permit systemd-allocated socket file-descriptors to be used.

[Matthew Leach]

Hi Lars,

Thanks for the review.

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

[...]

>> +:systemd-fd INT    -- use this file-descriptor, passed in through systemd,
>> +                      and don't call socket() and bind().
>
> There isn't really anything systemd-specific about any of this code (as
> far as I can see): It's about calling `make-network-socket' on a file
> descriptor that's already been opened.  So I'd just rename this
> parameter to :allocated-file-descriptor or something and adjust the
> names of the other variables used.

Initially I had the same thought and gave this variable a generic name
as you propose.  However, it turns out that systemd has already called
bind() on the socket and a further call to bind() would error.  At this
point I thought this interface was too systemd-specific to allow
non-systemd allocated file-descriptors to be passed in.

I could always go back to a generic interface, but stipulate that bind()
must have already been called on the socket?

Thanks,
-- 
Matt

Re: [PATCH 3/5] Permit systemd-allocated socket file-descriptors to be used.

[Andreas Schwab]

Matthew Leach <matthew@mattleach.net> writes:

> Initially I had the same thought and gave this variable a generic name
> as you propose.  However, it turns out that systemd has already called
> bind() on the socket and a further call to bind() would error.

Doesn't xinetd do the same?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 3/5] Permit systemd-allocated socket file-descriptors to be used.

[Lars Magne Ingebrigtsen]

Matthew Leach <matthew@mattleach.net> writes:

> I could always go back to a generic interface, but stipulate that bind()
> must have already been called on the socket?

Yes, I think that would make most sense...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH 3/5] Permit systemd-allocated socket file-descriptors to be used.

[Matthew Leach]

Hi Andreas,

Andreas Schwab <schwab@linux-m68k.org> writes:

> Matthew Leach <matthew@mattleach.net> writes:
>
>> Initially I had the same thought and gave this variable a generic name
>> as you propose.  However, it turns out that systemd has already called
>> bind() on the socket and a further call to bind() would error.
>
> Doesn't xinetd do the same?

I'm not sure.  I would guess that you're correct, as bind() must be
called for connection detection.  I've never looked at xinetd, though.

Thanks,
-- 
Matt

Re: [PATCH 3/5] Permit systemd-allocated socket file-descriptors to be used.

[Matthew Leach]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Matthew Leach <matthew@mattleach.net> writes:
>
>> I could always go back to a generic interface, but stipulate that bind()
>> must have already been called on the socket?
>
> Yes, I think that would make most sense...

That does sound like the better option - I'll include that on a V2.

Thanks,
-- 
Matt

Re: [PATCH 0/5] Add systemd socket launching support.

[Wolfgang Jenkner]

On Sun, Mar 27 2016, Matthew Leach wrote:

> The idea
> behind socket activation is that, systemd knows about the socket's
> existence and won't unmount any file-systems where the socket resides
> until the daemon has shut down, this is causing a race condition on
> reboot for me. 

So what's the problem?  A stale socket?

Wolfgang

Re: [PATCH 0/5] Add systemd socket launching support.

[Matthew Leach]

Wolfgang Jenkner <wjenkner@inode.at> writes:

> On Sun, Mar 27 2016, Matthew Leach wrote:
>
>> The idea
>> behind socket activation is that, systemd knows about the socket's
>> existence and won't unmount any file-systems where the socket resides
>> until the daemon has shut down, this is causing a race condition on
>> reboot for me. 
>
> So what's the problem?  A stale socket?

Emacs creates the server socket somewhere in /tmp.  What I think is
happening is when rebooting systemd unmounts /tmp and emacsclient can't
send "(kill-emacs)" to the server.  This results in the emacs daemon
process not being shutdown and systemd delaying the shutdown/reboot by
90 seconds.

Thanks,
-- 
Matt

Re: [PATCH 0/5] Add systemd socket launching support.

[Wolfgang Jenkner]

On Sun, Mar 27 2016, Matthew Leach wrote:

> Emacs creates the server socket somewhere in /tmp.  What I think is
> happening is when rebooting systemd unmounts /tmp and emacsclient can't
> send "(kill-emacs)" to the server.  This results in the emacs daemon
> process not being shutdown and systemd delaying the shutdown/reboot by
> 90 seconds.

With a normal init, shutdown would send SIGTERM to the remaining
processes before trying to umount file systems...

Wolfgang

Re: [PATCH 0/5] Add systemd socket launching support.

[Eli Zaretskii]

> From: Matthew Leach <matthew@mattleach.net>
> Date: Sat, 26 Mar 2016 21:16:37 +0000
> Cc: Matthew Leach <matthew@mattleach.net>
> 
> Feedback & comments welcome!

Thank you for working on this.  Some specific comments below.  In
addition, please provide changes for NEWS and for the 2 manuals
describing this new feature.

> +#ifdef HAVE_SYSTEMD
> +      /* Read the number of sockets passed through by systemd. */
> +      systemd_socket = sd_listen_fds(0);

Isn't it prudent to test the socket descriptor for validity?  What if
it isn't a socket, for example?

Also, is it really correct to call this function with zero as
argument?  Do we want subordinate Emacs processes to use the same
socket?

> +DEFUN ("systemd-socket", Fsystemd_socket, Ssystemd_socket, 0, 0, 0,
> +       doc: /* Returns non-nil if systemd passed a socket through.
> +When systemd passes a socket through to emacs, return `t'.*/)

This is a predicate, so its name should end in a "-p".

Also, this should be in process.c, not in emacs.c (if it's needed at
all, see below).

> * src/process.c (connect_network_socket): Allow a systemd-allocated
>   file-descriptor to be passed, and use it, avoiding the call to
>   socket() and bind().
>   (Fmake_network_process): Allow users to pass in :systemd-fd on the
>   parameter plist to use a systemd fd.
>   (wait_reading_process_output): Call socket() & bind() every time.

I'm not sure I understand the rationale for this design.  Why do we
need to drag the systemd socket through all the APIs, including
exposing its value to Lisp(!), when it is stored in an internal
variable that can be easily accessed by the low-level network-related
functions?  Why cannot we instead provide a boolean flag that just
tells these low-level functions to use that socket?

>   (syms_of_process): New symbol.

Please state the name of the new symbol in the log message entry.

> +  int use_systemd_fd = !NILP (systemd_fd) && INTEGERP (systemd_fd) &&

Is it valid for systemd_fd to be non-positive?  If not, then INTEGERP is
not the correct test here.  Also, if you test INTEGERP, the !NILP test
is redundant.

> +DEFUN ("systemd-socket-fd", Fsystemd_socket_fd, Ssystemd_socket_fd, 0, 0, 0,
> +       doc: /* Returns the fd of the socket that systemd will pass through.
> +When systemd support is not enabled, return nil.*/)
> +  (void)
> +{
> +#ifdef HAVE_SYSTEMD
> +    return make_number (SD_LISTEN_FDS_START);
> +#else
> +    return Qnil;
> +#endif
> +}

Not sure I understand why we need both system-socket the predicate and
system-socket-fd.  Can't a single function serve both purposes?

Also, if this function is really needed (see the question above about
the overall design), its place is in process.c.

Re: [PATCH 0/5] Add systemd socket launching support.

[Matthew Leach]

Hi Eli,

Thanks for the feedback.

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matthew Leach <matthew@mattleach.net>
>> Date: Sat, 26 Mar 2016 21:16:37 +0000
>> Cc: Matthew Leach <matthew@mattleach.net>
>> 
>> Feedback & comments welcome!
>
> Thank you for working on this.  Some specific comments below.  In
> addition, please provide changes for NEWS and for the 2 manuals
> describing this new feature.

Will do.

>> +#ifdef HAVE_SYSTEMD
>> +      /* Read the number of sockets passed through by systemd. */
>> +      systemd_socket = sd_listen_fds(0);
>
> Isn't it prudent to test the socket descriptor for validity?  What if
> it isn't a socket, for example?

Agreed.  I'm thinking about trying getsockname as a test for validity as
this should also ensure that the socket is already bound.

> Also, is it really correct to call this function with zero as
> argument?  Do we want subordinate Emacs processes to use the same
> socket?

Agreed. Once this socket has been consumed we shouldn't pass it to any
subsequent processes.  I'll pass a '1' here.

>> +DEFUN ("systemd-socket", Fsystemd_socket, Ssystemd_socket, 0, 0, 0,
>> +       doc: /* Returns non-nil if systemd passed a socket through.
>> +When systemd passes a socket through to emacs, return `t'.*/)
>
> This is a predicate, so its name should end in a "-p".
>
> Also, this should be in process.c, not in emacs.c (if it's needed at
> all, see below).
>
>> * src/process.c (connect_network_socket): Allow a systemd-allocated
>>   file-descriptor to be passed, and use it, avoiding the call to
>>   socket() and bind().
>>   (Fmake_network_process): Allow users to pass in :systemd-fd on the
>>   parameter plist to use a systemd fd.
>>   (wait_reading_process_output): Call socket() & bind() every time.
>
> I'm not sure I understand the rationale for this design.  Why do we
> need to drag the systemd socket through all the APIs, including
> exposing its value to Lisp(!), when it is stored in an internal
> variable that can be easily accessed by the low-level network-related
> functions?  Why cannot we instead provide a boolean flag that just
> tells these low-level functions to use that socket?

Indeed, that does seem like a better approach.  I'm guessing the flag
you refer to should be passed to make_network_process from server-start?

>>   (syms_of_process): New symbol.
>
> Please state the name of the new symbol in the log message entry.
>
>> +  int use_systemd_fd = !NILP (systemd_fd) && INTEGERP (systemd_fd) &&
>
> Is it valid for systemd_fd to be non-positive?  If not, then INTEGERP is
> not the correct test here.  Also, if you test INTEGERP, the !NILP test
> is redundant.

Indeed, system_fd should always be positive.  I'll look for a better
predicate to use.

>> +DEFUN ("systemd-socket-fd", Fsystemd_socket_fd, Ssystemd_socket_fd, 0, 0, 0,
>> +       doc: /* Returns the fd of the socket that systemd will pass through.
>> +When systemd support is not enabled, return nil.*/)
>> +  (void)
>> +{
>> +#ifdef HAVE_SYSTEMD
>> +    return make_number (SD_LISTEN_FDS_START);
>> +#else
>> +    return Qnil;
>> +#endif
>> +}
>
> Not sure I understand why we need both system-socket the predicate and
> system-socket-fd.  Can't a single function serve both purposes?
>
> Also, if this function is really needed (see the question above about
> the overall design), its place is in process.c.

I think I'll re-work this code to remove all the lisp defuns as you say.

Thanks,
-- 
Matt

Re: [PATCH 0/5] Add systemd socket launching support.

[Eli Zaretskii]

> From: Matthew Leach <matthew@mattleach.net>
> Cc: emacs-devel@gnu.org
> Date: Sun, 27 Mar 2016 16:17:38 +0100
> 
> >> * src/process.c (connect_network_socket): Allow a systemd-allocated
> >>   file-descriptor to be passed, and use it, avoiding the call to
> >>   socket() and bind().
> >>   (Fmake_network_process): Allow users to pass in :systemd-fd on the
> >>   parameter plist to use a systemd fd.
> >>   (wait_reading_process_output): Call socket() & bind() every time.
> >
> > I'm not sure I understand the rationale for this design.  Why do we
> > need to drag the systemd socket through all the APIs, including
> > exposing its value to Lisp(!), when it is stored in an internal
> > variable that can be easily accessed by the low-level network-related
> > functions?  Why cannot we instead provide a boolean flag that just
> > tells these low-level functions to use that socket?
> 
> Indeed, that does seem like a better approach.  I'm guessing the flag
> you refer to should be passed to make_network_process from server-start?

Yes.

Thanks.

Re: [PATCH 0/5] Add systemd socket launching support.

[Mark Oteiza]

Matthew Leach <matthew@mattleach.net> writes:
> Currently, I start emacs --daemon upon login of my user and this can
> delay the login process by a few seconds as Emacs initialises.  The idea
> behind socket activation is that, systemd knows about the socket's
> existence and won't unmount any file-systems where the socket resides
> until the daemon has shut down, this is causing a race condition on
> reboot for me.  Also, allows the start of emacs --daemon to be on-demand
> and so there is less of a delay at login.

Out of curiosity, how are you starting it? I just fork it off with
`emacs --daemon &' and so don't have this problem.

Re: [PATCH 0/5] Add systemd socket launching support.

[Philipp Stephani]

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

Matthew Leach <matthew@mattleach.net> schrieb am So., 27. März 2016 um
17:17 Uhr:

> >> +#ifdef HAVE_SYSTEMD
> >> +      /* Read the number of sockets passed through by systemd. */
> >> +      systemd_socket = sd_listen_fds(0);
> >
> > Isn't it prudent to test the socket descriptor for validity?  What if
> > it isn't a socket, for example?
>
> Agreed.  I'm thinking about trying getsockname as a test for validity as
> this should also ensure that the socket is already bound.
>

The systemd library has some helper functions for that:
https://www.freedesktop.org/software/systemd/man/sd_is_fifo.html

[-- Attachment #2: Type: text/html, Size: 1029 bytes --]

Re: [PATCH 0/5] Add systemd socket launching support.

[Matthew Leach]

Hi Philipp,

Philipp Stephani <p.stephani2@gmail.com> writes:

> Matthew Leach <matthew@mattleach.net> schrieb am So., 27. März 2016 um 17:17 Uhr:
>
>  >> +#ifdef HAVE_SYSTEMD
>  >> + /* Read the number of sockets passed through by systemd. */
>  >> + systemd_socket = sd_listen_fds(0);
>  >
>  > Isn't it prudent to test the socket descriptor for validity? What if
>  > it isn't a socket, for example?
>
>  Agreed. I'm thinking about trying getsockname as a test for validity as
>  this should also ensure that the socket is already bound.
>
> The systemd library has some helper functions for that: https://www.freedesktop.org/software/systemd/man/sd_is_fifo.html

Thanks for the pointer - I didn't know about those functions :-).
-- 
Matt

Re: [PATCH 0/5] Add systemd socket launching support.

[Matthew Leach]

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

Hi Mark,

Mark Oteiza <mvoteiza@udel.edu> writes:

> Matthew Leach <matthew@mattleach.net> writes:
>> Currently, I start emacs --daemon upon login of my user and this can
>> delay the login process by a few seconds as Emacs initialises.  The idea
>> behind socket activation is that, systemd knows about the socket's
>> existence and won't unmount any file-systems where the socket resides
>> until the daemon has shut down, this is causing a race condition on
>> reboot for me.  Also, allows the start of emacs --daemon to be on-demand
>> and so there is less of a delay at login.
>
> Out of curiosity, how are you starting it? I just fork it off with
> `emacs --daemon &' and so don't have this problem.

Attached is my user's emacs.service file and my .kill-emacs.sh.  The sh
file is needed for an unrelated issue, namely that emacsclient exists
before the server is shut down and as far as systemd is concerned, once
ExecStop has completed the daemon should be shut down.  If it is not,
the default behaviour is to send it SIGKILL.

Thanks,
-- 
Matt

[-- Attachment #2: emacs.service --]
[-- Type: application/octet-stream, Size: 223 bytes --]

[Unit]
Description=Emacs: the extensible, self-documenting text editor

[Service]
Type=forking
ExecStart=/usr/local/bin/emacs --daemon
ExecStop=/home/matthew/.kill-emacs.sh
Restart=always

[Install]
WantedBy=default.target

[-- Attachment #3: .kill-emacs.sh --]
[-- Type: application/x-sh, Size: 191 bytes --]