On 2024-05-12 09:50, Nicolas Graves wrote: > 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.
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
