all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Nicolas Graves via "Development of GNU Guix and the GNU System distribution." <guix-devel@gnu.org>
To: Eli Zaretskii <eliz@gnu.org>
Cc: monnier@iro.umontreal.ca, ludo@gnu.org, emacs-devel@gnu.org,
	andrew@trop.in, guix-devel@gnu.org, bjorn.bidar@thaodan.de,
	rudi@constantly.at, felix.lechner@lease-up.com
Subject: Re: [Nicolas Graves] [PATCH v6 01/10] rde: emacs: Start emacs in --daemon mode, with shepherd and pid-file
Date: Sun, 12 May 2024 09:50:06 +0200	[thread overview]
Message-ID: <87cyprbhap.fsf@ngraves.fr> (raw)
In-Reply-To: <86bk5b1r1i.fsf@gnu.org>

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


  reply	other threads:[~2024-05-12  7:51 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20240410234923.29319-2-ngraves@ngraves.fr>
     [not found] ` <875xwotg35.fsf@trop.in>
2024-04-11 11:15   ` [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.
2024-04-12 20:38     ` Ludovic Courtès
2024-04-13 14:20       ` Nicolas Graves via Development of GNU Guix and the GNU System distribution.
2024-04-13 15:09         ` Nicolas Graves via Development of GNU Guix and the GNU System distribution.
2024-04-13 15:16         ` Stefan Monnier
2024-04-14 19:11           ` Björn Bidar
2024-04-14 20:52             ` Stefan Monnier
2024-04-19 14:19               ` Ludovic Courtès
2024-04-19 14:36                 ` Rudolf Schlatte
2024-04-20  2:31                   ` Stefan Monnier
2024-04-19 14:17             ` Ludovic Courtès
2024-05-11 20:15           ` Nicolas Graves via Development of GNU Guix and the GNU System distribution.
2024-05-11 23:07             ` Nicolas Graves via Development of GNU Guix and the GNU System distribution.
2024-05-12  6:29               ` Eli Zaretskii
2024-05-12  7:50                 ` Nicolas Graves via Development of GNU Guix and the GNU System distribution. [this message]
2024-05-12  7:54                   ` Nicolas Graves via Development of GNU Guix and the GNU System distribution.
2024-05-12  9:36                   ` Eli Zaretskii
2024-05-12 11:11                     ` Nicolas Graves via Development of GNU Guix and the GNU System distribution.
2024-05-12 15:01                       ` Nicolas Graves via Development of GNU Guix and the GNU System distribution.
2024-04-13 16:50       ` Nicolas Graves via Development of GNU Guix and the GNU System distribution.
2024-04-19 14:25         ` Ludovic Courtès
2024-04-14 16:51       ` Felix Lechner via Development of GNU Guix and the GNU System distribution.

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87cyprbhap.fsf@ngraves.fr \
    --to=guix-devel@gnu.org \
    --cc=andrew@trop.in \
    --cc=bjorn.bidar@thaodan.de \
    --cc=eliz@gnu.org \
    --cc=emacs-devel@gnu.org \
    --cc=felix.lechner@lease-up.com \
    --cc=ludo@gnu.org \
    --cc=monnier@iro.umontreal.ca \
    --cc=ngraves@ngraves.fr \
    --cc=rudi@constantly.at \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.