| Message ID | 20130319141605.158831637@eitzenberger.org |
|---|---|
| State | RFC |
| Headers | show |
From: holger@eitzenberger.org Date: Tue, 19 Mar 2013 15:14:43 +0100 > + if (par->family == NFPROTO_IPV4) > + queue = (((u64) hash_v4(skb) * info->queues_total) >> > + 32) + queue; > +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) > + else if (par->family == NFPROTO_IPV6) > + queue = (((u64) hash_v6(skb) * info->queues_total) >> > + 32) + queue; > +#endif Maybe add a helper function so you don't have to indent so deeply. Something like: static u32 compute_queue(struct sk_buff *skb, u32 queue, u16 family, u16 qtotl) { u32 hash; if (family == NFPROTO_IPV4) hash = hash_v4(skb); #if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) else if (family == NFPROTO_IPV6) hash = hash_v6(skb); #endif return (((u64) hash * qtotl) >> 32) + queue; } -- 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
On Tuesday 2013-03-19 15:26, David Miller wrote: > >> + if (par->family == NFPROTO_IPV4) >> + queue = (((u64) hash_v4(skb) * info->queues_total) >> >> + 32) + queue; >> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) >> + else if (par->family == NFPROTO_IPV6) >> + queue = (((u64) hash_v6(skb) * info->queues_total) >> >> + 32) + queue; >> +#endif > >Maybe add a helper function so you don't have to indent so deeply. Something >like: And combined with smart arranging of clauses, won't need an extra helper function. http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful Something like +static unsigned int +nfqueue_tg_v3(struct sk_buff *skb, const struct xt_action_param *par) +{ + const struct xt_NFQ_info_v3 *info = par->targinfo; + u32 queue = info->queuenum; + + if (info->queues_total == 1) + return NF_QUEUE_NR(queue); + if (info->flags & NFQ_FLAG_CPU_FANOUT) { + int cpu = smp_processor_id(); + + queue = info->queuenum + cpu % info->queues_total; + } else if (par->family == NFPROTO_IPV4) { + queue = (((u64) hash_v4(skb) * info->queues_total) >> + 32) + queue; +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) + } else if (par->family == NFPROTO_IPV6) { + queue = (((u64) hash_v6(skb) * info->queues_total) >> + 32) + queue; + } +#endif + return NF_QUEUE_NR(queue); +} -- 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
From: Jan Engelhardt <jengelh@inai.de> Date: Tue, 19 Mar 2013 15:34:56 +0100 (CET) > > On Tuesday 2013-03-19 15:26, David Miller wrote: >> >>> + if (par->family == NFPROTO_IPV4) >>> + queue = (((u64) hash_v4(skb) * info->queues_total) >> >>> + 32) + queue; >>> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) >>> + else if (par->family == NFPROTO_IPV6) >>> + queue = (((u64) hash_v6(skb) * info->queues_total) >> >>> + 32) + queue; >>> +#endif >> >>Maybe add a helper function so you don't have to indent so deeply. Something >>like: > > And combined with smart arranging of clauses, won't need an extra > helper function. > http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful > > Something like Yes, but: > + queue = (((u64) hash_v4(skb) * info->queues_total) >> > + 32) + queue; ^^^^^^ it's that terribly indented giblet that I want to disappear. -- 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
holger@eitzenberger.org <holger@eitzenberger.org> wrote: > +struct xt_NFQ_info_v3 { > + __u16 queuenum; > + __u16 queues_total; > + __u16 bypass; > + __u16 flags; > +#define NFQ_FLAG_CPU_FANOUT 0x01 /* use current CPU (no hashing) */ > +}; minor nit: 'bypass' could be folded into 'flags' via a '#define NFQ_FLAG_BYPASS'. -- 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
> > +struct xt_NFQ_info_v3 { > > + __u16 queuenum; > > + __u16 queues_total; > > + __u16 bypass; > > + __u16 flags; > > +#define NFQ_FLAG_CPU_FANOUT 0x01 /* use current CPU (no hashing) */ > > +}; > > minor nit: > 'bypass' could be folded into 'flags' via a '#define NFQ_FLAG_BYPASS'. Yes, but I still need the new revision. Thanks. -- 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
Hi Jan, > http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful > > Something like > > +static unsigned int > +nfqueue_tg_v3(struct sk_buff *skb, const struct xt_action_param *par) > +{ > + const struct xt_NFQ_info_v3 *info = par->targinfo; > + u32 queue = info->queuenum; > + > + if (info->queues_total == 1) > + return NF_QUEUE_NR(queue); > + if (info->flags & NFQ_FLAG_CPU_FANOUT) { > + int cpu = smp_processor_id(); > + > + queue = info->queuenum + cpu % info->queues_total; > + } else if (par->family == NFPROTO_IPV4) { > + queue = (((u64) hash_v4(skb) * info->queues_total) >> > + 32) + queue; > +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) > + } else if (par->family == NFPROTO_IPV6) { > + queue = (((u64) hash_v6(skb) * info->queues_total) >> > + 32) + queue; > + } > +#endif > + return NF_QUEUE_NR(queue); > +} having read through the webpage - and using 'goto' for error handling in own code quite often - I don't see why your code above should be more readable. -- 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
On Tue, Mar 19, 2013 at 10:37:08AM -0400, David S. Miller wrote: > From: Jan Engelhardt <jengelh@inai.de> > Date: Tue, 19 Mar 2013 15:34:56 +0100 (CET) > > > > > On Tuesday 2013-03-19 15:26, David Miller wrote: > >> > >>> + if (par->family == NFPROTO_IPV4) > >>> + queue = (((u64) hash_v4(skb) * info->queues_total) >> > >>> + 32) + queue; > >>> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) > >>> + else if (par->family == NFPROTO_IPV6) > >>> + queue = (((u64) hash_v6(skb) * info->queues_total) >> > >>> + 32) + queue; > >>> +#endif > >> > >>Maybe add a helper function so you don't have to indent so deeply. Something > >>like: > > > > And combined with smart arranging of clauses, won't need an extra > > helper function. > > http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful > > > > Something like > > Yes, but: > > > + queue = (((u64) hash_v4(skb) * info->queues_total) >> > > + 32) + queue; > ^^^^^^ > > it's that terribly indented giblet that I want to disappear. Fully agreed, but at this time I kept the indentation. Take a look at the changes done in nfqueue_tg_v1() from patch #2. Will see how I can address your point. -- 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
On Tuesday 2013-03-19 22:34, Holger Eitzenberger wrote: >> http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful >> >> Something like >> >> +static unsigned int >> +nfqueue_tg_v3(struct sk_buff *skb, const struct xt_action_param *par) >> +{ >> + const struct xt_NFQ_info_v3 *info = par->targinfo; >> + u32 queue = info->queuenum; >> + >> + if (info->queues_total == 1) >> + return NF_QUEUE_NR(queue); >> ... >> +} > >having read through the webpage - and using 'goto' for error handling >in own code quite often - I don't see why your code above should be >more readable. It was only coincidental that the webpage also mentioned goto. The general idea (and google keywords) was called something like "Else Considered Harmful", and the lesson consisted of early exclusion of simple cases from the flow of the remainder of the function. Perhaps to illustrate it better: list_for_each_entry(thing, &yourlist, anchor) { if (thing->property == MAGIC) { do_stuff; } } => list_for_each_entry(thing, &yourlist, anchor) { if (thing->property != MAGIC) continue; do_stuff; } Code may become more readable as one gains one level of indent and have some more freedom in arranging the giblets inside the do_stuff parts. -- 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
> >having read through the webpage - and using 'goto' for error handling > >in own code quite often - I don't see why your code above should be > >more readable. > > It was only coincidental that the webpage also mentioned goto. > The general idea (and google keywords) was called something like > "Else Considered Harmful", and the lesson consisted of early > exclusion of simple cases from the flow of the remainder of the > function. Perhaps to illustrate it better: > > list_for_each_entry(thing, &yourlist, anchor) { > if (thing->property == MAGIC) { > do_stuff; > } > } > > => > > list_for_each_entry(thing, &yourlist, anchor) { > if (thing->property != MAGIC) > continue; > do_stuff; > } > > Code may become more readable as one gains one level of indent and > have some more freedom in arranging the giblets inside the do_stuff parts. Ack. -- 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 --git a/include/uapi/linux/netfilter/xt_NFQUEUE.h b/include/uapi/linux/netfilter/xt_NFQUEUE.h index 9eafdbb..1f24680 100644 --- a/include/uapi/linux/netfilter/xt_NFQUEUE.h +++ b/include/uapi/linux/netfilter/xt_NFQUEUE.h @@ -26,4 +26,12 @@ struct xt_NFQ_info_v2 { __u16 bypass; }; +struct xt_NFQ_info_v3 { + __u16 queuenum; + __u16 queues_total; + __u16 bypass; + __u16 flags; +#define NFQ_FLAG_CPU_FANOUT 0x01 /* use current CPU (no hashing) */ +}; + #endif /* _XT_NFQ_TARGET_H */ diff --git a/net/netfilter/xt_NFQUEUE.c b/net/netfilter/xt_NFQUEUE.c index 817f9e9..cf9c6a1 100644 --- a/net/netfilter/xt_NFQUEUE.c +++ b/net/netfilter/xt_NFQUEUE.c @@ -108,7 +108,7 @@ nfqueue_tg_v2(struct sk_buff *skb, const struct xt_action_param *par) static int nfqueue_tg_check(const struct xt_tgchk_param *par) { - const struct xt_NFQ_info_v2 *info = par->targinfo; + const struct xt_NFQ_info_v3 *info = par->targinfo; u32 maxid; if (unlikely(!rnd_inited)) { @@ -125,11 +125,39 @@ static int nfqueue_tg_check(const struct xt_tgchk_param *par) info->queues_total, maxid); return -ERANGE; } - if (par->target->revision == 2 && info->bypass > 1) + if (par->target->revision >= 2 && info->bypass > 1) + return -EINVAL; + if (par->target->revision == 3 && info->flags & ~NFQ_FLAG_CPU_FANOUT) return -EINVAL; + return 0; } +static unsigned int +nfqueue_tg_v3(struct sk_buff *skb, const struct xt_action_param *par) +{ + const struct xt_NFQ_info_v3 *info = par->targinfo; + u32 queue = info->queuenum; + + if (info->queues_total > 1) { + if (info->flags & NFQ_FLAG_CPU_FANOUT) { + int cpu = smp_processor_id(); + + queue = info->queuenum + cpu % info->queues_total; + } else { + if (par->family == NFPROTO_IPV4) + queue = (((u64) hash_v4(skb) * info->queues_total) >> + 32) + queue; +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) + else if (par->family == NFPROTO_IPV6) + queue = (((u64) hash_v6(skb) * info->queues_total) >> + 32) + queue; +#endif + } + } + return NF_QUEUE_NR(queue); +} + static struct xt_target nfqueue_tg_reg[] __read_mostly = { { .name = "NFQUEUE", @@ -156,6 +184,15 @@ static struct xt_target nfqueue_tg_reg[] __read_mostly = { .targetsize = sizeof(struct xt_NFQ_info_v2), .me = THIS_MODULE, }, + { + .name = "NFQUEUE", + .revision = 3, + .family = NFPROTO_UNSPEC, + .checkentry = nfqueue_tg_check, + .target = nfqueue_tg_v3, + .targetsize = sizeof(struct xt_NFQ_info_v3), + .me = THIS_MODULE, + }, }; static int __init nfqueue_tg_init(void)
Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org> --- include/uapi/linux/netfilter/xt_NFQUEUE.h | 8 ++++++ net/netfilter/xt_NFQUEUE.c | 41 ++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-) -- 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