Re: [PATCH 0/5] Add systemd socket launching support.

[Eli Zaretskii]

> From: Matthew Leach <address@hidden>
> Date: Sat, 26 Mar 2016 21:16:37 +0000
> Cc: Matthew Leach <address@hidden>
> 
> Feedback & comments welcome!

Thank you for working on this.  Some specific comments below.  In
addition, please provide changes for NEWS and for the 2 manuals
describing this new feature.

> +#ifdef HAVE_SYSTEMD
> +      /* Read the number of sockets passed through by systemd. */
> +      systemd_socket = sd_listen_fds(0);

Isn't it prudent to test the socket descriptor for validity?  What if
it isn't a socket, for example?

Also, is it really correct to call this function with zero as
argument?  Do we want subordinate Emacs processes to use the same
socket?

> +DEFUN ("systemd-socket", Fsystemd_socket, Ssystemd_socket, 0, 0, 0,
> +       doc: /* Returns non-nil if systemd passed a socket through.
> +When systemd passes a socket through to emacs, return `t'.*/)

This is a predicate, so its name should end in a "-p".

Also, this should be in process.c, not in emacs.c (if it's needed at
all, see below).

> * src/process.c (connect_network_socket): Allow a systemd-allocated
>   file-descriptor to be passed, and use it, avoiding the call to
>   socket() and bind().
>   (Fmake_network_process): Allow users to pass in :systemd-fd on the
>   parameter plist to use a systemd fd.
>   (wait_reading_process_output): Call socket() & bind() every time.

I'm not sure I understand the rationale for this design.  Why do we
need to drag the systemd socket through all the APIs, including
exposing its value to Lisp(!), when it is stored in an internal
variable that can be easily accessed by the low-level network-related
functions?  Why cannot we instead provide a boolean flag that just
tells these low-level functions to use that socket?

>   (syms_of_process): New symbol.

Please state the name of the new symbol in the log message entry.

> +  int use_systemd_fd = !NILP (systemd_fd) && INTEGERP (systemd_fd) &&

Is it valid for systemd_fd to be non-positive?  If not, then INTEGERP is
not the correct test here.  Also, if you test INTEGERP, the !NILP test
is redundant.

> +DEFUN ("systemd-socket-fd", Fsystemd_socket_fd, Ssystemd_socket_fd, 0, 0, 0,
> +       doc: /* Returns the fd of the socket that systemd will pass through.
> +When systemd support is not enabled, return nil.*/)
> +  (void)
> +{
> +#ifdef HAVE_SYSTEMD
> +    return make_number (SD_LISTEN_FDS_START);
> +#else
> +    return Qnil;
> +#endif
> +}

Not sure I understand why we need both system-socket the predicate and
system-socket-fd.  Can't a single function serve both purposes?

Also, if this function is really needed (see the question above about
the overall design), its place is in process.c.