netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH iproute2 v2 0/3] refactor the cmd_exec()
@ 2019-06-11 16:10 Matteo Croce
  2019-06-11 16:10 ` [PATCH iproute2 v2 1/3] netns: switch netns in the child when executing commands Matteo Croce
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Matteo Croce @ 2019-06-11 16:10 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, David Ahern

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH iproute2 v2 1/3] netns: switch netns in the child when executing commands
  2019-06-11 16:10 [PATCH iproute2 v2 0/3] refactor the cmd_exec() Matteo Croce
@ 2019-06-11 16:10 ` Matteo Croce
  2019-06-11 16:10 ` [PATCH iproute2 v2 2/3] ip vrf: use hook to change VRF in the child Matteo Croce
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-06-11 16:10 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, David Ahern

'ip netns exec' changes the current netns just before executing a child
process, and restores it after forking. This is needed if we're running
in batch or do_all mode.
Some cleanups must be done both in the parent and in the child: the
parent must restore the previous netns, while the child must reset any
VRF association.
Unfortunately, if do_all is set, the VRF are not reset in the child, and
the spawned processes are started with the wrong VRF context. This can
be triggered with this script:

	# ip -b - <<-'EOF'
		link add type vrf table 100
		link set vrf0 up
		link add type dummy
		link set dummy0 vrf vrf0 up
		netns add ns1
	EOF
	# ip -all -b - <<-'EOF'
		vrf exec vrf0 true
		netns exec setsid -f sleep 1h
	EOF
	# ip vrf pids vrf0
	  314  sleep
	# ps 314
	  PID TTY      STAT   TIME COMMAND
	  314 ?        Ss     0:00 sleep 1h

Refactor cmd_exec() and pass to it a function pointer which is called in
the child before the final exec. In the netns exec case the function just
resets the VRF and switches netns.

Doing it in the child is less error prone and safer, because the parent
environment is always kept unaltered.

After this refactor some utility functions became unused, so remove them.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 include/utils.h |  6 ++----
 ip/ipnetns.c    | 30 ++++++++++++++++--------------
 ip/ipvrf.c      |  2 +-
 lib/exec.c      |  7 ++++++-
 lib/utils.c     | 27 ---------------------------
 5 files changed, 25 insertions(+), 47 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 8a9c3020..927fdc17 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -294,14 +294,12 @@ extern int cmdlineno;
 ssize_t getcmdline(char **line, size_t *len, FILE *in);
 int makeargs(char *line, char *argv[], int maxargs);
 
-int do_each_netns(int (*func)(char *nsname, void *arg), void *arg,
-		bool show_label);
-
 char *int_to_str(int val, char *buf);
 int get_guid(__u64 *guid, const char *arg);
 int get_real_family(int rtm_type, int rtm_family);
 
-int cmd_exec(const char *cmd, char **argv, bool do_fork);
+int cmd_exec(const char *cmd, char **argv, bool do_fork,
+	     int (*setup)(void *), void *arg);
 int make_path(const char *path, mode_t mode);
 char *find_cgroup2_mount(void);
 int get_command_name(const char *pid, char *comm, size_t len);
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 8ead0c4c..58655676 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -396,11 +396,24 @@ static int netns_list(int argc, char **argv)
 	return 0;
 }
 
+static int do_switch(void *arg)
+{
+	char *netns = arg;
+
+	/* we just changed namespaces. clear any vrf association
+	 * with prior namespace before exec'ing command
+	 */
+	vrf_reset();
+
+	return netns_switch(netns);
+}
+
 static int on_netns_exec(char *nsname, void *arg)
 {
 	char **argv = arg;
 
-	cmd_exec(argv[1], argv + 1, true);
+	printf("\nnetns: %s\n", nsname);
+	cmd_exec(argv[0], argv, true, do_switch, nsname);
 	return 0;
 }
 
@@ -409,8 +422,6 @@ static int netns_exec(int argc, char **argv)
 	/* Setup the proper environment for apps that are not netns
 	 * aware, and execute a program in that environment.
 	 */
-	const char *cmd;
-
 	if (argc < 1 && !do_all) {
 		fprintf(stderr, "No netns name specified\n");
 		return -1;
@@ -421,22 +432,13 @@ static int netns_exec(int argc, char **argv)
 	}
 
 	if (do_all)
-		return do_each_netns(on_netns_exec, --argv, 1);
-
-	if (netns_switch(argv[0]))
-		return -1;
-
-	/* we just changed namespaces. clear any vrf association
-	 * with prior namespace before exec'ing command
-	 */
-	vrf_reset();
+		return netns_foreach(on_netns_exec, argv);
 
 	/* ip must return the status of the child,
 	 * but do_cmd() will add a minus to this,
 	 * so let's add another one here to cancel it.
 	 */
-	cmd = argv[1];
-	return -cmd_exec(cmd, argv + 1, !!batch_mode);
+	return -cmd_exec(argv[1], argv + 1, !!batch_mode, do_switch, argv[0]);
 }
 
 static int is_pid(const char *str)
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index a13cf653..2b019c6c 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -456,7 +456,7 @@ static int ipvrf_exec(int argc, char **argv)
 	if (vrf_switch(argv[0]))
 		return -1;
 
-	return -cmd_exec(argv[1], argv + 1, !!batch_mode);
+	return -cmd_exec(argv[1], argv + 1, !!batch_mode, NULL, NULL);
 }
 
 /* reset VRF association of current process to default VRF;
diff --git a/lib/exec.c b/lib/exec.c
index eb36b59d..9b1c8f4a 100644
--- a/lib/exec.c
+++ b/lib/exec.c
@@ -5,8 +5,10 @@
 #include <unistd.h>
 
 #include "utils.h"
+#include "namespace.h"
 
-int cmd_exec(const char *cmd, char **argv, bool do_fork)
+int cmd_exec(const char *cmd, char **argv, bool do_fork,
+	     int (*setup)(void *), void *arg)
 {
 	fflush(stdout);
 	if (do_fork) {
@@ -34,6 +36,9 @@ int cmd_exec(const char *cmd, char **argv, bool do_fork)
 		}
 	}
 
+	if (setup && setup(arg))
+		return -1;
+
 	if (execvp(cmd, argv)  < 0)
 		fprintf(stderr, "exec of \"%s\" failed: %s\n",
 				cmd, strerror(errno));
diff --git a/lib/utils.c b/lib/utils.c
index a81c0700..be0f11b0 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1418,33 +1418,6 @@ void print_nlmsg_timestamp(FILE *fp, const struct nlmsghdr *n)
 	fprintf(fp, "Timestamp: %s %lu us\n", tstr, usecs);
 }
 
-static int on_netns(char *nsname, void *arg)
-{
-	struct netns_func *f = arg;
-
-	if (netns_switch(nsname))
-		return -1;
-
-	return f->func(nsname, f->arg);
-}
-
-static int on_netns_label(char *nsname, void *arg)
-{
-	printf("\nnetns: %s\n", nsname);
-	return on_netns(nsname, arg);
-}
-
-int do_each_netns(int (*func)(char *nsname, void *arg), void *arg,
-		bool show_label)
-{
-	struct netns_func nsf = { .func = func, .arg = arg };
-
-	if (show_label)
-		return netns_foreach(on_netns_label, &nsf);
-
-	return netns_foreach(on_netns, &nsf);
-}
-
 char *int_to_str(int val, char *buf)
 {
 	sprintf(buf, "%d", val);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH iproute2 v2 2/3] ip vrf: use hook to change VRF in the child
  2019-06-11 16:10 [PATCH iproute2 v2 0/3] refactor the cmd_exec() Matteo Croce
  2019-06-11 16:10 ` [PATCH iproute2 v2 1/3] netns: switch netns in the child when executing commands Matteo Croce
@ 2019-06-11 16:10 ` Matteo Croce
  2019-06-11 16:10 ` [PATCH iproute2 v2 3/3] netns: make netns_{save,restore} static Matteo Croce
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-06-11 16:10 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, David Ahern

On vrf exec, reset the VRF associations in the child process, via the
new hook added to cmd_exec(). In this way, the parent doesn't have to
reset the VRF associations before spawning other processes.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 ip/ipnetns.c |  5 -----
 ip/ipvrf.c   | 12 ++++++++----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 58655676..1fff284e 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -400,11 +400,6 @@ static int do_switch(void *arg)
 {
 	char *netns = arg;
 
-	/* we just changed namespaces. clear any vrf association
-	 * with prior namespace before exec'ing command
-	 */
-	vrf_reset();
-
 	return netns_switch(netns);
 }
 
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index 2b019c6c..43366f6e 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -442,6 +442,13 @@ out:
 	return rc;
 }
 
+static int do_switch(void *arg)
+{
+	char *vrf = arg;
+
+	return vrf_switch(vrf);
+}
+
 static int ipvrf_exec(int argc, char **argv)
 {
 	if (argc < 1) {
@@ -453,10 +460,7 @@ static int ipvrf_exec(int argc, char **argv)
 		return -1;
 	}
 
-	if (vrf_switch(argv[0]))
-		return -1;
-
-	return -cmd_exec(argv[1], argv + 1, !!batch_mode, NULL, NULL);
+	return -cmd_exec(argv[1], argv + 1, !!batch_mode, do_switch, argv[0]);
 }
 
 /* reset VRF association of current process to default VRF;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH iproute2 v2 3/3] netns: make netns_{save,restore} static
  2019-06-11 16:10 [PATCH iproute2 v2 0/3] refactor the cmd_exec() Matteo Croce
  2019-06-11 16:10 ` [PATCH iproute2 v2 1/3] netns: switch netns in the child when executing commands Matteo Croce
  2019-06-11 16:10 ` [PATCH iproute2 v2 2/3] ip vrf: use hook to change VRF in the child Matteo Croce
@ 2019-06-11 16:10 ` Matteo Croce
  2019-06-13 17:07 ` [PATCH iproute2 v2 0/3] refactor the cmd_exec() Andrea Claudi
  2019-06-14 14:35 ` David Ahern
  4 siblings, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-06-11 16:10 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, David Ahern

The netns_{save,restore} functions are only used in ipnetns.c now, since
the restore is not needed anymore after the netns exec command.
Move them in ipnetns.c, and make them static.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 include/namespace.h |  2 --
 ip/ip.c             |  1 -
 ip/ipnetns.c        | 31 +++++++++++++++++++++++++++++++
 lib/namespace.c     | 31 -------------------------------
 4 files changed, 31 insertions(+), 34 deletions(-)

diff --git a/include/namespace.h b/include/namespace.h
index 89cdda11..e47f9b5d 100644
--- a/include/namespace.h
+++ b/include/namespace.h
@@ -49,8 +49,6 @@ static inline int setns(int fd, int nstype)
 }
 #endif /* HAVE_SETNS */
 
-void netns_save(void);
-void netns_restore(void);
 int netns_switch(char *netns);
 int netns_get_fd(const char *netns);
 int netns_foreach(int (*func)(char *nsname, void *arg), void *arg);
diff --git a/ip/ip.c b/ip/ip.c
index 49b3aa49..b71ae816 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -158,7 +158,6 @@ static int batch(const char *name)
 			if (!force)
 				break;
 		}
-		netns_restore();
 	}
 	if (line)
 		free(line);
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 1fff284e..21eb5d38 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -45,6 +45,7 @@ static int usage(void)
 static struct rtnl_handle rtnsh = { .fd = -1 };
 
 static int have_rtnl_getnsid = -1;
+static int saved_netns = -1;
 
 static int ipnetns_accept_msg(struct rtnl_ctrl_data *ctrl,
 			      struct nlmsghdr *n, void *arg)
@@ -630,6 +631,33 @@ static int create_netns_dir(void)
 	return 0;
 }
 
+/* Obtain a FD for the current namespace, so we can reenter it later */
+static void netns_save(void)
+{
+	if (saved_netns != -1)
+		return;
+
+	saved_netns = open("/proc/self/ns/net", O_RDONLY | O_CLOEXEC);
+	if (saved_netns == -1) {
+		perror("Cannot open init namespace");
+		exit(1);
+	}
+}
+
+static void netns_restore(void)
+{
+	if (saved_netns == -1)
+		return;
+
+	if (setns(saved_netns, CLONE_NEWNET)) {
+		perror("setns");
+		exit(1);
+	}
+
+	close(saved_netns);
+	saved_netns = -1;
+}
+
 static int netns_add(int argc, char **argv, bool create)
 {
 	/* This function creates a new network namespace and
@@ -723,9 +751,12 @@ static int netns_add(int argc, char **argv, bool create)
 			proc_path, netns_path, strerror(errno));
 		goto out_delete;
 	}
+	netns_restore();
+
 	return 0;
 out_delete:
 	if (create) {
+		netns_restore();
 		netns_delete(argc, argv);
 	} else if (unlink(netns_path) < 0) {
 		fprintf(stderr, "Cannot remove namespace file \"%s\": %s\n",
diff --git a/lib/namespace.c b/lib/namespace.c
index a2aea57a..06ae0a48 100644
--- a/lib/namespace.c
+++ b/lib/namespace.c
@@ -15,35 +15,6 @@
 #include "utils.h"
 #include "namespace.h"
 
-static int saved_netns = -1;
-
-/* Obtain a FD for the current namespace, so we can reenter it later */
-void netns_save(void)
-{
-	if (saved_netns != -1)
-		return;
-
-	saved_netns = open("/proc/self/ns/net", O_RDONLY | O_CLOEXEC);
-	if (saved_netns == -1) {
-		perror("Cannot open init namespace");
-		exit(1);
-	}
-}
-
-void netns_restore(void)
-{
-	if (saved_netns == -1)
-		return;
-
-	if (setns(saved_netns, CLONE_NEWNET)) {
-		perror("setns");
-		exit(1);
-	}
-
-	close(saved_netns);
-	saved_netns = -1;
-}
-
 static void bind_etc(const char *name)
 {
 	char etc_netns_path[sizeof(NETNS_ETC_DIR) + NAME_MAX];
@@ -90,8 +61,6 @@ int netns_switch(char *name)
 		return -1;
 	}
 
-	netns_save();
-
 	if (setns(netns, CLONE_NEWNET) < 0) {
 		fprintf(stderr, "setting the network namespace \"%s\" failed: %s\n",
 			name, strerror(errno));
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 v2 0/3] refactor the cmd_exec()
  2019-06-11 16:10 [PATCH iproute2 v2 0/3] refactor the cmd_exec() Matteo Croce
                   ` (2 preceding siblings ...)
  2019-06-11 16:10 ` [PATCH iproute2 v2 3/3] netns: make netns_{save,restore} static Matteo Croce
@ 2019-06-13 17:07 ` Andrea Claudi
  2019-06-14 14:35 ` David Ahern
  4 siblings, 0 replies; 12+ messages in thread
From: Andrea Claudi @ 2019-06-13 17:07 UTC (permalink / raw)
  To: Matteo Croce; +Cc: netdev, Stephen Hemminger, David Ahern

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 v2 0/3] refactor the cmd_exec()
  2019-06-11 16:10 [PATCH iproute2 v2 0/3] refactor the cmd_exec() Matteo Croce
                   ` (3 preceding siblings ...)
  2019-06-13 17:07 ` [PATCH iproute2 v2 0/3] refactor the cmd_exec() Andrea Claudi
@ 2019-06-14 14:35 ` David Ahern
  2019-06-15 14:06   ` Matteo Croce
  4 siblings, 1 reply; 12+ messages in thread
From: David Ahern @ 2019-06-14 14:35 UTC (permalink / raw)
  To: Matteo Croce, netdev; +Cc: Stephen Hemminger, David Ahern

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 v2 0/3] refactor the cmd_exec()
  2019-06-14 14:35 ` David Ahern
@ 2019-06-15 14:06   ` Matteo Croce
  2019-06-17 22:20     ` Matteo Croce
  0 siblings, 1 reply; 12+ messages in thread
From: Matteo Croce @ 2019-06-15 14:06 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Stephen Hemminger, David Ahern

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 v2 0/3] refactor the cmd_exec()
  2019-06-15 14:06   ` Matteo Croce
@ 2019-06-17 22:20     ` Matteo Croce
  2019-06-17 22:24       ` David Ahern
  0 siblings, 1 reply; 12+ messages in thread
From: Matteo Croce @ 2019-06-17 22:20 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Stephen Hemminger, David Ahern

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,
-- 
Matteo Croce
per aspera ad upstream

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 v2 0/3] refactor the cmd_exec()
  2019-06-17 22:20     ` Matteo Croce
@ 2019-06-17 22:24       ` David Ahern
  0 siblings, 0 replies; 12+ messages in thread
From: David Ahern @ 2019-06-17 22:24 UTC (permalink / raw)
  To: Matteo Croce; +Cc: netdev, Stephen Hemminger, David Ahern

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 v2 0/3] refactor the cmd_exec()
  2019-06-18 14:49 Matteo Croce
  2019-06-18 15:41 ` Matteo Croce
@ 2019-06-20 21:31 ` Stephen Hemminger
  1 sibling, 0 replies; 12+ messages in thread
From: Stephen Hemminger @ 2019-06-20 21:31 UTC (permalink / raw)
  To: Matteo Croce; +Cc: netdev, David Ahern, Andrea Claudi

On Tue, 18 Jun 2019 16:49:32 +0200
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 shows a tiny saving.
> 
> 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        | 61 ++++++++++++++++++++++++++++++++++-----------
>  ip/ipvrf.c          | 12 ++++++---
>  lib/exec.c          |  7 +++++-
>  lib/namespace.c     | 31 -----------------------
>  lib/utils.c         | 27 --------------------
>  8 files changed, 63 insertions(+), 84 deletions(-)
> 

Ok, applied.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 v2 0/3] refactor the cmd_exec()
  2019-06-18 14:49 Matteo Croce
@ 2019-06-18 15:41 ` Matteo Croce
  2019-06-20 21:31 ` Stephen Hemminger
  1 sibling, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-06-18 15:41 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, David Ahern, Andrea Claudi

On Tue, Jun 18, 2019 at 4:49 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 shows a tiny saving.
>
> 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        | 61 ++++++++++++++++++++++++++++++++++-----------
>  ip/ipvrf.c          | 12 ++++++---
>  lib/exec.c          |  7 +++++-
>  lib/namespace.c     | 31 -----------------------
>  lib/utils.c         | 27 --------------------
>  8 files changed, 63 insertions(+), 84 deletions(-)
>
> --
> 2.21.0
>

Hi all,

this should really be the v3, I did an off-by-one.

Sorry,
-- 
Matteo Croce
per aspera ad upstream

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH iproute2 v2 0/3] refactor the cmd_exec()
@ 2019-06-18 14:49 Matteo Croce
  2019-06-18 15:41 ` Matteo Croce
  2019-06-20 21:31 ` Stephen Hemminger
  0 siblings, 2 replies; 12+ messages in thread
From: Matteo Croce @ 2019-06-18 14:49 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, David Ahern, Andrea Claudi

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 shows a tiny saving.

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        | 61 ++++++++++++++++++++++++++++++++++-----------
 ip/ipvrf.c          | 12 ++++++---
 lib/exec.c          |  7 +++++-
 lib/namespace.c     | 31 -----------------------
 lib/utils.c         | 27 --------------------
 8 files changed, 63 insertions(+), 84 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-06-20 21:31 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-11 16:10 [PATCH iproute2 v2 0/3] refactor the cmd_exec() Matteo Croce
2019-06-11 16:10 ` [PATCH iproute2 v2 1/3] netns: switch netns in the child when executing commands Matteo Croce
2019-06-11 16:10 ` [PATCH iproute2 v2 2/3] ip vrf: use hook to change VRF in the child Matteo Croce
2019-06-11 16:10 ` [PATCH iproute2 v2 3/3] netns: make netns_{save,restore} static Matteo Croce
2019-06-13 17:07 ` [PATCH iproute2 v2 0/3] refactor the cmd_exec() Andrea Claudi
2019-06-14 14:35 ` David Ahern
2019-06-15 14:06   ` Matteo Croce
2019-06-17 22:20     ` Matteo Croce
2019-06-17 22:24       ` David Ahern
2019-06-18 14:49 Matteo Croce
2019-06-18 15:41 ` Matteo Croce
2019-06-20 21:31 ` Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).