[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0/5] Add systemd socket launching support.
From: |
Matthew Leach |
Subject: |
Re: [PATCH 0/5] Add systemd socket launching support. |
Date: |
Sun, 27 Mar 2016 16:17:38 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) |
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
Re: [PATCH 0/5] Add systemd socket launching support., Eli Zaretskii, 2016/03/27
- Re: [PATCH 0/5] Add systemd socket launching support.,
Matthew Leach <=