[PATCH iproute2] ip: Simplify executing ip cmd within namespace

[PATCH iproute2] ip: Simplify executing ip cmd within namespace

[Vadim Kochan]

From: Vadim Kochan <vadim4j@gmail.com>

Added new '-ns' option to simplify executing following cmd:

    ip netns exec NETNS ip OPTIONS COMMAND OBJECT

    to

    ip -ns NETNS OPTIONS COMMAND OBJECT

e.g.:

    ip -ns vnet0 link add br0 type bridge

Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
May be new option should have better name than '-ns' ?

 ip/ip.c        |  6 ++++++
 ip/ip_common.h |  1 +
 ip/ipnetns.c   |  2 +-
 man/man8/ip.8  | 21 +++++++++++++++++++++
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/ip/ip.c b/ip/ip.c
index 5f759d5..35cf463 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -262,6 +262,12 @@ int main(int argc, char **argv)
 			rcvbuf = size;
 		} else if (matches(opt, "-help") == 0) {
 			usage();
+		} else if (matches(opt, "-ns") == 0) {
+			argc--;
+			argv++;
+			argv[0] = argv[1];
+			argv[1] = basename;
+			return netns_exec(argc, argv);
 		} else {
 			fprintf(stderr, "Option \"%s\" is unknown, try \"ip -help\".\n", opt);
 			exit(-1);
diff --git a/ip/ip_common.h b/ip/ip_common.h
index 75bfb82..d4f7e1f 100644
--- a/ip/ip_common.h
+++ b/ip/ip_common.h
@@ -88,6 +88,7 @@ struct link_util
 struct link_util *get_link_kind(const char *kind);
 struct link_util *get_link_slave_kind(const char *slave_kind);
 int get_netns_fd(const char *name);
+int netns_exec(int argc, char **argv);
 
 #ifndef	INFINITY_LIFE_TIME
 #define     INFINITY_LIFE_TIME      0xFFFFFFFFU
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 1c8aa02..367841c 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -129,7 +129,7 @@ static void bind_etc(const char *name)
 	closedir(dir);
 }
 
-static int netns_exec(int argc, char **argv)
+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.
diff --git a/man/man8/ip.8 b/man/man8/ip.8
index 2d42e98..bfb0c53 100644
--- a/man/man8/ip.8
+++ b/man/man8/ip.8
@@ -134,6 +134,27 @@ the output.
 use the system's name resolver to print DNS names instead of
 host addresses.
 
+.TP
+.BR "\-ns " <NETNS>
+executes the following
+.RI "[ " OPTIONS " ] " OBJECT " { " COMMAND " | "
+.BR help " }"
+in the specified network namespace
+.IR NETNS .
+Actually it just simplifies executing of:
+
+.B ip netns exec
+.IR NETNS
+.B ip
+.RI "[ " OPTIONS " ] " OBJECT " { " COMMAND " | "
+.BR help " }"
+
+to
+
+.B ip
+.RI "-ns " NETNS " [ " OPTIONS " ] " OBJECT " { " COMMAND " | "
+.BR help " }"
+
 .SH IP - COMMAND SYNTAX
 
 .SS
-- 
2.1.3

Re: [PATCH iproute2] ip: Simplify executing ip cmd within namespace

[Nicolas Dichtel]

Le 10/12/2014 23:56, Vadim Kochan a écrit :
> From: Vadim Kochan <vadim4j@gmail.com>
>
> Added new '-ns' option to simplify executing following cmd:
>
>      ip netns exec NETNS ip OPTIONS COMMAND OBJECT
>
>      to
>
>      ip -ns NETNS OPTIONS COMMAND OBJECT
>
> e.g.:
>
>      ip -ns vnet0 link add br0 type bridge
>
> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
> ---
> May be new option should have better name than '-ns' ?
What about 'ip -netns' to be explicit like other options?
user may still use 'ip -n' at the end.


Regards,
Nicolas

Re: [PATCH iproute2] ip: Simplify executing ip cmd within namespace

[vadim4j]

On Thu, Dec 11, 2014 at 11:58:21AM +0100, Nicolas Dichtel wrote:
> Le 10/12/2014 23:56, Vadim Kochan a écrit :
> >From: Vadim Kochan <vadim4j@gmail.com>
> >
> >Added new '-ns' option to simplify executing following cmd:
> >
> >     ip netns exec NETNS ip OPTIONS COMMAND OBJECT
> >
> >     to
> >
> >     ip -ns NETNS OPTIONS COMMAND OBJECT
> >
> >e.g.:
> >
> >     ip -ns vnet0 link add br0 type bridge
> >
> >Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
> >---
> >May be new option should have better name than '-ns' ?
> What about 'ip -netns' to be explicit like other options?
> user may still use 'ip -n' at the end.
> 
> 
> Regards,
> Nicolas

May be left '-n' for some other future option, but use the following
options: -net[ns] and -ns ? What do you think ?

Thanks,

Re: [PATCH iproute2] ip: Simplify executing ip cmd within namespace

[Nicolas Dichtel]

Le 11/12/2014 11:57, vadim4j@gmail.com a écrit :
> On Thu, Dec 11, 2014 at 11:58:21AM +0100, Nicolas Dichtel wrote:
>> Le 10/12/2014 23:56, Vadim Kochan a écrit :
>>> From: Vadim Kochan <vadim4j@gmail.com>
>>>
>>> Added new '-ns' option to simplify executing following cmd:
>>>
>>>      ip netns exec NETNS ip OPTIONS COMMAND OBJECT
>>>
>>>      to
>>>
>>>      ip -ns NETNS OPTIONS COMMAND OBJECT
>>>
>>> e.g.:
>>>
>>>      ip -ns vnet0 link add br0 type bridge
>>>
>>> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
>>> ---
>>> May be new option should have better name than '-ns' ?
>> What about 'ip -netns' to be explicit like other options?
>> user may still use 'ip -n' at the end.
>>
>>
>> Regards,
>> Nicolas
>
> May be left '-n' for some other future option, but use the following
Options parsing in iproute2 will match -netns when typing -n because there
is no other options that begin with a 'n' (I've done a quick look, maybe I've
missed one).
Like -d which matches -details, etc.

> options: -net[ns] and -ns ? What do you think ?
One option is enough. '-netns' is an explicit reference to 'ip netns'.


Regards,
Nicolas

Re: [PATCH iproute2] ip: Simplify executing ip cmd within namespace

[vadim4j]

On Thu, Dec 11, 2014 at 01:50:17PM +0100, Nicolas Dichtel wrote:
> Le 11/12/2014 11:57, vadim4j@gmail.com a écrit :
> >On Thu, Dec 11, 2014 at 11:58:21AM +0100, Nicolas Dichtel wrote:
> >>Le 10/12/2014 23:56, Vadim Kochan a écrit :
> >>>From: Vadim Kochan <vadim4j@gmail.com>
> >>>
> >>>Added new '-ns' option to simplify executing following cmd:
> >>>
> >>>     ip netns exec NETNS ip OPTIONS COMMAND OBJECT
> >>>
> >>>     to
> >>>
> >>>     ip -ns NETNS OPTIONS COMMAND OBJECT
> >>>
> >>>e.g.:
> >>>
> >>>     ip -ns vnet0 link add br0 type bridge
> >>>
> >>>Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
> >>>---
> >>>May be new option should have better name than '-ns' ?
> >>What about 'ip -netns' to be explicit like other options?
> >>user may still use 'ip -n' at the end.
> >>
> >>
> >>Regards,
> >>Nicolas
> >
> >May be left '-n' for some other future option, but use the following
> Options parsing in iproute2 will match -netns when typing -n because there
> is no other options that begin with a 'n' (I've done a quick look, maybe I've
> missed one).
> Like -d which matches -details, etc.
> 
> >options: -net[ns] and -ns ? What do you think ?
> One option is enough. '-netns' is an explicit reference to 'ip netns'.
> 
> 
> Regards,
> Nicolas

OK, I agree.
I will re-work and resend v2.

Thanks,

Re: [PATCH iproute2] ip: Simplify executing ip cmd within namespace

[Jiri Benc]

On Thu, 11 Dec 2014 00:56:35 +0200, Vadim Kochan wrote:
> From: Vadim Kochan <vadim4j@gmail.com>
> 
> Added new '-ns' option to simplify executing following cmd:
> 
>     ip netns exec NETNS ip OPTIONS COMMAND OBJECT
> 
>     to
> 
>     ip -ns NETNS OPTIONS COMMAND OBJECT
> 
> e.g.:
> 
>     ip -ns vnet0 link add br0 type bridge

This is great! It's a thing that has been bothering me for long time
but never got high enough on my todo list. Thanks for working on this.

However,

> --- a/ip/ip.c
> +++ b/ip/ip.c
> @@ -262,6 +262,12 @@ int main(int argc, char **argv)
>  			rcvbuf = size;
>  		} else if (matches(opt, "-help") == 0) {
>  			usage();
> +		} else if (matches(opt, "-ns") == 0) {
> +			argc--;
> +			argv++;
> +			argv[0] = argv[1];
> +			argv[1] = basename;
> +			return netns_exec(argc, argv);

I very much dislike this. There's no reason to exec another ip binary.
The main reason I wanted the -n (or whatever) option was to speed up
execution of test scripts in environments with hundreds of interfaces
in different net namespaces.

Please just change to the specified netns and continue with interpreting
of the rest of the command line, there's absolutely no reason for doing
the exec.

Thanks,

 Jiri

-- 
Jiri Benc

Re: [PATCH iproute2] ip: Simplify executing ip cmd within namespace

[vadim4j]

On Thu, Dec 11, 2014 at 05:09:28PM +0100, Jiri Benc wrote:
> On Thu, 11 Dec 2014 00:56:35 +0200, Vadim Kochan wrote:
> > From: Vadim Kochan <vadim4j@gmail.com>
> > 
> > Added new '-ns' option to simplify executing following cmd:
> > 
> >     ip netns exec NETNS ip OPTIONS COMMAND OBJECT
> > 
> >     to
> > 
> >     ip -ns NETNS OPTIONS COMMAND OBJECT
> > 
> > e.g.:
> > 
> >     ip -ns vnet0 link add br0 type bridge
> 
> This is great! It's a thing that has been bothering me for long time
> but never got high enough on my todo list. Thanks for working on this.
> 
> However,
> 
> > --- a/ip/ip.c
> > +++ b/ip/ip.c
> > @@ -262,6 +262,12 @@ int main(int argc, char **argv)
> >  			rcvbuf = size;
> >  		} else if (matches(opt, "-help") == 0) {
> >  			usage();
> > +		} else if (matches(opt, "-ns") == 0) {
> > +			argc--;
> > +			argv++;
> > +			argv[0] = argv[1];
> > +			argv[1] = basename;
> > +			return netns_exec(argc, argv);
> 
> I very much dislike this. There's no reason to exec another ip binary.
> The main reason I wanted the -n (or whatever) option was to speed up
> execution of test scripts in environments with hundreds of interfaces
> in different net namespaces.
> 
> Please just change to the specified netns and continue with interpreting
> of the rest of the command line, there's absolutely no reason for doing
> the exec.
Yes, I will follow that way.

Thanks,
> 
> Thanks,
> 
>  Jiri
> 
> -- 
> Jiri Benc

Re: [PATCH iproute2] ip: Simplify executing ip cmd within namespace

[Marcelo Ricardo Leitner]

On 11-12-2014 14:33, vadim4j@gmail.com wrote:
> On Thu, Dec 11, 2014 at 05:09:28PM +0100, Jiri Benc wrote:
>> On Thu, 11 Dec 2014 00:56:35 +0200, Vadim Kochan wrote:
>>> From: Vadim Kochan <vadim4j@gmail.com>
>>>
>>> Added new '-ns' option to simplify executing following cmd:
>>>
>>>      ip netns exec NETNS ip OPTIONS COMMAND OBJECT
>>>
>>>      to
>>>
>>>      ip -ns NETNS OPTIONS COMMAND OBJECT
>>>
>>> e.g.:
>>>
>>>      ip -ns vnet0 link add br0 type bridge
>>
>> This is great! It's a thing that has been bothering me for long time
>> but never got high enough on my todo list. Thanks for working on this.
>>
>> However,
>>
>>> --- a/ip/ip.c
>>> +++ b/ip/ip.c
>>> @@ -262,6 +262,12 @@ int main(int argc, char **argv)
>>>   			rcvbuf = size;
>>>   		} else if (matches(opt, "-help") == 0) {
>>>   			usage();
>>> +		} else if (matches(opt, "-ns") == 0) {
>>> +			argc--;
>>> +			argv++;
>>> +			argv[0] = argv[1];
>>> +			argv[1] = basename;
>>> +			return netns_exec(argc, argv);
>>
>> I very much dislike this. There's no reason to exec another ip binary.
>> The main reason I wanted the -n (or whatever) option was to speed up
>> execution of test scripts in environments with hundreds of interfaces
>> in different net namespaces.
>>
>> Please just change to the specified netns and continue with interpreting
>> of the rest of the command line, there's absolutely no reason for doing
>> the exec.
> Yes, I will follow that way.

In that case, it would be interesting to also accelerate the original use 
case, no? So all usages we currently have will benefit from this speed up 
without a change.

if (command to be executed == myself)
   switch namespace, continue without fork/exec..

I'm not sure if this is feasible, though. Just sharing the idea, didn't even 
open the code..

   Marcelo

Re: [PATCH iproute2] ip: Simplify executing ip cmd within namespace

[Jiri Benc]

On Thu, 11 Dec 2014 15:33:34 -0200, Marcelo Ricardo Leitner wrote:
> In that case, it would be interesting to also accelerate the original use 
> case, no? So all usages we currently have will benefit from this speed up 
> without a change.
> 
> if (command to be executed == myself)
>    switch namespace, continue without fork/exec..

It's never good idea to do such tricks behind the user's back. This
particular case could easily break for users wanting to execute a
different ip binary (for whatever reason).

All programs should do what they are told to do, not try to outsmart
the user.

 Jiri

-- 
Jiri Benc

Re: [PATCH iproute2] ip: Simplify executing ip cmd within namespace

[Marcelo Ricardo Leitner]

On 11-12-2014 16:08, Jiri Benc wrote:
> On Thu, 11 Dec 2014 15:33:34 -0200, Marcelo Ricardo Leitner wrote:
>> In that case, it would be interesting to also accelerate the original use
>> case, no? So all usages we currently have will benefit from this speed up
>> without a change.
>>
>> if (command to be executed == myself)
>>     switch namespace, continue without fork/exec..
>
> It's never good idea to do such tricks behind the user's back. This
> particular case could easily break for users wanting to execute a
> different ip binary (for whatever reason).

Then the if() above wouldn't match. That if means to check 
/proc/self/exe against the result of the path expansion. If that fails, 
continue with the normal path. If it matches, it is the same binary, and 
no need to re-exec itself.

> All programs should do what they are told to do, not try to outsmart
> the user.

It's not outsmarting, it's just not being dumb and doing it the proper 
way. Bash itself does this twist a lot. If you just type 'echo hi', it 
won't execute /bin/echo but use a built-in version. But if you write 
"/bin/echo hi", it will use /bin/echo..

We could use the same idea. "ip netns exec ip" -> ellipse it and avoid 
the fork/exec. But if it's cmd != "ip", execute it..

Now consider other applications that are user of this command. They will 
have to implement something like:

if (this ip command has --netns argument) {
    cmd="ip --netns ..."
} else {
    cmd="ip netns exec ..."
}

which is ugly and inconvenient.

   Marcelo