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

[Nicolas Graves]

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.

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