| Message ID | 20190611161031.12898-1-mcroce@redhat.com |
|---|---|
| Headers | show |
| Series | refactor the cmd_exec() | expand |
On Tue, Jun 11, 2019 at 6:11 PM Matteo Croce <mcroce@redhat.com> wrote: > > Refactor the netns and ipvrf code so less steps are needed to exec commands > in a netns or a VRF context. > Also remove some code which became dead. bloat-o-meter output: > > $ bloat-o-meter ip.old ip > add/remove: 1/4 grow/shrink: 3/4 up/down: 174/-312 (-138) > Function old new delta > netns_add 971 1058 +87 > cmd_exec 207 256 +49 > on_netns_exec 32 60 +28 > do_switch - 10 +10 > netns_restore 69 67 -2 > do_ipvrf 811 802 -9 > netns_switch 838 822 -16 > on_netns_label 45 - -45 > do_netns 1226 1180 -46 > do_each_netns 57 - -57 > on_netns 60 - -60 > netns_save 77 - -77 > Total: Before=668234, After=668096, chg -0.02% > > Matteo Croce (3): > netns: switch netns in the child when executing commands > ip vrf: use hook to change VRF in the child > netns: make netns_{save,restore} static > > include/namespace.h | 2 -- > include/utils.h | 6 ++--- > ip/ip.c | 1 - > ip/ipnetns.c | 56 +++++++++++++++++++++++++++++++++------------ > ip/ipvrf.c | 12 ++++++---- > lib/exec.c | 7 +++++- > lib/namespace.c | 31 ------------------------- > lib/utils.c | 27 ---------------------- > 8 files changed, 58 insertions(+), 84 deletions(-) > > -- > 2.21.0 > For patch series: Reviewed-and-tested-by: Andrea Claudi <aclaudi@redhat.com>
On 6/11/19 10:10 AM, Matteo Croce wrote: > Refactor the netns and ipvrf code so less steps are needed to exec commands > in a netns or a VRF context. > Also remove some code which became dead. bloat-o-meter output: > This breaks the vrf reset after namespace switch # ip vrf ls Name Table ----------------------- red 1001 Set shell into vrf red context: # ip vrf exec red bash Add new namespace and do netns exec: # ip netns add foo # ./ip netns exec foo bash Check the vrf id: # ip vrf id red With the current command: # ip netns exec foo bash # ip vrf id <nothing - no vrf bind>
On Fri, Jun 14, 2019 at 4:36 PM David Ahern <dsahern@gmail.com> wrote: > > On 6/11/19 10:10 AM, Matteo Croce wrote: > > Refactor the netns and ipvrf code so less steps are needed to exec commands > > in a netns or a VRF context. > > Also remove some code which became dead. bloat-o-meter output: > > > > This breaks the vrf reset after namespace switch > > > # ip vrf ls > Name Table > ----------------------- > red 1001 > > Set shell into vrf red context: > # ip vrf exec red bash > > Add new namespace and do netns exec: > # ip netns add foo > # ./ip netns exec foo bash > > Check the vrf id: > # ip vrf id > red > > With the current command: > # ip netns exec foo bash > # ip vrf id > <nothing - no vrf bind> Hi David, if the vrf needs to be reset after a netns change, why don't we do in netns_switch()? This way all code paths will be covered. Cheers,
On Sat, Jun 15, 2019 at 4:06 PM Matteo Croce <mcroce@redhat.com> wrote: > > On Fri, Jun 14, 2019 at 4:36 PM David Ahern <dsahern@gmail.com> wrote: > > > > On 6/11/19 10:10 AM, Matteo Croce wrote: > > > Refactor the netns and ipvrf code so less steps are needed to exec commands > > > in a netns or a VRF context. > > > Also remove some code which became dead. bloat-o-meter output: > > > > > > > This breaks the vrf reset after namespace switch > > > > > > # ip vrf ls > > Name Table > > ----------------------- > > red 1001 > > > > Set shell into vrf red context: > > # ip vrf exec red bash > > > > Add new namespace and do netns exec: > > # ip netns add foo > > # ./ip netns exec foo bash > > > > Check the vrf id: > > # ip vrf id > > red > > > > With the current command: > > # ip netns exec foo bash > > # ip vrf id > > <nothing - no vrf bind> > > Hi David, > > if the vrf needs to be reset after a netns change, why don't we do in > netns_switch()? > This way all code paths will be covered. > > Cheers, > -- > Matteo Croce > per aspera ad upstream Hi David, I looked for every occourrence of netns_switch(): bridge/bridge.c: if (netns_switch(argv[1])) include/namespace.h:int netns_switch(char *netns); ip/ip.c: if (netns_switch(argv[1])) ip/ipnetns.c: if (netns_switch(argv[0])) lib/namespace.c:int netns_switch(char *name) lib/utils.c: if (netns_switch(nsname)) misc/ss.c: if (netns_switch(optarg)) tc/tc.c: if (netns_switch(argv[1])) the ones in {ss,tc,bridge,ip}.c are obviously handling the '-n netns' command line option. my question here is: should the VRF association be reset in all these cases? If we need to reset, we should really call vrf_reset() from netns_switch(). If don't, I can add the missing vrf_reset() in ipnetns.c but I'm curious to know what can happen to change netns and keep VRF associations belonging to another netns. Regards,
On 6/17/19 4:20 PM, Matteo Croce wrote: > > I looked for every occourrence of netns_switch(): > > bridge/bridge.c: if (netns_switch(argv[1])) > include/namespace.h:int netns_switch(char *netns); > ip/ip.c: if (netns_switch(argv[1])) > ip/ipnetns.c: if (netns_switch(argv[0])) > lib/namespace.c:int netns_switch(char *name) > lib/utils.c: if (netns_switch(nsname)) > misc/ss.c: if (netns_switch(optarg)) > tc/tc.c: if (netns_switch(argv[1])) > > the ones in {ss,tc,bridge,ip}.c are obviously handling the '-n netns' > command line option. > my question here is: should the VRF association be reset in all these cases? > If we need to reset, we should really call vrf_reset() from netns_switch(). > If don't, I can add the missing vrf_reset() in ipnetns.c but I'm > curious to know what can happen to change netns and keep VRF > associations belonging to another netns. > The VRF reset is needed on a netns exec - ie., running a command in a network namespace. Any netns_switch that only involves sending a netlink command in a different namespace does not need the reset.
Refactor the netns and ipvrf code so less steps are needed to exec commands in a netns or a VRF context. Also remove some code which became dead. bloat-o-meter output: $ bloat-o-meter ip.old ip add/remove: 1/4 grow/shrink: 3/4 up/down: 174/-312 (-138) Function old new delta netns_add 971 1058 +87 cmd_exec 207 256 +49 on_netns_exec 32 60 +28 do_switch - 10 +10 netns_restore 69 67 -2 do_ipvrf 811 802 -9 netns_switch 838 822 -16 on_netns_label 45 - -45 do_netns 1226 1180 -46 do_each_netns 57 - -57 on_netns 60 - -60 netns_save 77 - -77 Total: Before=668234, After=668096, chg -0.02% Matteo Croce (3): netns: switch netns in the child when executing commands ip vrf: use hook to change VRF in the child netns: make netns_{save,restore} static include/namespace.h | 2 -- include/utils.h | 6 ++--- ip/ip.c | 1 - ip/ipnetns.c | 56 +++++++++++++++++++++++++++++++++------------ ip/ipvrf.c | 12 ++++++---- lib/exec.c | 7 +++++- lib/namespace.c | 31 ------------------------- lib/utils.c | 27 ---------------------- 8 files changed, 58 insertions(+), 84 deletions(-)