RE: [net-next 1/2] dummy: add device MTU validation check

[张胜举]

> -Original Message-
> From: Sabrina Dubroca [mailto:s...@queasysnail.net]
> Sent: 2017年9月22日 20:23
> To: Eric Dumazet 
> Cc: Jarod Wilson ; Zhang Shengju
> ; da...@davemloft.net;
> will...@google.com; step...@networkplumber.org;
> netdev@vger.kernel.org
> Subject: Re: [net-next 1/2] dummy: add device MTU validation check
> 
> 2017-09-22, 04:05:09 -0700, Eric Dumazet wrote:
> > On Fri, 2017-09-22 at 10:56 +0200, Sabrina Dubroca wrote:
> > > 2017-09-21, 08:02:18 -0700, Eric Dumazet wrote:
> > > > On Thu, 2017-09-21 at 21:32 +0800, Zhang Shengju wrote:
> > > > > Currently, any mtu value can be assigned when adding a new dummy
> device:
> > > > > [~]# ip link add name dummy1 mtu 10 type dummy [~]# ip link
> > > > > show dummy1
> > > > > 15: dummy1:  mtu 10 qdisc noop state
> DOWN mode DEFAULT group default qlen 1000
> > > > > link/ether 0a:61:6b:16:14:ce brd ff:ff:ff:ff:ff:ff
> > > > >
> > > > > This patch adds device MTU validation check.
> > > >
> > > > What is wrong with big MTU on dummy ?
> > >
> > > It looks like the "centralize MTU checking" series broke that, but
> > > only for changing the MTU on an existing dummy device. Commit
> > > a52ad514fdf3 defined min_mtu/max_mtu in ether_setup, which dummy
> > > uses, but there was no MTU check in dummy prior to that commit.
> > >
> >
> > It looks like we accept big mtu on loopback, right ?
> 
> Yes. I only meant that before commit a52ad514fdf3, there was no range check
> on dummy's MTU. Commit 25e3e84b183a ("dummy: expend mtu range for
> dummy device") and 8b1efc0f83f1 ("net: remove MTU limits on a few
> ether_setup callers") fixed that only partially. It's the same with ifb, btw, 
> it
> didn't have any check before a52ad514fdf3, so we should set min_mtu =
> max_mtu = 0.
> 
> --
> Sabrina

I agree, dummy and ifb device should not have any limit on mtu, just like 
loopback device.
I will send v2 patch, and set min/max_mtu to zero for dummy and ifb device, 
thanks.

ZSJ

RE: [net-next 2/2] ifb: add device MTU validation check

[张胜举]

> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: 2017年9月21日 23:10
> To: Zhang Shengju 
> Cc: da...@davemloft.net; will...@google.com; netdev@vger.kernel.org
> Subject: Re: [net-next 2/2] ifb: add device MTU validation check
> 
> On Thu, 21 Sep 2017 21:32:02 +0800
> Zhang Shengju  wrote:
> 
> > Currently, any mtu value can be assigned when adding a new ifb device:
> > [~]# ip link add name ifb2 mtu 10 type ifb [~]# ip link show ifb2
> > 18: ifb2:  mtu 10 qdisc noop state DOWN mode
> DEFAULT group default qlen 32
> > link/ether 7a:bf:f4:63:da:d1 brd ff:ff:ff:ff:ff:ff
> >
> > This patch adds device MTU validation check.
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  drivers/net/ifb.c | 8 
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c index
> > 8870bd2..ce84ad2 100644
> > --- a/drivers/net/ifb.c
> > +++ b/drivers/net/ifb.c
> > @@ -282,6 +282,14 @@ static int ifb_validate(struct nlattr *tb[], struct
> nlattr *data[],
> > if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
> > return -EADDRNOTAVAIL;
> > }
> > +
> > +   if (tb[IFLA_MTU]) {
> > +   u32 mtu = nla_get_u32(tb[IFLA_MTU]);
> > +
> > +   if (mtu < ETH_MIN_MTU || mtu > ETH_DATA_LEN)
> > +   return -EINVAL;
> > +   }
> > +
> > return 0;
> >  }
> >
> 
> What about running ifb with packets coming from devices with jumbo frames?
> Also since ifb is an input only device, MTU doesn't matter.

Actually ifb_setup() function setup ifb valid MTU range: [68, 1500], and
this will be check at dev_set_mtu() function. 
You can't setup mtu out of this range after creation, but you can set any
value when adding new ifb device.
This is inconsistent, and I think we should add this check at creation time 

BRs,
ZSJ

RE: [net-next 1/2] dummy: add device MTU validation check

[张胜举]

> -Original Message-
> From: Eric Dumazet [mailto:eric.duma...@gmail.com]
> Sent: 2017年9月21日 23:02
> To: Zhang Shengju 
> Cc: da...@davemloft.net; will...@google.com;
> step...@networkplumber.org; netdev@vger.kernel.org
> Subject: Re: [net-next 1/2] dummy: add device MTU validation check
> 
> On Thu, 2017-09-21 at 21:32 +0800, Zhang Shengju wrote:
> > Currently, any mtu value can be assigned when adding a new dummy device:
> > [~]# ip link add name dummy1 mtu 10 type dummy [~]# ip link show
> > dummy1
> > 15: dummy1:  mtu 10 qdisc noop state DOWN
> mode DEFAULT group default qlen 1000
> > link/ether 0a:61:6b:16:14:ce brd ff:ff:ff:ff:ff:ff
> >
> > This patch adds device MTU validation check.
> 
> What is wrong with big MTU on dummy ?
> 
> If this is a generic rule, this check should belong in core network stack.
> 

dummy_setup() function setup mtu range: [0, ETH_MAX_MTU]. 
This will be checked at dev_set_mtu() function in core network stack.

So if you add a new dummy device without specify mtu value, you can't set a 
value out of range [0, ETH_MAX_MTU] afterward.
BUT you can set any mtu when adding new device. This cause an inconsistence.

> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  drivers/net/dummy.c | 8 
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c index
> > e31ab3b..0276b2b 100644
> > --- a/drivers/net/dummy.c
> > +++ b/drivers/net/dummy.c
> > @@ -365,6 +365,14 @@ static int dummy_validate(struct nlattr *tb[], struct
> nlattr *data[],
> > if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
> > return -EADDRNOTAVAIL;
> > }
> > +
> > +   if (tb[IFLA_MTU]) {
> > +   u32 mtu = nla_get_u32(tb[IFLA_MTU]);
> 
> You do not verify/validate nla_len(tb[IFLA_MTU]).
> 
> Do not ever trust user space.
MTU attribute is just u32, do you think it's necessary to check the length? 
Actually I don't see any place to check the length of mtu attribute in network 
stack code. 

> 
> > +
> > +   if (mtu > ETH_MAX_MTU)
> > +   return -EINVAL;
> > +   }
> > +
> > return 0;
> >  }
> >
> 

RE: [net-next] macvlan: propagate the mac address change status for lowerdev

[张胜举]

> -Original Message-
> From: Yuval Shaia [mailto:yuval.sh...@oracle.com]
> Sent: Wednesday, June 14, 2017 4:34 AM
> To: Zhang Shengju 
> Cc: da...@davemloft.net; f...@48lvckh6395k16k5.yundunddos.com;
> vyase...@redhat.com; netdev@vger.kernel.org
> Subject: Re: [net-next] macvlan: propagate the mac address change status
> for lowerdev
> 
> On Tue, Jun 13, 2017 at 10:45:11PM +0800, Zhang Shengju wrote:
> > The macvlan dev should propagate the return value of mac address
> > change for lower device in the passthru mode, instead of always return
0.
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  drivers/net/macvlan.c | 6 ++
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c index
> > 346ad2f..ade1213 100644
> > --- a/drivers/net/macvlan.c
> > +++ b/drivers/net/macvlan.c
> > @@ -703,10 +703,8 @@ static int macvlan_set_mac_address(struct
> net_device *dev, void *p)
> > if (!is_valid_ether_addr(addr->sa_data))
> > return -EADDRNOTAVAIL;
> >
> > -   if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
> > -   dev_set_mac_address(vlan->lowerdev, addr);
> > -   return 0;
> > -   }
> > +   if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> > +   return dev_set_mac_address(vlan->lowerdev, addr);
> 
> Do you think the following functions needs this fix as well?
> - alb_set_mac_address
> - bond_alb_handle_active_change
> - bond_enslave
> - __bond_release_one
> - macvlan_set_mac_address
> 
> Yuval
Actually, this patch fixes macvlan part. 
The other part is not so easy to fix in my option, you can try to fix if
possible.

BRs,
ZSJ
> 
> >
> > return macvlan_sync_address(dev, addr->sa_data);  }
> > --
> > 1.8.3.1
> >
> >
> >

RE: [net-next] openvswitch: add macro MODULE_ALIAS_VPORT_TYPE for vport type alias

[张胜举]

> -Original Message-
> From: Pravin Shelar [mailto:pshe...@ovn.org]
> Sent: Monday, June 05, 2017 10:30 AM
> To: 张胜举 <zhangshen...@cmss.chinamobile.com>
> Cc: Pravin Shelar <pshe...@nicira.com>; Linux Kernel Network Developers
> <netdev@vger.kernel.org>; David S. Miller <da...@davemloft.net>
> Subject: Re: [net-next] openvswitch: add macro
> MODULE_ALIAS_VPORT_TYPE for vport type alias
> 
> On Sun, Jun 4, 2017 at 1:12 AM, 张胜举
> <zhangshen...@cmss.chinamobile.com> wrote:
> >> -Original Message-
> >> From: Pravin Shelar [mailto:pshe...@ovn.org]
> >> Sent: Sunday, June 04, 2017 1:58 PM
> >> To: Zhang Shengju <zhangshen...@cmss.chinamobile.com>
> >> Cc: Pravin Shelar <pshe...@nicira.com>; Linux Kernel Network
> >> Developers <netdev@vger.kernel.org>; David S. Miller
> >> <da...@davemloft.net>
> >> Subject: Re: [net-next] openvswitch: add macro
> >> MODULE_ALIAS_VPORT_TYPE for vport type alias
> >>
> >> On Sat, Jun 3, 2017 at 6:47 AM, Zhang Shengju
> >> <zhangshen...@cmss.chinamobile.com> wrote:
> >> > Add a new macro MODULE_ALIAS_VPORT_TYPE to unify and simplify
> the
> >> > declaration of vport type alias, and replace magic numbers with
> >> > symbolic constants.
> >> >
> >> > Signed-off-by: Zhang Shengju <zhangshen...@cmss.chinamobile.com>
> >> > ---
> >> >  net/openvswitch/vport-geneve.c | 2 +-
> >> >  net/openvswitch/vport-gre.c| 2 +-
> >> >  net/openvswitch/vport-vxlan.c  | 2 +-
> >> >  net/openvswitch/vport.h| 3 +++
> >> >  4 files changed, 6 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/net/openvswitch/vport-geneve.c
> >> > b/net/openvswitch/vport-geneve.c index 5aaf3ba..1c068d6 100644
> >> > --- a/net/openvswitch/vport-geneve.c
> >> > +++ b/net/openvswitch/vport-geneve.c
> >> > @@ -141,4 +141,4 @@ static void __exit ovs_geneve_tnl_exit(void)
> >> >
> >> >  MODULE_DESCRIPTION("OVS: Geneve switching port");
> >> > MODULE_LICENSE("GPL"); -MODULE_ALIAS("vport-type-5");
> >> > +MODULE_ALIAS_VPORT_TYPE(OVS_VPORT_TYPE_GENEVE);
> >> > diff --git a/net/openvswitch/vport-gre.c
> >> > b/net/openvswitch/vport-gre.c index 0e72d95..48a5852 100644
> >> > --- a/net/openvswitch/vport-gre.c
> >> > +++ b/net/openvswitch/vport-gre.c
> >> > @@ -113,4 +113,4 @@ static void __exit ovs_gre_tnl_exit(void)
> >> >
> >> >  MODULE_DESCRIPTION("OVS: GRE switching port");
> >> > MODULE_LICENSE("GPL"); -MODULE_ALIAS("vport-type-3");
> >> > +MODULE_ALIAS_VPORT_TYPE(OVS_VPORT_TYPE_GRE);
> >>
> >> This is user visible change. For example this is changing the gre
> >> module alias from "vport-type-3" to "vport-type-OVS_VPORT_TYPE_GRE".
> >> This could break userspace application.
> > Hi Shelar,
> > Actually this change does not change module alias name. I use
> > '__stringify' to address this.  Below is my build result:
> > ```
> > [zhangshengju@promote net-next]$ modinfo net/openvswitch/vport-
> gre.ko
> > filename:   /gitlab/net-next/net/openvswitch/vport-gre.ko
> > alias:  vport-type-3
> > ...
> > ```
> 
> Yes, I expected this. But when I tried modinfo with your patch, I did not see 
> it.
> here is modinfo output from my setup.
> ---8<---
> root@ubuntu:/home/pravin/linux/net-next# modinfo
> net/openvswitch/vport-gre.ko
> filename:   /home/pravin/linux/net-next/./net/openvswitch/vport-gre.ko
> alias:  vport-type-OVS_VPORT_TYPE_GRE
> license:GPL
> description:OVS: GRE switching port
> srcversion: AD3B4449820F294E0B5681C
> depends:openvswitch
> intree: Y
> vermagic:   4.12.0-rc2+ SMP mod_unload modversions
Oh, I'm sorry. You are right, it seems ' OVS_VPORT_TYPE_GRE' doesn't expand. 
Maybe I can send another version to use magic number, such as:
MODULE_ALIAS_VPORT_TYPE(3);

Is this okay for you?

BRs,
Zhang Shengju.

RE: [net-next] openvswitch: add macro MODULE_ALIAS_VPORT_TYPE for vport type alias

[张胜举]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Monday, June 05, 2017 4:13 AM
> To: pshe...@ovn.org
> Cc: zhangshen...@cmss.chinamobile.com; pshe...@nicira.com;
> netdev@vger.kernel.org
> Subject: Re: [net-next] openvswitch: add macro
> MODULE_ALIAS_VPORT_TYPE for vport type alias
> 
> From: Pravin Shelar 
> Date: Sat, 3 Jun 2017 22:58:22 -0700
> 
> > On Sat, Jun 3, 2017 at 6:47 AM, Zhang Shengju
> >  wrote:
> >> Add a new macro MODULE_ALIAS_VPORT_TYPE to unify and simplify the
> >> declaration of vport type alias, and replace magic numbers with
> >> symbolic constants.
> >>
> >> Signed-off-by: Zhang Shengju 
> >> ---
> >>  net/openvswitch/vport-geneve.c | 2 +-
> >>  net/openvswitch/vport-gre.c| 2 +-
> >>  net/openvswitch/vport-vxlan.c  | 2 +-
> >>  net/openvswitch/vport.h| 3 +++
> >>  4 files changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/openvswitch/vport-geneve.c
> >> b/net/openvswitch/vport-geneve.c index 5aaf3ba..1c068d6 100644
> >> --- a/net/openvswitch/vport-geneve.c
> >> +++ b/net/openvswitch/vport-geneve.c
> >> @@ -141,4 +141,4 @@ static void __exit ovs_geneve_tnl_exit(void)
> >>
> >>  MODULE_DESCRIPTION("OVS: Geneve switching port");
> >> MODULE_LICENSE("GPL"); -MODULE_ALIAS("vport-type-5");
> >> +MODULE_ALIAS_VPORT_TYPE(OVS_VPORT_TYPE_GENEVE);
> >> diff --git a/net/openvswitch/vport-gre.c
> >> b/net/openvswitch/vport-gre.c index 0e72d95..48a5852 100644
> >> --- a/net/openvswitch/vport-gre.c
> >> +++ b/net/openvswitch/vport-gre.c
> >> @@ -113,4 +113,4 @@ static void __exit ovs_gre_tnl_exit(void)
> >>
> >>  MODULE_DESCRIPTION("OVS: GRE switching port");
> >> MODULE_LICENSE("GPL"); -MODULE_ALIAS("vport-type-3");
> >> +MODULE_ALIAS_VPORT_TYPE(OVS_VPORT_TYPE_GRE);
> >
> > This is user visible change. For example this is changing the gre
> > module alias from "vport-type-3" to "vport-type-OVS_VPORT_TYPE_GRE".
> > This could break userspace application.
> 
> Agreed, you really can't do this.

Hi David,
Actually this change does not change module alias name. I use '__stringify'
to 
address this.  Please refer my previous reply.

BRs,
Zhang Shengju

RE: [net-next] openvswitch: add macro MODULE_ALIAS_VPORT_TYPE for vport type alias

[张胜举]

> -Original Message-
> From: Pravin Shelar [mailto:pshe...@ovn.org]
> Sent: Sunday, June 04, 2017 1:58 PM
> To: Zhang Shengju 
> Cc: Pravin Shelar ; Linux Kernel Network Developers
> ; David S. Miller 
> Subject: Re: [net-next] openvswitch: add macro
> MODULE_ALIAS_VPORT_TYPE for vport type alias
> 
> On Sat, Jun 3, 2017 at 6:47 AM, Zhang Shengju
>  wrote:
> > Add a new macro MODULE_ALIAS_VPORT_TYPE to unify and simplify the
> > declaration of vport type alias, and replace magic numbers with
> > symbolic constants.
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  net/openvswitch/vport-geneve.c | 2 +-
> >  net/openvswitch/vport-gre.c| 2 +-
> >  net/openvswitch/vport-vxlan.c  | 2 +-
> >  net/openvswitch/vport.h| 3 +++
> >  4 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/openvswitch/vport-geneve.c
> > b/net/openvswitch/vport-geneve.c index 5aaf3ba..1c068d6 100644
> > --- a/net/openvswitch/vport-geneve.c
> > +++ b/net/openvswitch/vport-geneve.c
> > @@ -141,4 +141,4 @@ static void __exit ovs_geneve_tnl_exit(void)
> >
> >  MODULE_DESCRIPTION("OVS: Geneve switching port");
> > MODULE_LICENSE("GPL"); -MODULE_ALIAS("vport-type-5");
> > +MODULE_ALIAS_VPORT_TYPE(OVS_VPORT_TYPE_GENEVE);
> > diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
> > index 0e72d95..48a5852 100644
> > --- a/net/openvswitch/vport-gre.c
> > +++ b/net/openvswitch/vport-gre.c
> > @@ -113,4 +113,4 @@ static void __exit ovs_gre_tnl_exit(void)
> >
> >  MODULE_DESCRIPTION("OVS: GRE switching port");
> > MODULE_LICENSE("GPL"); -MODULE_ALIAS("vport-type-3");
> > +MODULE_ALIAS_VPORT_TYPE(OVS_VPORT_TYPE_GRE);
> 
> This is user visible change. For example this is changing the gre
> module alias from "vport-type-3" to "vport-type-OVS_VPORT_TYPE_GRE".
> This could break userspace application.
Hi Shelar,
Actually this change does not change module alias name. I use '__stringify' to 
address this.  Below is my build result:
```
[zhangshengju@promote net-next]$ modinfo net/openvswitch/vport-gre.ko 
filename:   /gitlab/net-next/net/openvswitch/vport-gre.ko
alias:  vport-type-3
...
```

BRs,
Zhang Shengju

RE: [net-next] net: remove duplicate add_device_randomness() call

[张胜举]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Thursday, May 04, 2017 11:13 PM
> To: zhangshen...@cmss.chinamobile.com
> Cc: netdev@vger.kernel.org; eduma...@google.com
> Subject: Re: [net-next] net: remove duplicate add_device_randomness() call
> 
> From: Zhang Shengju 
> Date: Thu,  4 May 2017 11:40:42 +0800
> 
> > Since register_netdevice() already call add_device_randomness() and
> > dev_set_mac_address() will call it after mac address change.
> > It's not necessary to call at device UP.
> >
> > Signed-off-by: Zhang Shengju 
> 
> The net-next tree is closed, please resubmit this when the net-next tree
> opens back up.

Okay, thanks David.

BRs,
ZSJ

RE: [iproute2] iplink: add support for IFLA_CARRIER attribute

[张胜举]

> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Wednesday, April 26, 2017 11:08 PM
> To: Zhang Shengju 
> Cc: netdev@vger.kernel.org
> Subject: Re: [iproute2] iplink: add support for IFLA_CARRIER attribute
> 
> On Wed, 26 Apr 2017 15:08:39 +0800
> Zhang Shengju  wrote:
> 
> > Add support to set IFLA_CARRIER attribute.
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  ip/iplink.c | 12 
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/ip/iplink.c b/ip/iplink.c index 866ad72..263bfdd 100644
> > --- a/ip/iplink.c
> > +++ b/ip/iplink.c
> > @@ -72,6 +72,7 @@ void iplink_usage(void)
> > " [ allmulticast { on | off } ]\n"
> > " [ promisc { on | off } ]\n"
> > " [ trailers { on | off } ]\n"
> > +   " [ carrier { on | off } ]\n"
> > " [ txqueuelen PACKETS ]\n"
> > " [ name NEWNAME ]\n"
> > " [ address LLADDR ]\n"
> > @@ -673,6 +674,17 @@ int iplink_parse(int argc, char **argv, struct
> iplink_req *req,
> > req->i.ifi_flags |= IFF_NOARP;
> > else
> > return on_off("arp", *argv);
> > +   } else if (strcmp(*argv, "carrier") == 0) {
> > +   int carrier;
> > +   NEXT_ARG();
> > +   if (strcmp(*argv, "on") == 0)
> > +   carrier = 1;
> > +   else if (strcmp(*argv, "off") == 0)
> > +   carrier = 0;
> > +   else
> > +   return on_off("carrier", *argv);
> > +
> > +   addattr8(>n, sizeof(*req), IFLA_CARRIER,
> carrier);
> > } else if (strcmp(*argv, "vf") == 0) {
> > struct rtattr *vflist;
> >
> 
> The general policy of ip link command is all options should be invertable.
Yes, so I add 'on' and 'off' subcommand to make sure that it can be
invertable.

> There are some VPN's that use this to save and restore state. So if you
add
> an option to set something there should be similar output under the
detailed
> show command.
Currently, "show command" already can display 'carrier' status. Such as:
 dummy0: 

RE: [net-next] icmp: correct return value of icmp_rcv()

[张胜举]

> -Original Message-
> From: Eric Dumazet [mailto:eric.duma...@gmail.com]
> Sent: Wednesday, December 07, 2016 10:18 PM
> To: Zhang Shengju 
> Cc: netdev@vger.kernel.org
> Subject: Re: [net-next] icmp: correct return value of icmp_rcv()
> 
> On Wed, 2016-12-07 at 14:52 +0800, Zhang Shengju wrote:
> > Currently, icmp_rcv() always return zero on a packet delivery upcall.
> >
> > To make its behavior more compliant with the way this API should be
> > used, this patch changes this to let it return NET_RX_SUCCESS when the
> > packet is proper handled, and NET_RX_DROP otherwise.
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  net/ipv4/icmp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index 691146a..f79d7a8
> > 100644
> > --- a/net/ipv4/icmp.c
> > +++ b/net/ipv4/icmp.c
> > @@ -1047,12 +1047,12 @@ int icmp_rcv(struct sk_buff *skb)
> >
> > if (success)  {
> > consume_skb(skb);
> > -   return 0;
> > +   return NET_RX_SUCCESS;
> > }
> >
> >  drop:
> > kfree_skb(skb);
> > -   return 0;
> > +   return NET_RX_DROP;
> >  csum_error:
> > __ICMP_INC_STATS(net, ICMP_MIB_CSUMERRORS);
> >  error:
> 
> 
> I am curious, what external/visible effects do you expect from such a change ?
> 
> We now have a very precise monitoring of where packets are dropped
> (consume_skb()/kfree_skb())
> 
> 

I know that the return value is always ignored, I just to want to make it 
compliant with the way this API required like I said in the comment.

Thanks,

RE: [net-next] neigh: remove duplicate check for same neigh

[张胜举]

> -Original Message-
> From: David Ahern [mailto:d...@cumulusnetworks.com]
> Sent: Wednesday, November 30, 2016 12:23 AM
> To: Zhang Shengju ;
> netdev@vger.kernel.org
> Subject: Re: [net-next] neigh: remove duplicate check for same neigh
> 
> On 11/29/16 1:22 AM, Zhang Shengju wrote:
> > @@ -2285,20 +2295,15 @@ static int neigh_dump_table(struct neigh_table
> *tbl, struct sk_buff *skb,
> > rcu_read_lock_bh();
> > nht = rcu_dereference_bh(tbl->nht);
> >
> > -   for (h = s_h; h < (1 << nht->hash_shift); h++) {
> > -   if (h > s_h)
> > -   s_idx = 0;
> > +   for (h = s_h; h < (1 << nht->hash_shift); h++, s_idx = 0) {
> > for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0;
> >  n != NULL;
> > -n = rcu_dereference_bh(n->next)) {
> > -   if (!net_eq(dev_net(n->dev), net))
> > -   continue;
> > -   if (neigh_ifindex_filtered(n->dev, filter_idx))
> > +n = rcu_dereference_bh(n->next), idx++) {
> 
> incrementing idx here ...
> 
> > +   if (idx < s_idx || !net_eq(dev_net(n->dev), net))
> > continue;
> > -   if (neigh_master_filtered(n->dev,
filter_master_idx))
> > +   if (neigh_dump_filtered(n->dev, filter_idx,
> > +   filter_master_idx))
> > continue;
> > -   if (idx < s_idx)
> > -   goto next;
> > if (neigh_fill_info(skb, n, NETLINK_CB(cb-
> >skb).portid,
> > cb->nlh->nlmsg_seq,
> > RTM_NEWNEIGH,
> 
> ... causes a missed entry when an error happens here
> 
> idx is only incremented when a neigh has been successfully added to the
skb.

If an error happens, we just goto 'out' and exit this loop, this will skip
the 'idx++'.  
So no missed entry, right?

> 
> 
> Your intention is to save a few cycles by making idx an absolute index
within
> the hash bucket and jumping to the next entry to be dumped without
> evaluating any filters per entry.
> 
> So why not keep it simple and do this:

This has less changes, but I preferred to add a new function to do the
filter logic, this is the same as your code  @rtnl_dump_ifinfo().

And for the 'idx', it should be increased when neigh entry is filter out or
is successfully added to the skb. Isn't it straight-forward to put it in for
loop?

> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c index
> 2ae929f9bd06..2e49a85e696a 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2291,14 +2291,12 @@ static int neigh_dump_table(struct neigh_table
> *tbl, struct sk_buff *skb,
> for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx =
0;
>  n != NULL;
>  n = rcu_dereference_bh(n->next)) {
> -   if (!net_eq(dev_net(n->dev), net))
> -   continue;
> -   if (neigh_ifindex_filtered(n->dev, filter_idx))
> -   continue;
> -   if (neigh_master_filtered(n->dev,
filter_master_idx))
> -   continue;
> if (idx < s_idx)
> goto next;
> +   if (!net_eq(dev_net(n->dev), net) ||
> +   neigh_ifindex_filtered(n->dev, filter_idx) ||
> +   neigh_master_filtered(n->dev,
filter_master_idx))
> +   goto next;
> if (neigh_fill_info(skb, n,
NETLINK_CB(cb->skb).portid,
> cb->nlh->nlmsg_seq,
> RTM_NEWNEIGH,

RE: [net,v2] neigh: fix the loop index error in neigh dump

[张胜举]

> -Original Message-
> From: David Ahern [mailto:d...@cumulusnetworks.com]
> Sent: Monday, November 28, 2016 1:07 PM
> To: 张胜举 <zhangshen...@cmss.chinamobile.com>;
> netdev@vger.kernel.org
> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump
> 
> On 11/27/16 9:50 PM, 张胜举 wrote:
> > No, when dump request must be processed by multiple 'recv/recvmsg'
> > system calls, idx stores which dev/neigh the previous call have
> > processed, so that next call will scan from the right place.
> 
> I have tested multiple calls and I do not see redundant information or
missing
> information.
> 
> >
> > So no matter whether the dev/neigh is filtered, the idx should be
> > increased anyway.
> 
> No, it does not. Again, idx is the index in the list of devices/ of
interest. It is
> NOT a device index nor is it the absolute index in the list. It is a
relative index.
> The filter is the same across recvmsg calls so the idx count is absolutely
fine.
> 
> Produce a test case that fails.
David, I know your point. And I agree with you that this will not make 
redundant or missing link information.

But this will cause the filtered out device be scanned multiple times. 

For example, assume that netlink message can only store two devices info.

And eth2-eth5 are filtered out.

For the first loop, idx will point to eth2, but the code already scan to
eth6.
eth0->eth1->eth2(out)->eth3(out)-> eth4(out)->eth5(out)->eth6->eth7
 ^
The next loop, the code will start to scan from eth2 to eth8, but eth2-eth5 
already scanned by previous loop. After this loop, idx will point to eth4.
eth0->eth1->eth2(out)->eth3(out)->eth4(out)->eth5(out)->eth6->eth7->eth8
  ^
So this will cause the same device to be scanned multiple times.

Almost all other dump functions treat idx as the absolute index in the list,

and will not have the above problem. 

We don't treat this a bugfix, but i think we'd better in line with other 
dump functions.

RE: [net,v2] neigh: fix the loop index error in neigh dump

[张胜举]

> -Original Message-
> From: David Ahern [mailto:d...@cumulusnetworks.com]
> Sent: Monday, November 28, 2016 11:10 AM
> To: 张胜举 <zhangshen...@cmss.chinamobile.com>;
> netdev@vger.kernel.org
> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump
> 
> On 11/27/16 7:56 PM, David Ahern wrote:
> > On 11/27/16 7:53 PM, 张胜举 wrote:
> >>
> >>
> >>> -Original Message-
> >>> From: David Ahern [mailto:d...@cumulusnetworks.com]
> >>> Sent: Monday, November 28, 2016 10:39 AM
> >>> To: 张胜举 <zhangshen...@cmss.chinamobile.com>;
> >>> netdev@vger.kernel.org
> >>> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump
> >>>
> >>> On 11/27/16 7:34 PM, 张胜举 wrote:
> >>>>> -Original Message-
> >>>>> From: David Ahern [mailto:d...@cumulusnetworks.com]
> >>>>> Sent: Monday, November 28, 2016 10:10 AM
> >>>>> To: Zhang Shengju <zhangshen...@cmss.chinamobile.com>;
> >>>>> netdev@vger.kernel.org
> >>>>> Subject: Re: [net,v2] neigh: fix the loop index error in neigh
> >>>>> dump
> >>>>>
> >>>>> On 11/27/16 6:32 PM, Zhang Shengju wrote:
> >>>>>> Loop index in neigh dump function is not updated correctly under
> >>>>>> some circumstances, this patch will fix it.
> >>>>>
> >>>>> What's an example?
> >>>>
> >>>> If dev is filtered out, the original code goes to next loop without
> >>>> updating loop index 'idx'.
> >>>
> >>> And you have a use case with missing or redundant data? Or is your
> >>> comment based on a review of code only?
> >> It's on my code review. No use case currently,  this is uncommon to
> happen.
> >>
> >>
> >>>
> >>>>> You are completely rewriting the dump loops.
> >>>>
> >>>> I put 'idx++' into for loop,  so I replace 'goto' with 'continue'.
> >>>> The other change is style related.
> >>>
> >>> A "fixes" should not include 'style related' changes.
> >> Okay, I will send another version without style changes.
> >>
> >
> > Personally, I think you need to produce a use case that fails before
sending
> another patch. I have not seen a problem with this code.
> >
> 
> And looking back at 3f0ae05d6f I should not have acked it (reviewed it too
> quickly while on PTO). Your change is a no-op because of what idx
represents
> - the position in the hash list for devices relevant for the dump request.
> Same goes for the neigh dump so this patch is not needed.
> 
No, when dump request must be processed by multiple 'recv/recvmsg' system
calls, 
idx stores which dev/neigh the previous call have processed, so that next
call will scan 
from the right place.  

So no matter whether the dev/neigh is filtered, the idx should be increased
anyway.

It's hard to produce a use case, because we mostly have only one entity in
hash list. Even with
multiple entities, we also need the function to exit right at the place
where dev/neigh is filter out.

All other dump functiones for RT netlink keep this logic, you can refer
inet_dump_ifaddr() if you wish.

RE: [net,v2] neigh: fix the loop index error in neigh dump

[张胜举]

> -Original Message-
> From: David Ahern [mailto:d...@cumulusnetworks.com]
> Sent: Monday, November 28, 2016 10:39 AM
> To: 张胜举 <zhangshen...@cmss.chinamobile.com>;
> netdev@vger.kernel.org
> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump
> 
> On 11/27/16 7:34 PM, 张胜举 wrote:
> >> -Original Message-
> >> From: David Ahern [mailto:d...@cumulusnetworks.com]
> >> Sent: Monday, November 28, 2016 10:10 AM
> >> To: Zhang Shengju <zhangshen...@cmss.chinamobile.com>;
> >> netdev@vger.kernel.org
> >> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump
> >>
> >> On 11/27/16 6:32 PM, Zhang Shengju wrote:
> >>> Loop index in neigh dump function is not updated correctly under
> >>> some circumstances, this patch will fix it.
> >>
> >> What's an example?
> >
> > If dev is filtered out, the original code goes to next loop without
> > updating loop index 'idx'.
> 
> And you have a use case with missing or redundant data? Or is your
> comment based on a review of code only?
It's on my code review. No use case currently,  this is uncommon to happen.


> 
> >> You are completely rewriting the dump loops.
> >
> > I put 'idx++' into for loop,  so I replace 'goto' with 'continue'.
> > The other change is style related.
> 
> A "fixes" should not include 'style related' changes.
Okay, I will send another version without style changes.

RE: [net,v2] neigh: fix the loop index error in neigh dump

[张胜举]

> -Original Message-
> From: David Ahern [mailto:d...@cumulusnetworks.com]
> Sent: Monday, November 28, 2016 10:10 AM
> To: Zhang Shengju ;
> netdev@vger.kernel.org
> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump
> 
> On 11/27/16 6:32 PM, Zhang Shengju wrote:
> > Loop index in neigh dump function is not updated correctly under some
> > circumstances, this patch will fix it.
> 
> What's an example?

If dev is filtered out, the original code goes to next loop without updating
loop index 'idx'.

> 
> >
> > Fixes: 16660f0bd9 ("net: Add support for filtering neigh dump by
> > device index")
> > Fixes: 21fdd092ac ("net: Add support for filtering neigh dump by
> > master device")
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  net/core/neighbour.c | 39 ++-
> >  1 file changed, 18 insertions(+), 21 deletions(-)
> >
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c index
> > 2ae929f..ce32e9c 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -2256,6 +2256,16 @@ static bool neigh_ifindex_filtered(struct
> net_device *dev, int filter_idx)
> > return false;
> >  }
> >
> > +static bool neigh_dump_filtered(struct net_device *dev, int filter_idx,
> > +   int filter_master_idx)
> > +{
> > +   if (neigh_ifindex_filtered(dev, filter_idx) ||
> > +   neigh_master_filtered(dev, filter_master_idx))
> > +   return true;
> > +
> > +   return false;
> > +}
> > +
> >  static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff
*skb,
> > struct netlink_callback *cb)
> >  {
> > @@ -2285,20 +2295,15 @@ static int neigh_dump_table(struct neigh_table
> *tbl, struct sk_buff *skb,
> > rcu_read_lock_bh();
> > nht = rcu_dereference_bh(tbl->nht);
> >
> > -   for (h = s_h; h < (1 << nht->hash_shift); h++) {
> > -   if (h > s_h)
> > -   s_idx = 0;
> > +   for (h = s_h; h < (1 << nht->hash_shift); h++, s_idx = 0) {
> > for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0;
> >  n != NULL;
> > -n = rcu_dereference_bh(n->next)) {
> > -   if (!net_eq(dev_net(n->dev), net))
> > -   continue;
> > -   if (neigh_ifindex_filtered(n->dev, filter_idx))
> > +n = rcu_dereference_bh(n->next), idx++) {
> > +   if (idx < s_idx || !net_eq(dev_net(n->dev), net))
> > continue;
> > -   if (neigh_master_filtered(n->dev,
filter_master_idx))
> > +   if (neigh_dump_filtered(n->dev, filter_idx,
> > +   filter_master_idx))
> > continue;
> > -   if (idx < s_idx)
> > -   goto next;
> > if (neigh_fill_info(skb, n, NETLINK_CB(cb-
> >skb).portid,
> > cb->nlh->nlmsg_seq,
> > RTM_NEWNEIGH,
> > @@ -2306,8 +2311,6 @@ static int neigh_dump_table(struct neigh_table
> *tbl, struct sk_buff *skb,
> > rc = -1;
> > goto out;
> > }
> > -next:
> > -   idx++;
> > }
> > }
> > rc = skb->len;
> > @@ -2328,14 +2331,10 @@ static int pneigh_dump_table(struct
> > neigh_table *tbl, struct sk_buff *skb,
> >
> > read_lock_bh(>lock);
> >
> > -   for (h = s_h; h <= PNEIGH_HASHMASK; h++) {
> > -   if (h > s_h)
> > -   s_idx = 0;
> > -   for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next) {
> > -   if (pneigh_net(n) != net)
> > +   for (h = s_h; h <= PNEIGH_HASHMASK; h++, s_idx = 0) {
> > +   for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next,
idx++)
> {
> > +   if (idx < s_idx || pneigh_net(n) != net)
> > continue;
> > -   if (idx < s_idx)
> > -   goto next;
> > if (pneigh_fill_info(skb, n, NETLINK_CB(cb-
> >skb).portid,
> > cb->nlh->nlmsg_seq,
> > RTM_NEWNEIGH,
> > @@ -2344,8 +2343,6 @@ static int pneigh_dump_table(struct neigh_table
> *tbl, struct sk_buff *skb,
> > rc = -1;
> > goto out;
> > }
> > -   next:
> > -   idx++;
> > }
> > }
> >
> 
> This fix is way to be complicated to be fixing anything related to
16660f0bd9
> or 21fdd092ac. Both of those commits added a continue:
> 
> if (neigh_ifindex_filtered(n->dev, filter_idx))
> continue;
> if (neigh_master_filtered(n->dev,

RE: [net-next] rtnl: fix the loop index update error in rtnl_dump_ifinfo()

[张胜举]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Sunday, November 20, 2016 11:15 AM
> To: zhangshen...@cmss.chinamobile.com
> Cc: netdev@vger.kernel.org; d...@cumulusnetworks.com
> Subject: Re: [net-next] rtnl: fix the loop index update error in
> rtnl_dump_ifinfo()
> 
> From: Zhang Shengju 
> Date: Sat, 19 Nov 2016 23:28:32 +0800
> 
> > If the link is filtered out, loop index should also be updated. If
> > not, loop index will not be correct.
> >
> > Signed-off-by: Zhang Shengju 
> 
> Applied to 'net' and queued up for -stable.
> 
> Bug fixes should always be targetted at 'net' not 'net-next'.

Thanks David, I will pay attention to this next time.

BRs,
Zhang Shengju

Re: [net-next] bond: output message before setting slave to inactive

[张胜举]

> From: Zhang Shengju 
> Date: Tue, 29 Mar 2016 06:32:57 +
> 
> > This patch moves output message before setting slave to inactive, this
> > will print the correct status of slave device.
> >
> > Signed-off-by: Zhang Shengju 
> 
> I think the message is in the appropriate spot wrt. state, so I will not
apply
> this, thanks.

The message will always print 'backup',  since the code set it to inactive
before this output.
I think this will cause a little confused, because even when I detach an
active slave, the output is always 'backup'.

Thanks.

Re: [net-next 1/2] bonding: remove duplicate set of flag IFF_MULTICAST

[张胜举]

> On 03/16/2016 10:59 AM, Zhang Shengju wrote:
> > Remove unnecessary set of flag IFF_MULTICAST, since ether_setup
> > already does this.
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  drivers/net/bonding/bond_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> 
> There're a few more bonding maintainers, I've added them to the CC list.
> 
> Reviewed-by: Nikolay Aleksandrov 

Thanks, Nikolay.

RE: [net-next,iproute2] netconf: add support for ignore route attribute

[张胜举]

> On Mon, 14 Mar 2016 04:55:36 +
> Zhang Shengju  wrote:
> 
> > Add support for ignore_routes_with_linkdown attribute.
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  ip/ipnetconf.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/ip/ipnetconf.c b/ip/ipnetconf.c index eca6eee..6fec818
> > 100644
> > --- a/ip/ipnetconf.c
> > +++ b/ip/ipnetconf.c
> > @@ -119,6 +119,10 @@ int print_netconf(const struct sockaddr_nl *who,
> struct rtnl_ctrl_data *ctrl,
> > fprintf(fp, "proxy_neigh %s ",
> > *(int
> *)RTA_DATA(tb[NETCONFA_PROXY_NEIGH])?"on":"off");
> >
> > +   if (tb[NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN])
> > +   fprintf(fp, "ignore_routes_with_linkdown %s ",
> > +   *(int
> >
> +*)RTA_DATA(tb[NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN])?"on":"
> off");
> 
> This is a good idea.
> 
> But the option name is too long, and the code does not follow current best
> practices.
I agree with you that the name is too long, but I can't figure out a shorter
name. 
Any good suggestion? What about "ignore_routes" ? 

>   1. Lines are too long
>   2. There needs to be whitespace around ? :
>   3. There are helper routines (rte_getattr_XXX) which should be used
rather
> than
>  cast RTE_DATA directly.
> 
> Also, help and man page??
Yes, man page need to be enhanced.

Re: [net-next] arp: correct return value of arp_rcv

[张胜举]

> From: Zhang Shengju 
> Date: Tue,  1 Mar 2016 07:24:42 +
> 
> > Currently, arp_rcv() always return zero on a packet delivery upcall.
> >
> > To make its behavior more compliant with the way this API should be
> > used, this patch changes this to let it return NET_RX_SUCCESS when the
> > packet is proper handled, and NET_RX_DROP otherwise.
> >
> > Signed-off-by: Zhang Shengju 
> 
> This change is bogus.
> 
> > @@ -880,7 +880,7 @@ out:
> > consume_skb(skb);
> >  out_free_dst:
> > dst_release(reply_dst);
> > -   return 0;
> > +   return NET_RX_SUCCESS;
> 
> Most of the paths to out: are dropping the request for one reason or
> another.
> 
> If you want to do this right you need to split these two exit paths,
convert
> consume_skb() to kfree_skb() in the drop path, and then you can return
> accurate NET_* codes.

You are right, I will change this part and sent another version later.

Shengju

Re: [net] net: fix double free issue of skbuff

[张胜举]

> On Mon, 2016-02-29 at 12:22 +, Zhang Shengju wrote:
> > If skb_reorder_vlan_header() failed, skb is freed and NULL is returned.
> > Then at skb_vlan_untag(), it will free skbuff again which cause double
> > free.
> 
> On skb_reorder_vlan_header() failure, skb_vlan_untag() will call
> kfree_skb() using the return value of skb_reorder_vlan_header(), that is
> NULL. kfree_skb() is a noop when the argument is NULL.
> 
> The current code seams safe.
> 
> Paolo
Hi Paolo, even current code is safe, this's still a potential problem. We 
should make an
assumption that inner function doesn't free skb, and let outside function take 
care of this.

BRs, 
Shengju

Re: [net] net: fix double free issue of skbuff

[张胜举]

> Hello.
> 
> On 2/29/2016 3:22 PM, Zhang Shengju wrote:
> 
> > If skb_reorder_vlan_header() failed, skb is freed and NULL is returned.
> > Then at skb_vlan_untag(), it will free skbuff again which cause double
> > free.
> >
> > This patch removes kfree_skb() call in function
> skb_reorder_vlan_header().
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >   net/core/skbuff.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c index
> > 488566b..1312d4b 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -4350,7 +4350,6 @@
> EXPORT_SYMBOL_GPL(skb_gso_transport_seglen);
> >   static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
> >   {
> > if (skb_cow(skb, skb_headroom(skb)) < 0) {
> > -   kfree_skb(skb);
> > return NULL;
> > }
> 
> You now need to remove {}.
> 
> MBR, Sergei

Thanks Sergei, I will add this in v2. 

BRs,
Shengju

Re: [net-next] vlan: change return type of vlan_proc_rem_dev

[张胜举]

> 
> On Wed, Feb 17, 2016 at 3:44 AM, Zhang Shengju
>  wrote:
> > Since function vlan_proc_rem_dev() will only return 0, it's better to
> > return void instead of int.
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  net/8021q/vlanproc.c | 3 +--
> >  net/8021q/vlanproc.h | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/8021q/vlanproc.c b/net/8021q/vlanproc.c index
> > ae63cf7..5f1446c 100644
> > --- a/net/8021q/vlanproc.c
> > +++ b/net/8021q/vlanproc.c
> > @@ -184,12 +184,11 @@ int vlan_proc_add_dev(struct net_device
> > *vlandev)
> >  /*
> >   * Delete directory entry for VLAN device.
> >   */
> > -int vlan_proc_rem_dev(struct net_device *vlandev)
> > +void vlan_proc_rem_dev(struct net_device *vlandev)
> >  {
> > /** NOTE:  This will consume the memory pointed to by dent, it 
> > seems.
> */
> > proc_remove(vlan_dev_priv(vlandev)->dent);
> > vlan_dev_priv(vlandev)->dent = NULL;
> > -   return 0;
> >  }
> >
> >  /** Proc filesystem entry points
> > /
> > diff --git a/net/8021q/vlanproc.h b/net/8021q/vlanproc.h index
> > 063f60a..a9d8734 100644
> > --- a/net/8021q/vlanproc.h
> > +++ b/net/8021q/vlanproc.h
> > @@ -5,7 +5,7 @@
> >  struct net;
> >
> >  int vlan_proc_init(struct net *net);
> > -int vlan_proc_rem_dev(struct net_device *vlandev);
> > +void vlan_proc_rem_dev(struct net_device *vlandev);
> >  int vlan_proc_add_dev(struct net_device *vlandev);  void
> > vlan_proc_cleanup(struct net *net);
> 
> You forget to change the !PROC_FS case:
> 
> #define vlan_proc_rem_dev(dev)  ({(void)(dev), 0; })

Thanks, I will add the missing part. 

Reply: [net] bonding: use return instead of goto

[张胜举]

> On Fri, Feb 05, 2016 at 09:42:24AM +0800, 张胜举 wrote:
> > > On Wed, Feb 03, 2016 at 06:15:22AM +, Zhang Shengju wrote:
> > > > Replace 'goto' with 'return' to remove unnecessary check at label:
> > > > err_undo_flags.
> > >
> > > I think you're going to have to explain how you came to the
> > > conclusion that the check isn't necessary.
> ...
> > The reason is that 'err_undo_flags' do two things for the first slave
> > device:
> > 1. revert bond mac address if it is set by the slave device.
> > 2. revert bond device type if it's not ARPHRD_ETHER.
> >
> > I think it's not necessary for the three places, they changed neither
> > bond mac address nor type.
> > it's straightforward to return directly.
> 
> I see what you're saying, and that does look to be true if you're only adding 
> a
> singular first device right now. But looking at the enslave and release 
> paths, I
> don't see anything preventing concurrent slave adds and removes, which
> could mean there are situations where those checks really are necessary. I
> don't actually know.
> 
> --
> Jarod Wilson
> ja...@redhat.com

All of three places don't change any attributes of bond device, 
just for sanity check before enslaving.  
If it's necessary for these three places to goto 'err_undo_flags',  
it will mean that all sanity checks before these three places need to do so, 
which is not right clearly. 

Even there is concurrent situation which make the check necessary, which I 
haven't figure out yet,
it's not right place to do this.  

Thanks,
Shengju

Reply: [net] bonding: use return instead of goto

[张胜举]

> On Wed, Feb 03, 2016 at 06:15:22AM +, Zhang Shengju wrote:
> > Replace 'goto' with 'return' to remove unnecessary check at label:
> > err_undo_flags.
> 
> I think you're going to have to explain how you came to the conclusion
that
> the check isn't necessary.
> 
> --
> Jarod Wilson
> ja...@redhat.com
Hi Jarod,

The reason is that 'err_undo_flags' do two things for the first slave
device:
1. revert bond mac address if it is set by the slave device.
2. revert bond device type if it's not ARPHRD_ETHER.

I think it's not necessary for the three places, they changed neither  bond
mac address nor type. 
it's straightforward to return directly.

Thanks,
Shengju

Reply: [PATCH iproute2] ip-link: remove warning message

[张胜举]

> On Thu, 21 Jan 2016 02:23:49 +
> Zhang Shengju  wrote:
> 
> > the warning was:
> > iproute.c:301:12: warning: 'val' may be used uninitialized in this
> > function [-Wmaybe-uninitialized]
> >features &= ~RTAX_FEATURE_ECN;
> > ^
> > iproute.c:575:10: note: 'val' was declared here
> >__u32 val;
> >   ^
> >
> > Signed-off-by: Zhang Shengju 
> > ---
> >  ip/iproute.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ip/iproute.c b/ip/iproute.c index d5e3ebe..afe70e1 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -572,7 +572,7 @@ int print_route(const struct sockaddr_nl *who,
> struct nlmsghdr *n, void *arg)
> > mxlock =
> *(unsigned*)RTA_DATA(mxrta[RTAX_LOCK]);
> >
> > for (i=2; i<= RTAX_MAX; i++) {
> > -   __u32 val;
> > +   __u32 val = 0U;
> >
> > if (mxrta[i] == NULL)
> > continue;
> 
> Your compiler is doing bad dependency analysis.
> There is not really a bug here.
> 
> It would still be best to initialize to keep broken compilers from causing
> warning.

Yes, it's not really a bug here. This variable is set by all means.  This
patch just want to remove the warning. 

Thanks,
Shengju

RE: [PATCH iproute2] ip: replace exit with return

[张胜举]

 
 From: netdev-ow...@vger.kernel.org
  Sent: 11 August 2015 10:40
  In our manual, we have this description of 'EXIT STATUS':
  Exit status is 0 if command was successful, and 1 if there is a syntax
  error.
 
  But we exit in command functions with code -1 when there is a syntax
error.
  It's better to use return.
 
 Eh?
 Using exit() makes it much more obvious that the program is going to exit.
 
 I've not looked at the call site (I'm not entirely sure where this code is
in the
 source tree), but main() shouldn't return -1 any more than exit(-1) is
invalid.
 The domain for both is 0..127.
 So the code should be using a valid value.

1. Using exit(-1) will make program exit with -1. 
With return -1, the do_cmd() function will make sure 1 is returned.  
do_cmd()
{

return -(c-func(argc-1, argv+1));

}
2.  Replace with return will confirm with manual description like I said in
last mail.

BRs,
Zhang
 
 ...
  diff --git a/ip/ipaddress.c b/ip/ipaddress.c index b7b4e3e..6d29a69
  100644
  --- a/ip/ipaddress.c
  +++ b/ip/ipaddress.c
  @@ -1879,5 +1879,5 @@ int do_ipaddr(int argc, char **argv)
  if (matches(*argv, help) == 0)
  usage();
  fprintf(stderr, Command \%s\ is unknown, try \ip addr
help\.\n,
 *argv);
  -   exit(-1);
  +   return -1;
   }
 ...
 
   David



--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html