On Tue, Jul 12, 2022 at 11:56 AM Nathan Chancellor <nathan@xxxxxxxxxx> wrote: > > Hi Justin, > > On Tue, Jul 12, 2022 at 11:34:52AM -0700, Justin Stitt wrote: > > pr_debug calls are no longer needed in this file. > > > > Pablo suggested "a patch to remove these pr_debug calls". This patch has > > some other beneficial collateral as it also silences multiple Clang > > -Wformat warnings that were present in the pr_debug calls. > > > > Suggested-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> > > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> > > Thanks for the patch! > > Reviewed-by: Nathan Chancellor <nathan@xxxxxxxxxx> > > Couple of style comments below that probably warrant a v2, you can carry > the above tag forward for future revisions. No need to give me a > "Suggested-by". > > > --- > > Suggestion here: https://lore.kernel.org/all/Ys0zZACWwGilTwHx@salvia/ > > > > net/netfilter/xt_TPROXY.c | 19 ------------------- > > 1 file changed, 19 deletions(-) > > > > diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c > > index 459d0696c91a..dc7284e6357b 100644 > > --- a/net/netfilter/xt_TPROXY.c > > +++ b/net/netfilter/xt_TPROXY.c > > @@ -74,18 +74,10 @@ tproxy_tg4(struct net *net, struct sk_buff *skb, __be32 laddr, __be16 lport, > > /* This should be in a separate target, but we don't do multiple > > targets on the same rule yet */ > > skb->mark = (skb->mark & ~mark_mask) ^ mark_value; > > - > > - pr_debug("redirecting: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", > > - iph->protocol, &iph->daddr, ntohs(hp->dest), > > - &laddr, ntohs(lport), skb->mark); > > - > > nf_tproxy_assign_sock(skb, sk); > > return NF_ACCEPT; > > } > > > > - pr_debug("no socket, dropping: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", > > - iph->protocol, &iph->saddr, ntohs(hp->source), > > - &iph->daddr, ntohs(hp->dest), skb->mark); > > return NF_DROP; > > } > > > > @@ -123,13 +115,11 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > > > > tproto = ipv6_find_hdr(skb, &thoff, -1, NULL, NULL); > > if (tproto < 0) { > > checkpatch.pl should have warned that these if statement braces here and > below are no longer necessary because there is only one statement within > them now. Weirdly, checkpatch.pl gave 0 warnings regarding this patch. At any rate, v2 is coming shortly. Thanks for the review! > > if (tproto < 0) > return NF_DROP; > > I believe it is important to do these types of style cleanups when doing > a larger change so that people do not try to do them as standalone > changes, which can irritate maintainers. > > > - pr_debug("unable to find transport header in IPv6 packet, dropping\n"); > > return NF_DROP; > > } > > > > hp = skb_header_pointer(skb, thoff, sizeof(_hdr), &_hdr); > > if (hp == NULL) { > > - pr_debug("unable to grab transport header contents in IPv6 packet, dropping\n"); > > return NF_DROP; > > } > > if (hp == NULL) > return NF_DROP; > > could even go a step farther and make it > > if (!hp) > return NF_DROP; > > if there is a warning about that. > > > > > @@ -168,19 +158,10 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > > /* This should be in a separate target, but we don't do multiple > > targets on the same rule yet */ > > skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; > > - > > - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > > - tproto, &iph->saddr, ntohs(hp->source), > > - laddr, ntohs(lport), skb->mark); > > - > > nf_tproxy_assign_sock(skb, sk); > > return NF_ACCEPT; > > } > > > > - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > > - tproto, &iph->saddr, ntohs(hp->source), > > - &iph->daddr, ntohs(hp->dest), skb->mark); > > - > > return NF_DROP; > > } > > > > -- > > 2.37.0.144.g8ac04bfd2-goog > >
