Skip to contentSkip to conversation

return, don't exist, at coord error

PATCH In review Serge E. Hallyn 3 participants 4 messages Oct 19, 2009 3 messages not in the archive

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 return, don't exist, at coord error

All right I can't explain it at the moment, but exit(1) at ckpt_coordinator() (on error) causes restart.c to exit with 0, whereas return(ret) causes it to correctly exit with -1.

Without this, a restart whose kernel portion ends with say -1 ends up returning from user-cr/restart.c with 0, so userspace can't tell whether sys_restart succeeded or failed. With the patch, it fails correctly.

---
 restart.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
restart.c +1 -1
index e6e72ac..87899ba 100644
@@ -933,7 +933,7 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
 		perror("restart failed");
 		ckpt_verbose("Failed\n");
 		ckpt_dbg("restart failed ?\n");
-		exit(1);
+		return ret;
 	}
 
 	ckpt_verbose("Success\n");
Signature · 1 line
1.6.1.1

A message that is not in the archive (sent off-list or not yet received): 20091019022942.GA21306-r/[email protected]

#2 lore raw

What was the command line you used for the restart ?

Oren.

Quoted 27 lines · Serge E. Hallyn

All right I can't explain it at the moment, but exit(1) at ckpt_coordinator() (on error) causes restart.c to exit with 0, whereas return(ret) causes it to correctly exit with -1.

Without this, a restart whose kernel portion ends with say -1 ends up returning from user-cr/restart.c with 0, so userspace can't tell whether sys_restart succeeded or failed. With the patch, it fails correctly.

---
 restart.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
restart.c +1 -1
index e6e72ac..87899ba 100644
@@ -933,7 +933,7 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
 		perror("restart failed");
 		ckpt_verbose("Failed\n");
 		ckpt_dbg("restart failed ?\n");
-		exit(1);
+		return ret;
 	}
 
 	ckpt_verbose("Success\n");

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw

Hmm, well... I dont' know what's going on with my userspace, Now it's not working for me even with this patch.

As root I do:

setcap cap_sys_admin+pe /bin/restart
ckpt > /tmp/out

then as a non-rootuid I do

	cat /tmp/out | /bin/retart -vd
	echo $?

giving me:
[hallyn@linuz11 ~]$ /bin/restart -vd < /tmp/out
<4028>number of tasks: 1
<4028>total tasks (including ghosts): 2
<4028>pid 4177: propagate session 4285
<4028>pid 4177: creator set to 4285
<4028>pid 4177: set session
<4028>pid 4177: moving up to 4285
<4028>[0] pid 4285 ppid 4177 sid 0 creator 0
<4028>[1] pid 4177 ppid 4285 sid 4285 creator 4285 S G
<4028>found pgid/sid 0, need pidns
<4028>new pidns without init
<4028>forking coordinator in new pidns
<1>forking child vpid 4285 flags 0x1
<1>forked child vpid 2 (asked 4285)
<2>root task pid 2
<2>pid 4285: pid 2 sid 0 parent 1
<2>pid 4285: fork child 4177 with session
<2>forking child vpid 4177 flags 0x12
<3>pid 4177: pid 3 sid 0 parent 2
<4029>c/r swap old 4177 new 3
<3>about to call sys_restart(), flags 0x4
<2>forked child vpid 3 (asked 4177)
<4029>c/r swap old 4285 new 2
<4029>c/r read input 16384
<4029>c/r read input 16384
<4029>c/r read input 16384
restart failed: Operation not permitted
Failed
<1>restart failed ?
<4029>c/r read input 16384
<4029>c/r read input 14862
<2>about to call sys_restart(), flags 0
<4028>SIGCHLD: already collected
<4028>task exited with status 255
[hallyn@linuz11 ~]$ echo $?
0

And when I previously added a debug statement at the end of main() printin gout the return value, it would say 'returning 0' even though 'restart failed ?' in the restart -vd output inicates that ret < 0.

Note that if you don't set cap_sys_admin+pe on
/bin/restart, then restart does return 255 (-1).

I'm done for the day - will have to look at it more tomorrow.

-serge

Quoted 33 lines · Oren Laadan

What was the command line you used for the restart ?

Oren.

Quoted 27 lines · Serge E. Hallyn

All right I can't explain it at the moment, but exit(1) at ckpt_coordinator() (on error) causes restart.c to exit with 0, whereas return(ret) causes it to correctly exit with -1.

Without this, a restart whose kernel portion ends with say -1 ends up returning from user-cr/restart.c with 0, so userspace can't tell whether sys_restart succeeded or failed. With the patch, it fails correctly.

---
 restart.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
restart.c +1 -1
index e6e72ac..87899ba 100644
@@ -933,7 +933,7 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
 		perror("restart failed");
 		ckpt_verbose("Failed\n");
 		ckpt_dbg("restart failed ?\n");
-		exit(1);
+		return ret;
 	}
 
 	ckpt_verbose("Success\n");

A message that is not in the archive (sent off-list or not yet received): 20091019035033.GA23390-r/[email protected]

#6 lore raw
Quoted 12 lines · Serge E. Hallyn

Hmm, well... I dont' know what's going on with my userspace, Now it's not working for me even with this patch.

As root I do:

setcap cap_sys_admin+pe /bin/restart
ckpt > /tmp/out

then as a non-rootuid I do

	cat /tmp/out | /bin/retart -vd
	echo $?
Quoted 15 lines

giving me:
[hallyn@linuz11 ~]$ /bin/restart -vd < /tmp/out
<4028>number of tasks: 1
<4028>total tasks (including ghosts): 2
<4028>pid 4177: propagate session 4285
<4028>pid 4177: creator set to 4285
<4028>pid 4177: set session
<4028>pid 4177: moving up to 4285
<4028>[0] pid 4285 ppid 4177 sid 0 creator 0
<4028>[1] pid 4177 ppid 4285 sid 4285 creator 4285 S G
<4028>found pgid/sid 0, need pidns
<4028>new pidns without init
<4028>forking coordinator in new pidns
<1>forking child vpid 4285 flags 0x1
<1>forked child vpid 2 (asked 4285)

Note that 'restart' decided to start a new pidns even though you didn't ask for one. This may be related to the problem, because it creates another intermediate process at restart - the container init.

My recent user-space patch should fix this, and 'restart' should not start a new pidns in this case. This may also solve your current problem.

However, we still need to figure out why it does not report the error correctly in the --pidns case.

Oren.

Quoted 72 lines

<2>root task pid 2
<2>pid 4285: pid 2 sid 0 parent 1
<2>pid 4285: fork child 4177 with session
<2>forking child vpid 4177 flags 0x12
<3>pid 4177: pid 3 sid 0 parent 2
<4029>c/r swap old 4177 new 3
<3>about to call sys_restart(), flags 0x4
<2>forked child vpid 3 (asked 4177)
<4029>c/r swap old 4285 new 2
<4029>c/r read input 16384
<4029>c/r read input 16384
<4029>c/r read input 16384
restart failed: Operation not permitted
Failed
<1>restart failed ?
<4029>c/r read input 16384
<4029>c/r read input 14862
<2>about to call sys_restart(), flags 0
<4028>SIGCHLD: already collected
<4028>task exited with status 255
[hallyn@linuz11 ~]$ echo $?
0

And when I previously added a debug statement at the end of main() printin gout the return value, it would say 'returning 0' even though 'restart failed ?' in the restart -vd output inicates that ret < 0.

Note that if you don't set cap_sys_admin+pe on
/bin/restart, then restart does return 255 (-1).

I'm done for the day - will have to look at it more tomorrow.

-serge

Quoted 33 lines · Oren Laadan

What was the command line you used for the restart ?

Oren.

Quoted 27 lines · Serge E. Hallyn

All right I can't explain it at the moment, but exit(1) at ckpt_coordinator() (on error) causes restart.c to exit with 0, whereas return(ret) causes it to correctly exit with -1.

Without this, a restart whose kernel portion ends with say -1 ends up returning from user-cr/restart.c with 0, so userspace can't tell whether sys_restart succeeded or failed. With the patch, it fails correctly.

---
 restart.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
restart.c +1 -1
index e6e72ac..87899ba 100644
@@ -933,7 +933,7 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
 		perror("restart failed");
 		ckpt_verbose("Failed\n");
 		ckpt_dbg("restart failed ?\n");
-		exit(1);
+		return ret;
 	}
 
 	ckpt_verbose("Success\n");