Re: [Nicolas Graves] [PATCH v6 01/10] rde: emacs: Start emacs in --daemon mode, with shepherd and pid-file

[Nicolas Graves via "Development of GNU Guix and the GNU System distribution." <guix-devel@gnu.org>]

On 2024-05-12 09:50, Nicolas Graves wrote:

> On 2024-05-12 09:29, Eli Zaretskii wrote:
>
>>> Cc: Ludovic Courtès <ludo@gnu.org>, emacs-devel@gnu.org,
>>>  Andrew Tropin <andrew@trop.in>, guix-devel@gnu.org, bjorn.bidar@thaodan.de,
>>>  rudi@constantly.at, felix.lechner@lease-up.com
>>> Date: Sun, 12 May 2024 01:07:51 +0200
>>> From:  Nicolas Graves via "Emacs development discussions." <emacs-devel@gnu.org>
>>> 
>>> A lightly cleaned-up version attached.
>>
>> Thanks.  What was the motivation for this, again?
>
> A light summary (all is in the preceding exchanges in the mailing list):
>
> - Ludovic Courtès suggested this change because linking with systemd is
> unnecessary (very light usage), and increases the attack surface.
>
> - Rudolf Schlatte highlights that Lennart Poettering says that the
> notify protocol is stable and independent of libsystemd.
>           https://mastodon.social/@pid_eins/112202687764571433
>           https://mastodon.social/@pid_eins/112202696589971319
>   This mastondon thread itself contains a lot of info/answers about
>   this, including examples of similar work on other projects:
>   - https://github.com/FRRouting/frr/pull/8508
>   - https://github.com/OISF/suricata/pull/10757
>   Lennart Poettering also says that the protocol has been stable for a
>   decade and will surely be for at least another decade.

A bit more on the stability promise :
 This protocol is guaranteed to be stable as per:
 https://systemd.io/PORTABILITY_AND_STABILITY/
 
 sd_notify is definitely given as a stable interface. 

>
> This should also answer the worry about significant maintenance.
>
> What I'm less confident about is edge cases as I highlighted earlier
> (are there cases where systemd's safe_atoi is necessary compared to
> strtol ? Is it fine if LISTEN_FDS is set but LISTEN_PID unset, or
> should be check for LISTEN_PID definition too ?)
>
>>
>>>  configure.ac     |  19 +------
>>>  lib/Makefile.in  |   2 +-
>>>  lib/gnulib.mk.in |   2 -
>>>  lib/sd-socket.c  | 137 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  lib/sd-socket.h  |  57 ++++++++++++++++++++
>>>  msdos/sed1v2.inp |   3 --
>>>  src/Makefile.in  |   9 ++--
>>>  src/deps.mk      |   2 +-
>>>  src/emacs.c      |  50 ++++++++---------
>>>  9 files changed, 226 insertions(+), 55 deletions(-)
>>>  create mode 100644 lib/sd-socket.c
>>>  create mode 100644 lib/sd-socket.h
>>
>> Your code is not from Gnulib, so it is wrong to place it in lib/.  It
>> should be in src/, and probably just an addition to sysdep.c.  Then
>> many of the changes to the build infrastructure will not be needed.
>
> Thanks, I'll place that there.
>
>>
>>> -HAVE_LIBSYSTEMD=no
>>> -if test "${with_libsystemd}" = "yes" ; then
>>> -  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.])
>>> -  fi
>>> -fi
>>> -
>>> -AC_SUBST([LIBSYSTEMD_LIBS])
>>> -AC_SUBST([LIBSYSTEMD_CFLAGS])
>>
>> This test should be replaced by a test to determine whether the
>> systemd interface should be compiled into Emacs.  Not all of the
>> supported platform can use it, so we need to determine that at
>> configure time.
>>
> Thanks, will do.
>
>>> -
>>>  HAVE_JSON=no
>>>  JSON_OBJ=
>>>  
>>> @@ -6652,7 +6636,7 @@ AC_DEFUN
>>>  optsep=
>>>  emacs_config_features=
>>>  for opt in ACL BE_APP CAIRO DBUS FREETYPE GCONF GIF GLIB GMP GNUTLS GPM GSETTINGS \
>>> - HARFBUZZ IMAGEMAGICK JPEG JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 \
>>> + HARFBUZZ IMAGEMAGICK JPEG JSON LCMS2 LIBOTF LIBSELINUX LIBXML2 \
>>>   M17N_FLT MODULES NATIVE_COMP NOTIFY NS OLDXMENU PDUMPER PGTK PNG RSVG SECCOMP \
>>>   SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER \
>>>   UNEXEC WEBP X11 XAW3D XDBE XFT XIM XINPUT2 XPM XWIDGETS X_TOOLKIT \
>>> @@ -6721,7 +6705,6 @@ AC_DEFUN
>>>    Does Emacs use -lm17n-flt?                              ${HAVE_M17N_FLT}
>>>    Does Emacs use -lotf?                                   ${HAVE_LIBOTF}
>>>    Does Emacs use -lxft?                                   ${HAVE_XFT}
>>> -  Does Emacs use -lsystemd?                               ${HAVE_LIBSYSTEMD}
>>>    Does Emacs use -ljansson?                               ${HAVE_JSON}
>>>    Does Emacs use -ltree-sitter?                           ${HAVE_TREE_SITTER}
>>>    Does Emacs use the GMP library?                         ${HAVE_GMP}
>>
>> The above summary of the configuration should still call out the
>> result of the configure test for systemd interface, and I think the
>> list of features should include some string which tells us whether
>> systemd interface is supported.
>
> Understood. 
>>
>>> -#ifdef HAVE_LIBSYSTEMD
>>>        /* Read the number of sockets passed through by systemd.  */
>>> -      int systemd_socket = sd_listen_fds (1);
>>> -
>>> -      if (systemd_socket > 1)
>>> -        fputs (("\n"
>>> -		"Warning: systemd passed more than one socket to Emacs.\n"
>>> -		"Try 'Accept=false' in the Emacs socket unit file.\n"),
>>> -	       stderr);
>>> -      else if (systemd_socket == 1
>>> -	       && (0 < sd_is_socket (SD_LISTEN_FDS_START,
>>> -				     AF_UNSPEC, SOCK_STREAM, 1)))
>>> -	sockfd = SD_LISTEN_FDS_START;
>>> -#endif /* HAVE_LIBSYSTEMD */
>>> +      const char *fds = getenv("LISTEN_FDS");
>>> +      if (fds) {
>>> +	int systemd_socket = strtol(fds, NULL, 0);
>>> +	if (systemd_socket > 1)
>>> +	  fputs (("\n"
>>> +		  "Warning: systemd passed more than one socket to Emacs.\n"
>>> +		  "Try 'Accept=false' in the Emacs socket unit file.\n"),
>>> +		 stderr);
>>> +	else if (systemd_socket == 1
>>> +		 && (0 < sd_is_socket (SD_LISTEN_FDS_START, SOCK_STREAM, 1)))
>>> +	  sockfd = SD_LISTEN_FDS_START;
>>> +      }
>>
>> The new code to interface with systemd cannot be compiled
>> unconditionally, it should have some #ifdef condition, determined by
>> the configure script, because not all the platforms we support can use
>> systemd.
>
> Same point.
>>
>>> @@ -2857,12 +2854,15 @@ DEFUN ("kill-emacs", Fkill_emacs, Skill_emacs, 0, 2, "P",
>>>      }
>>>  #endif
>>>  
>>> -#ifdef HAVE_LIBSYSTEMD
>>>    /* Notify systemd we are shutting down, but only if we have notified
>>>       it about startup.  */
>>> -  if (daemon_type == -1)
>>> -    sd_notify(0, "STOPPING=1");
>>> -#endif /* HAVE_LIBSYSTEMD */
>>> +  if (daemon_type == -1){
>>> +    int r = sd_notify_stopping();
>>> +    if (r < 0) {
>>> +      fprintf(stderr, "Failed to report termination to $NOTIFY_SOCKET: %s\n", strerror(-r));
>>> +      exit (EXIT_FAILURE);
>>> +    }
>>> +  }
>>
>> Same here.
>>
>>>    /* Fsignal calls emacs_abort () if it sees that waiting_for_input is
>>>       set.  */
>>> @@ -3382,9 +3382,11 @@ DEFUN ("daemon-initialized", Fdaemon_initialized, Sdaemon_initialized, 0, 0, 0,
>>>  
>>>    if (daemon_type == 1)
>>>      {
>>> -#ifdef HAVE_LIBSYSTEMD
>>> -      sd_notify(0, "READY=1");
>>> -#endif /* HAVE_LIBSYSTEMD */
>>> +      int r = sd_notify_ready();
>>> +      if (r < 0) {
>>> +	fprintf(stderr, "Failed to notify readiness to $NOTIFY_SOCKET: %s\n", strerror(-r));
>>> +	exit (EXIT_FAILURE);
>>> +      }
>>>      }
>>
>> And here.
>>
>>> > Seems to work properly on my side with the following patch, on Guix with
>>> > the shepherd service I sent a few weeks ago. The patch is actually
>>> > pretty simple.
>>
>> How sure we are that the code you wrote will not need any significant
>> maintenance due to changes in how systemd works?  The significant
>> advantage of using libsystemd is that the systemd developers take care
>> of updating it as needed.  With these changes, that burden is now on
>> us.  I don't think we'd be happy maintaining system-level code that
>> have very little relevance to Emacs.
>>
>>> > - The sd_notify code is taken from
>>> > https://www.freedesktop.org/software/systemd/man/devel/sd_notify.html#Notes
>>
>> I'm not sure what would be the legal aspects of such code borrowing.
>
> The code is given as MIT-0, hence also the two different licenses for
> the two functions sd_notify and sd_is_socket. Not an expert on licenses
> either, but with a proper flag about what this function's license is, I
> guess it should be fine, since other projects also do that.

-- 
Best regards,
Nicolas Graves