From: Matthew Leach <matthew@mattleach.net>
To: Eli Zaretskii <eliz@gnu.org>
Cc: emacs-devel@gnu.org
Subject: Re: [PATCH 0/5] Add systemd socket launching support.
Date: Sun, 27 Mar 2016 16:17:38 +0100 [thread overview]
Message-ID: <87fuvcosq5.fsf@mattleach.net> (raw)
In-Reply-To: <83mvpkynzd.fsf@gnu.org> (Eli Zaretskii's message of "Sun, 27 Mar 2016 17:49:58 +0300")
Hi Eli,
Thanks for the feedback.
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Matthew Leach <matthew@mattleach.net>
>> Date: Sat, 26 Mar 2016 21:16:37 +0000
>> Cc: Matthew Leach <matthew@mattleach.net>
>>
>> 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
next prev parent reply other threads:[~2016-03-27 15:17 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-26 21:16 [PATCH 0/5] Add systemd socket launching support Matthew Leach
2016-03-26 21:16 ` [PATCH 1/5] Check for libsystemd when building Emacs Matthew Leach
2016-03-26 21:16 ` [PATCH 2/5] Read the number of sockets passed by systemd Matthew Leach
2016-03-26 21:16 ` [PATCH 3/5] Permit systemd-allocated socket file-descriptors to be used Matthew Leach
2016-03-27 12:08 ` Lars Magne Ingebrigtsen
2016-03-27 12:23 ` Matthew Leach
2016-03-27 12:42 ` Andreas Schwab
2016-03-27 13:38 ` Matthew Leach
2016-03-27 13:05 ` Lars Magne Ingebrigtsen
2016-03-27 13:39 ` Matthew Leach
2016-03-26 21:16 ` [PATCH 4/5] Allow the systed socket fd to be retrieved Matthew Leach
2016-03-26 21:16 ` [PATCH 5/5] When set, use the systemd socket descriptor Matthew Leach
2016-03-27 0:47 ` [PATCH 0/5] Add systemd socket launching support Alan Mackenzie
2016-03-27 0:59 ` Alexis
2016-03-27 8:44 ` Matthew Leach
2016-03-27 11:15 ` Alexis
2016-03-27 8:41 ` Matthew Leach
2016-03-27 13:48 ` Wolfgang Jenkner
2016-03-27 13:53 ` Matthew Leach
2016-03-27 14:18 ` Wolfgang Jenkner
2016-03-27 15:49 ` Mark Oteiza
2016-03-27 18:16 ` Matthew Leach
2016-03-27 14:49 ` Eli Zaretskii
2016-03-27 15:17 ` Matthew Leach [this message]
2016-03-27 15:23 ` Eli Zaretskii
2016-03-27 17:21 ` Philipp Stephani
2016-03-27 18:10 ` Matthew Leach
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=87fuvcosq5.fsf@mattleach.net \
--to=matthew@mattleach.net \
--cc=eliz@gnu.org \
--cc=emacs-devel@gnu.org \
/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/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.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.