[systemd-devel] [PATCH 1/3] socket: Add Support for TCP keep alive variables

[Lennart Poettering]

On Tue, 29.07.14 23:10, Susant Sahani (susant at redhat.com) wrote:

> The tcp keep alive variables now can be configured via conf
> parameter. Follwing variables are now supported by this patch.
> 
> tcp_keepalive_intvl: The number of seconds between TCP keep-alive probes
> 
> tcp_keepalive_probes: The maximum number of TCP keep-alive probes to
> send before giving up and killing the connection if no response is
> obtained from the other end.
> 
> tcp_keepalive_time: The number of seconds a connection needs to be
> idle before TCP begins sending out keep-alive probes.

Looks pretty OK.

> ---
>  man/systemd.socket.xml                | 36 +++++++++++++++++++++++++++++++++++
>  src/core/dbus-socket.c                |  3 +++
>  src/core/load-fragment-gperf.gperf.m4 |  3 +++
>  src/core/socket.c                     | 33 ++++++++++++++++++++++++++++++++
>  src/core/socket.h                     |  3 +++
>  5 files changed, 78 insertions(+)
> 
> diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
> index 09a7311..6dbcc81 100644
> --- a/man/systemd.socket.xml
> +++ b/man/systemd.socket.xml
> @@ -488,6 +488,42 @@
>                          </varlistentry>
>  
>                          <varlistentry>
> +                          <term><varname>KeepAliveTime=</varname></term>
> +                          <listitem><para>Takes time (in seconds) as
> argument . The connection needs to remain
   
        ^^^^^ there's a spurious space too much here...


> +                          idle before TCP starts sending keepalive probes. This controls the TCP_KEEPIDLE
> +                          socket option (see
> +                          <citerefentry><refentrytitle>socket</refentrytitle><manvolnum>7</manvolnum></citerefentry>
> +                          and the <ulink
> +                          url="http://www.tldp.org/HOWTO/html_single/TCP-Keepalive-HOWTO/">TCP
> +                          Keepalive HOWTO</ulink> for details.)
> +                          Defaults  value is 7200 seconds (2 hours).</para></listitem>
> +                        </varlistentry>

Could you indent this like the rest of the settings, please?

> +        SD_BUS_PROPERTY("KeepAliveTime", "t", bus_property_get_usec, offsetof(Socket, keep_alive_time), SD_BUS_VTABLE_PROPERTY_CONST),
> +        SD_BUS_PROPERTY("KeepAliveInterval", "t", bus_property_get_usec, offsetof(Socket, keep_alive_interval), SD_BUS_VTABLE_PROPERTY_CONST),
> +        SD_BUS_PROPERTY("KeepAliveProbes", "i", bus_property_get_int,
> offsetof(Socket, keep_alive_cnt), SD_BUS_VTABLE_PROPERTY_CONST),

This should really be an "u" and use bus_property_get_unsigned(), no? I
mean, there is no negative count possible, is there?

> +        if(s->keep_alive_time)
> +                fprintf(f,
> +                        "%sKeepAliveTime: %lo\n",
> +                        prefix, s->keep_alive_time / USEC_PER_SEC);

Please format this with format_timespan()!

Otherwise looks good!

Lennart

-- 
Lennart Poettering, Red Hat