| Message ID | 20190610221613.7554-2-mcroce@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | refactor the 'ip netns exec' command | expand |
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.
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,
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,
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.
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);
'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(-)