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

[Matthew Leach]

Hi Eli,

Thanks for the feedback.

Eli Zaretskii <address@hidden> writes:

>> 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.

Will do.

>> +#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?

Agreed.  I'm thinking about trying getsockname as a test for validity as
this should also ensure that the socket is already bound.

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

Agreed. Once this socket has been consumed we shouldn't pass it to any
subsequent processes.  I'll pass a '1' here.

>> +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?

Indeed, that does seem like a better approach.  I'm guessing the flag
you refer to should be passed to make_network_process from server-start?

>>   (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.

Indeed, system_fd should always be positive.  I'll look for a better
predicate to use.

>> +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.

I think I'll re-work this code to remove all the lisp defuns as you say.

Thanks,
-- 
Matt