| Message ID | 20240916-ct-ifdef-v1-0-81ef1798143b@kernel.org |
|---|---|
| Headers | show |
| Series | netfilter: conntrack: label helpers conditional compilation updates | expand |
On Mon, Sep 16, 2024 at 04:14:40PM +0100, Simon Horman wrote: > Hi, > > This short series updates conditional compilation of label helpers to: > > 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled > or not. It is safe to do so as the functions will always return 0 if > CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should > optimise waway the code. Which is the desired behaviour. > > 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is > enabled. This addresses a warning about this function being unused > in this case. Both make sense to me, FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Mon, Sep 16, 2024 at 04:14:40PM +0100, Simon Horman wrote: > Hi, > > This short series updates conditional compilation of label helpers to: > > 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled > or not. It is safe to do so as the functions will always return 0 if > CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should > optimise waway the code. Which is the desired behaviour. > > 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is > enabled. This addresses a warning about this function being unused > in this case. Patch 1) -#ifdef CONFIG_NF_CONNTRACK_LABELS static inline int ctnetlink_label_size(const struct nf_conn *ct) Patch 2) +#ifdef CONFIG_NF_CONNTRACK_EVENTS static inline int ctnetlink_label_size(const struct nf_conn *ct) They both refer to ctnetlink_label_size(), #ifdef check is not correct. Would you mind if I collapsed these two patches before applying? Thanks Simon.
On Wed, Sep 18, 2024 at 01:52:14PM +0200, Pablo Neira Ayuso wrote: > On Mon, Sep 16, 2024 at 04:14:40PM +0100, Simon Horman wrote: > > Hi, > > > > This short series updates conditional compilation of label helpers to: > > > > 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled > > or not. It is safe to do so as the functions will always return 0 if > > CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should > > optimise waway the code. Which is the desired behaviour. > > > > 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is > > enabled. This addresses a warning about this function being unused > > in this case. > > Patch 1) > > -#ifdef CONFIG_NF_CONNTRACK_LABELS > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > Patch 2) > > +#ifdef CONFIG_NF_CONNTRACK_EVENTS > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > They both refer to ctnetlink_label_size(), #ifdef check is not > correct. But the first one touches more, no?
On Wed, Sep 18, 2024 at 04:55:15PM +0300, Andy Shevchenko wrote: > On Wed, Sep 18, 2024 at 01:52:14PM +0200, Pablo Neira Ayuso wrote: > > On Mon, Sep 16, 2024 at 04:14:40PM +0100, Simon Horman wrote: > > > Hi, > > > > > > This short series updates conditional compilation of label helpers to: > > > > > > 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled > > > or not. It is safe to do so as the functions will always return 0 if > > > CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should > > > optimise waway the code. Which is the desired behaviour. > > > > > > 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is > > > enabled. This addresses a warning about this function being unused > > > in this case. > > > > Patch 1) > > > > -#ifdef CONFIG_NF_CONNTRACK_LABELS > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > Patch 2) > > > > +#ifdef CONFIG_NF_CONNTRACK_EVENTS > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > They both refer to ctnetlink_label_size(), #ifdef check is not > > correct. > > But the first one touches more, no? Yes, it also remove a #define ctnetlink_label_size() macro in patch #1. I am fine with this series as is.
On Wed, Sep 18, 2024 at 04:29:14PM +0200, Pablo Neira Ayuso wrote: > On Wed, Sep 18, 2024 at 04:55:15PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 18, 2024 at 01:52:14PM +0200, Pablo Neira Ayuso wrote: > > > On Mon, Sep 16, 2024 at 04:14:40PM +0100, Simon Horman wrote: > > > > Hi, > > > > > > > > This short series updates conditional compilation of label helpers to: > > > > > > > > 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled > > > > or not. It is safe to do so as the functions will always return 0 if > > > > CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should > > > > optimise waway the code. Which is the desired behaviour. > > > > > > > > 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is > > > > enabled. This addresses a warning about this function being unused > > > > in this case. > > > > > > Patch 1) > > > > > > -#ifdef CONFIG_NF_CONNTRACK_LABELS > > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > > > Patch 2) > > > > > > +#ifdef CONFIG_NF_CONNTRACK_EVENTS > > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > > > They both refer to ctnetlink_label_size(), #ifdef check is not > > > correct. > > > > But the first one touches more, no? > > Yes, it also remove a #define ctnetlink_label_size() macro in patch #1. > I am fine with this series as is. What I meant is that the original patch 1 takes care about definitions of two functions. Not just a single one.
On Wed, Sep 18, 2024 at 01:52:14PM +0200, Pablo Neira Ayuso wrote: > On Mon, Sep 16, 2024 at 04:14:40PM +0100, Simon Horman wrote: > > Hi, > > > > This short series updates conditional compilation of label helpers to: > > > > 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled > > or not. It is safe to do so as the functions will always return 0 if > > CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should > > optimise waway the code. Which is the desired behaviour. > > > > 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is > > enabled. This addresses a warning about this function being unused > > in this case. > > Patch 1) > > -#ifdef CONFIG_NF_CONNTRACK_LABELS > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > Patch 2) > > +#ifdef CONFIG_NF_CONNTRACK_EVENTS > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > They both refer to ctnetlink_label_size(), #ifdef check is not > correct. > > Would you mind if I collapsed these two patches before applying? Sure, no problem.
On Wed, Sep 18, 2024 at 08:01:21PM +0300, Andy Shevchenko wrote: > On Wed, Sep 18, 2024 at 04:29:14PM +0200, Pablo Neira Ayuso wrote: > > On Wed, Sep 18, 2024 at 04:55:15PM +0300, Andy Shevchenko wrote: > > > On Wed, Sep 18, 2024 at 01:52:14PM +0200, Pablo Neira Ayuso wrote: > > > > On Mon, Sep 16, 2024 at 04:14:40PM +0100, Simon Horman wrote: > > > > > Hi, > > > > > > > > > > This short series updates conditional compilation of label helpers to: > > > > > > > > > > 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled > > > > > or not. It is safe to do so as the functions will always return 0 if > > > > > CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should > > > > > optimise waway the code. Which is the desired behaviour. > > > > > > > > > > 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is > > > > > enabled. This addresses a warning about this function being unused > > > > > in this case. > > > > > > > > Patch 1) > > > > > > > > -#ifdef CONFIG_NF_CONNTRACK_LABELS > > > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > > > > > Patch 2) > > > > > > > > +#ifdef CONFIG_NF_CONNTRACK_EVENTS > > > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > > > > > They both refer to ctnetlink_label_size(), #ifdef check is not > > > > correct. > > > > > > But the first one touches more, no? > > > > Yes, it also remove a #define ctnetlink_label_size() macro in patch #1. > > I am fine with this series as is. > > What I meant is that the original patch 1 takes care about definitions of > two functions. Not just a single one. My understanding is that #ifdef CONFIG_NF_CONNTRACK_LABELS that wraps ctnetlink_label_size() is not correct (patch 1), instead CONFIG_NF_CONNTRACK_EVENTS should be used (patch 2). Then, as a side effect this goes away (patch 1): -#else -#define ctnetlink_dump_labels(a, b) (0) -#define ctnetlink_label_size(a) (0) -#endif that is why I am proposing to coaleasce these two patches in one.
On Wed, Sep 18, 2024 at 11:56:24PM +0200, Pablo Neira Ayuso wrote: > On Wed, Sep 18, 2024 at 08:01:21PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 18, 2024 at 04:29:14PM +0200, Pablo Neira Ayuso wrote: > > > On Wed, Sep 18, 2024 at 04:55:15PM +0300, Andy Shevchenko wrote: > > > > On Wed, Sep 18, 2024 at 01:52:14PM +0200, Pablo Neira Ayuso wrote: > > > > > On Mon, Sep 16, 2024 at 04:14:40PM +0100, Simon Horman wrote: > > > > > > Hi, > > > > > > > > > > > > This short series updates conditional compilation of label helpers to: > > > > > > > > > > > > 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled > > > > > > or not. It is safe to do so as the functions will always return 0 if > > > > > > CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should > > > > > > optimise waway the code. Which is the desired behaviour. > > > > > > > > > > > > 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is > > > > > > enabled. This addresses a warning about this function being unused > > > > > > in this case. > > > > > > > > > > Patch 1) > > > > > > > > > > -#ifdef CONFIG_NF_CONNTRACK_LABELS > > > > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > > > > > > > Patch 2) > > > > > > > > > > +#ifdef CONFIG_NF_CONNTRACK_EVENTS > > > > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > > > > > > > They both refer to ctnetlink_label_size(), #ifdef check is not > > > > > correct. > > > > > > > > But the first one touches more, no? > > > > > > Yes, it also remove a #define ctnetlink_label_size() macro in patch #1. > > > I am fine with this series as is. > > > > What I meant is that the original patch 1 takes care about definitions of > > two functions. Not just a single one. > > My understanding is that #ifdef CONFIG_NF_CONNTRACK_LABELS that wraps > ctnetlink_label_size() is not correct (patch 1), instead > CONFIG_NF_CONNTRACK_EVENTS should be used (patch 2). > > Then, as a side effect this goes away (patch 1): > > -#else > -#define ctnetlink_dump_labels(a, b) (0) > -#define ctnetlink_label_size(a) (0) > -#endif > > that is why I am proposing to coaleasce these two patches in one. Thanks, Just to clarify. I did think there is value in separating the two changes. But that was a subjective judgement on my part. Your understanding of the overall change is correct. And if it is preferred to have a single patch - as seems to be the case - then that is fine by me. Going forward, I'll try to remember not to split-up patches for netfilter so much. Kind regards, Simon
On Thu, Sep 19, 2024 at 08:19:37AM +0100, Simon Horman wrote: > On Wed, Sep 18, 2024 at 11:56:24PM +0200, Pablo Neira Ayuso wrote: > > On Wed, Sep 18, 2024 at 08:01:21PM +0300, Andy Shevchenko wrote: > > > On Wed, Sep 18, 2024 at 04:29:14PM +0200, Pablo Neira Ayuso wrote: > > > > On Wed, Sep 18, 2024 at 04:55:15PM +0300, Andy Shevchenko wrote: > > > > > On Wed, Sep 18, 2024 at 01:52:14PM +0200, Pablo Neira Ayuso wrote: > > > > > > On Mon, Sep 16, 2024 at 04:14:40PM +0100, Simon Horman wrote: > > > > > > > Hi, > > > > > > > > > > > > > > This short series updates conditional compilation of label helpers to: > > > > > > > > > > > > > > 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled > > > > > > > or not. It is safe to do so as the functions will always return 0 if > > > > > > > CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should > > > > > > > optimise waway the code. Which is the desired behaviour. > > > > > > > > > > > > > > 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is > > > > > > > enabled. This addresses a warning about this function being unused > > > > > > > in this case. > > > > > > > > > > > > Patch 1) > > > > > > > > > > > > -#ifdef CONFIG_NF_CONNTRACK_LABELS > > > > > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > > > > > > > > > Patch 2) > > > > > > > > > > > > +#ifdef CONFIG_NF_CONNTRACK_EVENTS > > > > > > static inline int ctnetlink_label_size(const struct nf_conn *ct) > > > > > > > > > > > > They both refer to ctnetlink_label_size(), #ifdef check is not > > > > > > correct. > > > > > > > > > > But the first one touches more, no? > > > > > > > > Yes, it also remove a #define ctnetlink_label_size() macro in patch #1. > > > > I am fine with this series as is. > > > > > > What I meant is that the original patch 1 takes care about definitions of > > > two functions. Not just a single one. > > > > My understanding is that #ifdef CONFIG_NF_CONNTRACK_LABELS that wraps > > ctnetlink_label_size() is not correct (patch 1), instead > > CONFIG_NF_CONNTRACK_EVENTS should be used (patch 2). > > > > Then, as a side effect this goes away (patch 1): > > > > -#else > > -#define ctnetlink_dump_labels(a, b) (0) > > -#define ctnetlink_label_size(a) (0) > > -#endif > > > > that is why I am proposing to coaleasce these two patches in one. > > Thanks, > > Just to clarify. I did think there is value in separating the two changes. > But that was a subjective judgement on my part. > > Your understanding of the overall change is correct. > And if it is preferred to have a single patch - as seems to be the case - > then that is fine by me. > > Going forward, I'll try to remember not to split-up patches for netfilter > so much. Never mind too much, your splitting helps for reviewing. This is also subjective judgement on my side.
Hi, This short series updates conditional compilation of label helpers to: 1) Compile them regardless of if CONFIG_NF_CONNTRACK_LABELS is enabled or not. It is safe to do so as the functions will always return 0 if CONFIG_NF_CONNTRACK_LABELS is not enabled. And the compiler should optimise waway the code. Which is the desired behaviour. 2) Only compile ctnetlink_label_size if CONFIG_NF_CONNTRACK_EVENTS is enabled. This addresses a warning about this function being unused in this case. Found by inspection. Patches have been compile tested only. --- Simon Horman (2): netfilter: conntrack: compile label helpers unconditionally netfilter: conntrack: conditionally compile ctnetlink_label_size net/netfilter/nf_conntrack_netlink.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) base-commit: d5c4546062fd6f5dbce575c7ea52ad66d1968678