diff mbox

[RFC,conntrackd] conntrackd: add basic systemd notification support

Message ID 20151017105720.GA1462@salvia
State RFC
Delegated to: Pablo Neira
Headers show

Commit Message

Pablo Neira Ayuso Oct. 17, 2015, 10:57 a.m. UTC 
On Fri, Oct 16, 2015 at 11:37:31PM +0200, Jan Engelhardt wrote:
> On Friday 2015-10-16 21:23, Arturo Borrero Gonzalez wrote:
> 
> >On 16 October 2015 at 16:10, Jan Engelhardt <jengelh@inai.de> wrote:
> >>
> >> On Friday 2015-10-16 14:10, Arturo Borrero Gonzalez wrote:
> >>
> >>>+AC_ARG_ENABLE([systemd],
> >>>+      AS_HELP_STRING([--disable-systemd], [Do not build systemd support]),
> >>>+      [enable_systemd="no"], [enable_systemd="yes"])
> >>
> >> This is incorrect. It needs to be
> >>
> >>         [enable_systemd="$enableval"], [enable_systemd="yes"])
> >>
> >
> >I don't understand why. Could you please elaborate? The code above
> >(cthelper, cttimeout) is also bad?
> >
> >https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Package-Options.html
> 
> Quoting the page:
> 
> — Macro: AC_ARG_ENABLE (feature, help-string, [action-if-given], 
> [action-if-not-given])
>     If the user gave configure the option --enable-feature or 
>     --disable-feature, run shell commands action-if-given.
> 
> 
> So
> 
> $ ./configure --enable-systemd
> 
> runs the 3rd argument, and always sets enable_systemd=no,
> which is counter to what the user requested of configure.
> 
> In that regard, yes, conntrack-tools is also negatively affected. 
> Possibly other software from netfilter.org too.

Something like this (that we already have in nftables) should be fine:

commit 26b31033261427a3058a8fb994d54273bc93f7bf
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Tue Jul 7 14:32:21 2015 +0200

    configure: fix --enable-debug
    
    As the documentation indicates "The most common mistake for this
macro is to
    consider the two actions as action-if-enabled and
action-if-disabled."
    
    Use AS_IF in the action-if-present to check the real argument that
we're
    getting from the user.
    
    Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

 AC_SUBST(with_debug)
 AM_CONDITIONAL([BUILD_DEBUG], [test "x$with_debug" != xno])

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

Comments

Jan Engelhardt Oct. 18, 2015, 10:50 p.m. UTC | #1 
On Saturday 2015-10-17 12:57, Pablo Neira Ayuso wrote:
>Something like this (that we already have in nftables) should be fine:
>
> AC_ARG_ENABLE([debug],
>-             AS_HELP_STRING([--enable-debug], [Enable debugging]),
>-             [with_debug=no],
>+             AS_HELP_STRING([--enable-debug], [Disable debugging]),
>+             AS_IF([test "x$enable_debug" = "xno"], [with_debug=no],
>[with_debug=yes]),
>              [with_debug=yes])

Well the whole AS_IF part is not really needed, because you
are already testing in AM_CONDITIONAL:

> AC_SUBST(with_debug)
> AM_CONDITIONAL([BUILD_DEBUG], [test "x$with_debug" != xno])
--
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
diff mbox

Patch

diff --git a/configure.ac b/configure.ac
index d8f949a..931dbe1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -23,8 +23,8 @@  AC_DEFINE([_GNU_SOURCE], [], [Enable various GNU
extensions])
 AC_DEFINE([_STDC_FORMAT_MACROS], [], [printf-style format macros])
 
 AC_ARG_ENABLE([debug],
-             AS_HELP_STRING([--enable-debug], [Enable debugging]),
-             [with_debug=no],
+             AS_HELP_STRING([--enable-debug], [Disable debugging]),
+             AS_IF([test "x$enable_debug" = "xno"], [with_debug=no],
[with_debug=yes]),
              [with_debug=yes])