diff mbox

[iproute2,v4] ipbatch: fix use of 'ip netns exec'

Message ID 1373385312-4899-1-git-send-email-nicolas.dichtel@6wind.com
State Changes Requested, archived
Delegated to: stephen hemminger
Headers show

Commit Message

Nicolas Dichtel July 9, 2013, 3:55 p.m. UTC 
From: JunweiZhang <junwei.zhang@6wind.com>

execvp() does not return when the command succeed, hence all commands in the
batch file after the line 'ip netns exec' are not executed.

Let's fork before calling execvp() if batch mode is used..

Example:
$ cat test.batch
netns add netns1
netns exec netns1 ip l
netns
$ ip -b test.batch
1: lo: <LOOPBACK> mtu 65536 qdisc noop state DOWN mode DEFAULT
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: sit0: <NOARP> mtu 1480 qdisc noop state DOWN mode DEFAULT
    link/sit 0.0.0.0 brd 0.0.0.0

All command after 'netns exec' are never executed.

With the patch:
$ ip -b test.batch
1: lo: <LOOPBACK> mtu 65536 qdisc noop state DOWN mode DEFAULT
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: sit0: <NOARP> mtu 1480 qdisc noop state DOWN mode DEFAULT
    link/sit 0.0.0.0 brd 0.0.0.0
netns1

Now, existing netns are displayed.

Signed-off-by: JunweiZhang <junwei.zhang@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v4: use pid_t type for pid
    use _exit() instead of exit()
    fork only if batch is used

v3: be more verbose on error (fork(), waitpid())
    when child fails, just exit to avoid double cleanning
    check WIFEXITED() and waitpid() before using WEXITSTATUS()

v2: keep exit status of the child
    add an example in the commit log

 ip/ipnetns.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Ben Hutchings July 9, 2013, 4:19 p.m. UTC | #1 
On Tue, 2013-07-09 at 17:55 +0200, Nicolas Dichtel wrote:
[...]
> diff --git a/ip/ipnetns.c b/ip/ipnetns.c
> index fa2b681..cdc3101 100644
> --- a/ip/ipnetns.c
> +++ b/ip/ipnetns.c
> @@ -42,6 +42,7 @@
>  #define MS_SHARED	(1 << 20)
>  #endif
>  
> +extern char *batch_file;

Should be declared in a header file.

>  #ifndef HAVE_SETNS
>  static int setns(int fd, int nstype)
> @@ -185,6 +186,42 @@ static int netns_exec(int argc, char **argv)
>  	/* Setup bind mounts for config files in /etc */
>  	bind_etc(name);
>  
> +	if (batch_file) {
> +		int status;
> +		pid_t pid;
> +
> +		pid = fork();
> +		if (pid < 0) {
> +			perror("fork");
> +			return EXIT_FAILURE;

return -1;

> +		}
> +
> +		if (pid == 0) {
> +			/* Child */
> +			if (execvp(cmd, argv + 1)  < 0)
> +				fprintf(stderr, "exec of \"%s\" failed: %s\n",
> +					cmd, strerror(errno));
> +
> +			_exit(EXIT_FAILURE);
> +		}
> +
> +		/* Parent */
> +		if (waitpid(pid, &status, 0) < 0) {
> +			perror("waitpid");
> +			return EXIT_FAILURE;

return -1;

> +		}
> +
> +		if (WIFEXITED(status)) {
> +			/* ip must returns the status of the child, but do_cmd()
> +			 * will add a minus to this returned value, so let's add
> +			 * another one here to cancel it.
> +			 */
> +			return -WEXITSTATUS(status);
> +		}
> +
> +		return EXIT_FAILURE;

return -1;

> +	}
> +
>  	if (execvp(cmd, argv + 1)  < 0)
>  		fprintf(stderr, "exec of \"%s\" failed: %s\n",
>  			cmd, strerror(errno));
Nicolas Dichtel July 9, 2013, 4:59 p.m. UTC | #2 
Le 09/07/2013 18:19, Ben Hutchings a écrit :
> On Tue, 2013-07-09 at 17:55 +0200, Nicolas Dichtel wrote:
> [...]
>> diff --git a/ip/ipnetns.c b/ip/ipnetns.c
>> index fa2b681..cdc3101 100644
>> --- a/ip/ipnetns.c
>> +++ b/ip/ipnetns.c
>> @@ -42,6 +42,7 @@
>>   #define MS_SHARED	(1 << 20)
>>   #endif
>>
>> +extern char *batch_file;
I let Stephen choose, he suggest me this.

>
> Should be declared in a header file.
>
>>   #ifndef HAVE_SETNS
>>   static int setns(int fd, int nstype)
>> @@ -185,6 +186,42 @@ static int netns_exec(int argc, char **argv)
>>   	/* Setup bind mounts for config files in /etc */
>>   	bind_etc(name);
>>
>> +	if (batch_file) {
>> +		int status;
>> +		pid_t pid;
>> +
>> +		pid = fork();
>> +		if (pid < 0) {
>> +			perror("fork");
>> +			return EXIT_FAILURE;
>
> return -1;
man exit says:
The use of EXIT_SUCCESS and EXIT_FAILURE is slightly more portable (to non-UNIX 
environments) than the use of 0 and some nonzero value like 1 or -1.

And in fact, it was so before my patch.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings July 9, 2013, 6:13 p.m. UTC | #3 
On Tue, 2013-07-09 at 18:59 +0200, Nicolas Dichtel wrote:
> Le 09/07/2013 18:19, Ben Hutchings a écrit :
> > On Tue, 2013-07-09 at 17:55 +0200, Nicolas Dichtel wrote:
> > [...]
> >> diff --git a/ip/ipnetns.c b/ip/ipnetns.c
> >> index fa2b681..cdc3101 100644
> >> --- a/ip/ipnetns.c
> >> +++ b/ip/ipnetns.c
> >> @@ -42,6 +42,7 @@
> >>   #define MS_SHARED	(1 << 20)
> >>   #endif
> >>
> >> +extern char *batch_file;
> I let Stephen choose, he suggest me this.
> 
> >
> > Should be declared in a header file.
> >
> >>   #ifndef HAVE_SETNS
> >>   static int setns(int fd, int nstype)
> >> @@ -185,6 +186,42 @@ static int netns_exec(int argc, char **argv)
> >>   	/* Setup bind mounts for config files in /etc */
> >>   	bind_etc(name);
> >>
> >> +	if (batch_file) {
> >> +		int status;
> >> +		pid_t pid;
> >> +
> >> +		pid = fork();
> >> +		if (pid < 0) {
> >> +			perror("fork");
> >> +			return EXIT_FAILURE;
> >
> > return -1;
> man exit says:
> The use of EXIT_SUCCESS and EXIT_FAILURE is slightly more portable (to non-UNIX 
> environments) than the use of 0 and some nonzero value like 1 or -1.
> 
> And in fact, it was so before my patch.

As your own earlier comment says, do_cmd() negates the return value.

Ben.
Nicolas Dichtel July 10, 2013, 9:52 a.m. UTC | #4 
Le 09/07/2013 20:13, Ben Hutchings a écrit :
> On Tue, 2013-07-09 at 18:59 +0200, Nicolas Dichtel wrote:
>> Le 09/07/2013 18:19, Ben Hutchings a écrit :
>>> On Tue, 2013-07-09 at 17:55 +0200, Nicolas Dichtel wrote:
>>> [...]
>>>> diff --git a/ip/ipnetns.c b/ip/ipnetns.c
>>>> index fa2b681..cdc3101 100644
>>>> --- a/ip/ipnetns.c
>>>> +++ b/ip/ipnetns.c
>>>> @@ -42,6 +42,7 @@
>>>>    #define MS_SHARED	(1 << 20)
>>>>    #endif
>>>>
>>>> +extern char *batch_file;
>> I let Stephen choose, he suggest me this.
>>
>>>
>>> Should be declared in a header file.
>>>
>>>>    #ifndef HAVE_SETNS
>>>>    static int setns(int fd, int nstype)
>>>> @@ -185,6 +186,42 @@ static int netns_exec(int argc, char **argv)
>>>>    	/* Setup bind mounts for config files in /etc */
>>>>    	bind_etc(name);
>>>>
>>>> +	if (batch_file) {
>>>> +		int status;
>>>> +		pid_t pid;
>>>> +
>>>> +		pid = fork();
>>>> +		if (pid < 0) {
>>>> +			perror("fork");
>>>> +			return EXIT_FAILURE;
>>>
>>> return -1;
>> man exit says:
>> The use of EXIT_SUCCESS and EXIT_FAILURE is slightly more portable (to non-UNIX
>> environments) than the use of 0 and some nonzero value like 1 or -1.
>>
>> And in fact, it was so before my patch.
>
> As your own earlier comment says, do_cmd() negates the return value.
Yes, but EXIT_FAILURE is used everywhere in ip/ipnetns.c.

Stephen, should I send a patch to replace use of EXIT_FAILURE by -1 in ipnetns.c?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index fa2b681..cdc3101 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -42,6 +42,7 @@ 
 #define MS_SHARED	(1 << 20)
 #endif
 
+extern char *batch_file;
 
 #ifndef HAVE_SETNS
 static int setns(int fd, int nstype)
@@ -185,6 +186,42 @@  static int netns_exec(int argc, char **argv)
 	/* Setup bind mounts for config files in /etc */
 	bind_etc(name);
 
+	if (batch_file) {
+		int status;
+		pid_t pid;
+
+		pid = fork();
+		if (pid < 0) {
+			perror("fork");
+			return EXIT_FAILURE;
+		}
+
+		if (pid == 0) {
+			/* Child */
+			if (execvp(cmd, argv + 1)  < 0)
+				fprintf(stderr, "exec of \"%s\" failed: %s\n",
+					cmd, strerror(errno));
+
+			_exit(EXIT_FAILURE);
+		}
+
+		/* Parent */
+		if (waitpid(pid, &status, 0) < 0) {
+			perror("waitpid");
+			return EXIT_FAILURE;
+		}
+
+		if (WIFEXITED(status)) {
+			/* ip must returns the status of the child, but do_cmd()
+			 * will add a minus to this returned value, so let's add
+			 * another one here to cancel it.
+			 */
+			return -WEXITSTATUS(status);
+		}
+
+		return EXIT_FAILURE;
+	}
+
 	if (execvp(cmd, argv + 1)  < 0)
 		fprintf(stderr, "exec of \"%s\" failed: %s\n",
 			cmd, strerror(errno));