Message ID | 1460132748-23561-1-git-send-email-weongyo.linux@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
From: Weongyo Jeong <weongyo.linux@gmail.com> Date: Fri, 8 Apr 2016 09:25:48 -0700 > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 1ecfa71..4e054bb 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -2042,6 +2042,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > u8 *skb_head = skb->data; > int skb_len = skb->len; > unsigned int snaplen, res; > + bool err = false; > > if (skb->pkt_type == PACKET_LOOPBACK) > goto drop; It is non-canonical to use a variable named 'err' as a boolean. Instead, all programmers expect a variable named 'err' to be an integer type and to hold an error code. Therefore please use a more appropriate name for this variable. Name it in a way which describes the exact important condition which is true or false. Thank you.
On Wed, Apr 13, 2016 at 10:56:01PM -0400, David Miller wrote: > From: Weongyo Jeong <weongyo.linux@gmail.com> > Date: Fri, 8 Apr 2016 09:25:48 -0700 > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > index 1ecfa71..4e054bb 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -2042,6 +2042,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > > u8 *skb_head = skb->data; > > int skb_len = skb->len; > > unsigned int snaplen, res; > > + bool err = false; > > > > if (skb->pkt_type == PACKET_LOOPBACK) > > goto drop; > > It is non-canonical to use a variable named 'err' as a boolean. > Instead, all programmers expect a variable named 'err' to be an > integer type and to hold an error code. > > Therefore please use a more appropriate name for this variable. > > Name it in a way which describes the exact important condition > which is true or false. > > Thank you. Thank you for your opinion. I'd fixed it to proper name and submitted v4 patch. Regards, Weongyo Jeong
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 1ecfa71..4e054bb 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -2042,6 +2042,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, u8 *skb_head = skb->data; int skb_len = skb->len; unsigned int snaplen, res; + bool err = false; if (skb->pkt_type == PACKET_LOOPBACK) goto drop; @@ -2130,6 +2131,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, return 0; drop_n_acct: + err = true; spin_lock(&sk->sk_receive_queue.lock); po->stats.stats1.tp_drops++; atomic_inc(&sk->sk_drops); @@ -2141,7 +2143,10 @@ drop_n_restore: skb->len = skb_len; } drop: - consume_skb(skb); + if (!err) + consume_skb(skb); + else + kfree_skb(skb); return 0; } @@ -2160,6 +2165,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, struct sk_buff *copy_skb = NULL; struct timespec ts; __u32 ts_status; + bool err = false; /* struct tpacket{2,3}_hdr is aligned to a multiple of TPACKET_ALIGNMENT. * We may add members to them until current aligned size without forcing @@ -2367,10 +2373,14 @@ drop_n_restore: skb->len = skb_len; } drop: - kfree_skb(skb); + if (!err) + consume_skb(skb); + else + kfree_skb(skb); return 0; drop_n_account: + err = true; po->stats.stats1.tp_drops++; spin_unlock(&sk->sk_receive_queue.lock);
consume_skb() isn't for error cases that kfree_skb() is more proper one. At this patch, it fixed tpacket_rcv() and packet_rcv() to be consistent for error or non-error cases letting perf trace its event properly. Signed-off-by: Weongyo Jeong <weongyo.linux@gmail.com> --- net/packet/af_packet.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)