On 2024-05-12 09:29, Eli Zaretskii wrote:
>> Cc: Ludovic Courtès <l...@gnu.org>, emacs-de...@gnu.org,
>> Andrew Tropin <and...@trop.in>, guix-devel@gnu.org, bjorn.bi...@thaodan.de,
>> r...@constantly.at, felix.lech...@lease-up.com
>> Date: Sun, 12 May 2024 01:07:51 +0200
>> From: Nicolas Graves via "Emacs development discussions."
>> <emacs-de...@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
