Re: [conntrackd PATCH 2/2 v2] conntrackd: add systemd support

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Nov 06, 2015 at 04:59:22PM +0100, Arturo Borrero Gonzalez wrote:
> On 6 November 2015 at 14:53, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> +void sd_ct_watchdog_init(void)
> >> +{
> >> +     /* ignoring systemd API erros: only care if admin expects watchdog */
> >
> > Not sure what you mean with this comment.
> 
> The API call may returns:
>  * negative errno-style error code
>  * 0 if watchdog is not expected by systemd for this daemon
>  * >0 if watchdog is expected by systemd for this daemon
> 
> My patch ignores the 2 first options, we only continue executing this
> function if watchdog is expected by systemd (i.e, the admin configured
> it).

I think it is a good idea to report that no systemd watchdog is going
on through log messages.

> I ignored errors because I would not like to exit conntrackd if there
> is some issue in the systemd side, just ignore it.
> 
> >
> >> +     if (sd_watchdog_enabled(0, &sd_watchdog_interval) < 1)
> >
> > So sd_watchdog_enabled is setting sd_watchdog_interval?
> >
> > What is the value that sd_watchdog_interval is being set?
> >
> 
> Yes, only if sd_watchdog_enabled returned >0.
> 
> From the man page:
> 
> ""
> int sd_watchdog_enabled(int unset_environment, uint64_t *usec);
> [...]
> If the usec parameter is non-NULL, sd_watchdog_enabled() will write
> the timeout in µs for the watchdog logic to it.
> ""

I guess this is configured from systemd, what is the default?

On top of this, I wonder if we should have a configuration option from
our configuration file, something that allows us to do "Systemd Off".
I would suggest default in "Systemd On" if not specified, given that
main distros decided to follow this path.

After this patch, we can only enable/disable systemd integration at
configuration/compilation stage, however I expect most users will be
using packages from distros. I think it would be good to provide them
a runtime way to disable this if they don't want any interference with
their existing infrastructure.

Does this sound reasonable to you? Other than that above, this patch
looks good to me.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html