[PATCH iproute2 0/2] refactor the 'ip netns exec' command

[PATCH iproute2 0/2] refactor the 'ip netns exec' command

[Matteo Croce]

Refactor the netns code so less steps are needed to exec commands in a netns.
Also remove some code which became dead. bloat-o-meter output:

$ bloat-o-meter ip.old ip
add/remove: 0/5 grow/shrink: 3/3 up/down: 159/-358 (-199)
Function                                     old     new   delta
netns_add                                    971    1058     +87
cmd_exec                                     207     254     +47
on_netns_exec                                 32      57     +25
netns_restore                                 69      67      -2
netns_switch                                 838     822     -16
on_netns_label                                45       -     -45
do_netns                                    1226    1180     -46
vrf_reset                                     55       -     -55
do_each_netns                                 57       -     -57
on_netns                                      60       -     -60
netns_save                                    77       -     -77
Total: Before=667505, After=667306, chg -0.03%

Matteo Croce (2):
  netns: switch netns in the child when executing commands
  netns: make netns_{save,restore} static

 include/namespace.h |  2 --
 include/utils.h     |  5 +----
 ip/ip.c             |  1 -
 ip/ip_common.h      |  1 -
 ip/ipnetns.c        | 49 ++++++++++++++++++++++++++++++++-------------
 ip/ipvrf.c          | 16 +--------------
 lib/exec.c          |  6 +++++-
 lib/namespace.c     | 31 ----------------------------
 lib/utils.c         | 27 -------------------------
 9 files changed, 42 insertions(+), 96 deletions(-)

-- 
2.21.0

[PATCH iproute2 1/2] netns: switch netns in the child when executing commands

[Matteo Croce]

'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, as well as other cleanup things like VRF associations.
Add an argument to cmd_exec() which allows to switch the current netns directly
in the child, so the parent environment is kept unaltered.
By doing so, some utility functions became unused, so remove them.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 include/utils.h |  5 +----
 ip/ip_common.h  |  1 -
 ip/ipnetns.c    | 18 ++++--------------
 ip/ipvrf.c      | 16 +---------------
 lib/exec.c      |  6 +++++-
 lib/utils.c     | 27 ---------------------------
 6 files changed, 11 insertions(+), 62 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 8a9c3020..c58a3886 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -294,14 +294,11 @@ 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, char *netns);
 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/ip_common.h b/ip/ip_common.h
index b4aa34a7..38203aae 100644
--- a/ip/ip_common.h
+++ b/ip/ip_common.h
@@ -77,7 +77,6 @@ int do_tcp_metrics(int argc, char **argv);
 int do_ipnetconf(int argc, char **argv);
 int do_iptoken(int argc, char **argv);
 int do_ipvrf(int argc, char **argv);
-void vrf_reset(void);
 int netns_identify_pid(const char *pidstr, char *name, int len);
 int do_seg6(int argc, char **argv);
 
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 8ead0c4c..9e414b55 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -400,7 +400,8 @@ 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, nsname);
 	return 0;
 }
 
@@ -409,8 +410,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 +420,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, argv[0]);
 }
 
 static int is_pid(const char *str)
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index a13cf653..894d85fc 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -456,21 +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);
-}
-
-/* reset VRF association of current process to default VRF;
- * used by netns_exec
- */
-void vrf_reset(void)
-{
-	char vrf[32];
-
-	if (vrf_identify(getpid(), vrf, sizeof(vrf)) ||
-	    (vrf[0] == '\0'))
-		return;
-
-	vrf_switch("default");
+	return -cmd_exec(argv[1], argv + 1, !!batch_mode, NULL);
 }
 
 static int ipvrf_filter_req(struct nlmsghdr *nlh, int reqlen)
diff --git a/lib/exec.c b/lib/exec.c
index eb36b59d..3b07e908 100644
--- a/lib/exec.c
+++ b/lib/exec.c
@@ -5,8 +5,9 @@
 #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, char *netns)
 {
 	fflush(stdout);
 	if (do_fork) {
@@ -34,6 +35,9 @@ int cmd_exec(const char *cmd, char **argv, bool do_fork)
 		}
 	}
 
+	if (netns && netns_switch(netns))
+		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

[PATCH iproute2 2/2] netns: make netns_{save,restore} static

[Matteo Croce]

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 9e414b55..beb7fb8f 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)
@@ -623,6 +624,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
@@ -716,9 +744,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

Re: [PATCH iproute2 1/2] netns: switch netns in the child when executing commands

[Stephen Hemminger]

On Tue, 11 Jun 2019 00:16:12 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> +	printf("\nnetns: %s\n", nsname);
> +	cmd_exec(argv[0], argv, true, nsname);
>  	return 0;

simple printf breaks JSON output.

Re: [PATCH iproute2 1/2] netns: switch netns in the child when executing commands

[Matteo Croce]

On Tue, Jun 11, 2019 at 12:46 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 11 Jun 2019 00:16:12 +0200
> Matteo Croce <mcroce@redhat.com> wrote:
>
> > +     printf("\nnetns: %s\n", nsname);
> > +     cmd_exec(argv[0], argv, true, nsname);
> >       return 0;
>
> simple printf breaks JSON output.

It was just moved from on_netns_label(). I will check how the json
output works when running doall and provide a similar behaviour.

Anyway, I noticed that the VRF env should be reset but only in the
child. I'm adding a function pointer to cmd_exec which will
point to an hook which changes the netns when doing 'ip netns exec'
and reset the VRF on vrf exec.

Regards,
-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH iproute2 1/2] netns: switch netns in the child when executing commands

[Matteo Croce]

On Tue, Jun 11, 2019 at 12:52 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Tue, Jun 11, 2019 at 12:46 AM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Tue, 11 Jun 2019 00:16:12 +0200
> > Matteo Croce <mcroce@redhat.com> wrote:
> >
> > > +     printf("\nnetns: %s\n", nsname);
> > > +     cmd_exec(argv[0], argv, true, nsname);
> > >       return 0;
> >
> > simple printf breaks JSON output.
>
> It was just moved from on_netns_label(). I will check how the json
> output works when running doall and provide a similar behaviour.
>
> Anyway, I noticed that the VRF env should be reset but only in the
> child. I'm adding a function pointer to cmd_exec which will
> point to an hook which changes the netns when doing 'ip netns exec'
> and reset the VRF on vrf exec.
>
> Regards,
> --
> Matteo Croce
> per aspera ad upstream

Hi Stephen,

just checked, but it seems that netns exec in batch mode produces an
invalid output anyway:

# ip netns add n1
# ip netns add n2
# ip -all -json netns exec date

netns: n2
Tue 11 Jun 2019 12:55:11 AM CEST

netns: n1
Tue 11 Jun 2019 12:55:11 AM CEST

Probably there is very little sense in using -all netns exec and json
together, but worth noting it.

Bye,
-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH iproute2 1/2] netns: switch netns in the child when executing commands

[Stephen Hemminger]

On Tue, 11 Jun 2019 01:03:57 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> On Tue, Jun 11, 2019 at 12:52 AM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > On Tue, Jun 11, 2019 at 12:46 AM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:  
> > >
> > > On Tue, 11 Jun 2019 00:16:12 +0200
> > > Matteo Croce <mcroce@redhat.com> wrote:
> > >  
> > > > +     printf("\nnetns: %s\n", nsname);
> > > > +     cmd_exec(argv[0], argv, true, nsname);
> > > >       return 0;  
> > >
> > > simple printf breaks JSON output.  
> >
> > It was just moved from on_netns_label(). I will check how the json
> > output works when running doall and provide a similar behaviour.
> >
> > Anyway, I noticed that the VRF env should be reset but only in the
> > child. I'm adding a function pointer to cmd_exec which will
> > point to an hook which changes the netns when doing 'ip netns exec'
> > and reset the VRF on vrf exec.
> >
> > Regards,
> > --
> > Matteo Croce
> > per aspera ad upstream  
> 
> Hi Stephen,
> 
> just checked, but it seems that netns exec in batch mode produces an
> invalid output anyway:
> 
> # ip netns add n1
> # ip netns add n2
> # ip -all -json netns exec date
> 
> netns: n2
> Tue 11 Jun 2019 12:55:11 AM CEST
> 
> netns: n1
> Tue 11 Jun 2019 12:55:11 AM CEST
> 
> Probably there is very little sense in using -all netns exec and json
> together, but worth noting it.
> 
> Bye,

Thanks, just being paranoid about json output.