Re: [lxc-devel] [PATCH 1/5] start child in its own process group, and put it into the foreground
On Fri, 2010-06-11 at 14:26 -0700, Matt Helsley wrote: I think shells implementing job control do it in the parent (shell) rather than the child (job) purely out of convention. It might be good They usually do it in both the parent and child. to follow a similar convention even if lxc is not, strictly speaking, a shell. This could be done but it isn't really needed as the parent and child have a synchronization point anyway to setup the container. Cheers. -- Gregory Kurz gk...@fr.ibm.com Software Engineer @ IBM/Meiosys http://www.ibm.com Tel +33 (0)534 638 479 Fax +33 (0)561 400 420 Anarchy is about taking complete responsibility for yourself. Alan Moore. -- ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo ___ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 1/5] start child in its own process group, and put it into the foreground
On Thu, Jun 10, 2010 at 11:57:20PM +0200, Ferenc Wagner wrote: Daniel Lezcano daniel.lezc...@free.fr writes: On 06/09/2010 07:56 PM, Ferenc Wagner wrote: @@ -509,6 +510,22 @@ int lxc_spawn(struct lxc_handler *handler) } } + if (setpgid(handler-pid, 0)) { + SYSERROR(failed to create new process group); + goto out_delete_net; + } + DEBUG(created new process group %d, handler-pid); + ctty = open(/dev/tty, O_RDONLY); + if (ctty != -1) { + int ret = tcsetpgrp(ctty, handler-pid); + close(ctty); + if (ret) { + SYSERROR(failed to set terminal foreground process group); + goto out_delete_net; + } + DEBUG(set terminal foreground process group); + } Is there a particular reason to do that from the parent and not from the child ? I can't think of one. It shouldn't matter, as long as the child can open /dev/tty. I think shells implementing job control do it in the parent (shell) rather than the child (job) purely out of convention. It might be good to follow a similar convention even if lxc is not, strictly speaking, a shell. Cheers, -Matt Helsley -- ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo ___ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 1/5] start child in its own process group, and put it into the foreground
On 06/09/2010 07:56 PM, Ferenc Wagner wrote: Signed-off-by: Ferenc Wagnerwf...@niif.hu --- src/lxc/start.c | 17 + 1 files changed, 17 insertions(+), 0 deletions(-) diff --git a/src/lxc/start.c b/src/lxc/start.c index b69ac88..7bbcf5a 100644 --- a/src/lxc/start.c +++ b/src/lxc/start.c @@ -463,6 +463,7 @@ int lxc_spawn(struct lxc_handler *handler) int clone_flags; int failed_before_rename = 0; const char *name = handler-name; + int ctty; if (lxc_sync_init(handler)) return -1; @@ -509,6 +510,22 @@ int lxc_spawn(struct lxc_handler *handler) } } + if (setpgid(handler-pid, 0)) { + SYSERROR(failed to create new process group); + goto out_delete_net; + } + DEBUG(created new process group %d, handler-pid); + ctty = open(/dev/tty, O_RDONLY); + if (ctty != -1) { + int ret = tcsetpgrp(ctty, handler-pid); + close(ctty); + if (ret) { + SYSERROR(failed to set terminal foreground process group); + goto out_delete_net; + } + DEBUG(set terminal foreground process group); + } Is there a particular reason to do that from the parent and not from the child ? /* Tell the child to continue its initialization and wait for * it to exec or return an error */ -- ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo ___ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 1/5] start child in its own process group, and put it into the foreground
Daniel Lezcano daniel.lezc...@free.fr writes: On 06/09/2010 07:56 PM, Ferenc Wagner wrote: @@ -509,6 +510,22 @@ int lxc_spawn(struct lxc_handler *handler) } } +if (setpgid(handler-pid, 0)) { +SYSERROR(failed to create new process group); +goto out_delete_net; +} +DEBUG(created new process group %d, handler-pid); +ctty = open(/dev/tty, O_RDONLY); +if (ctty != -1) { +int ret = tcsetpgrp(ctty, handler-pid); +close(ctty); +if (ret) { +SYSERROR(failed to set terminal foreground process group); +goto out_delete_net; +} +DEBUG(set terminal foreground process group); +} Is there a particular reason to do that from the parent and not from the child ? I can't think of one. It shouldn't matter, as long as the child can open /dev/tty. -- Regards, Feri. -- ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo ___ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 1/5] start child in its own process group, and put it into the foreground
Matt Helsley matth...@us.ibm.com writes: On Wed, Jun 09, 2010 at 07:56:03PM +0200, Ferenc Wagner wrote: Signed-off-by: Ferenc Wagner wf...@niif.hu --- src/lxc/start.c | 17 + 1 files changed, 17 insertions(+), 0 deletions(-) diff --git a/src/lxc/start.c b/src/lxc/start.c index b69ac88..7bbcf5a 100644 --- a/src/lxc/start.c +++ b/src/lxc/start.c @@ -463,6 +463,7 @@ int lxc_spawn(struct lxc_handler *handler) int clone_flags; int failed_before_rename = 0; const char *name = handler-name; +int ctty; if (lxc_sync_init(handler)) return -1; @@ -509,6 +510,22 @@ int lxc_spawn(struct lxc_handler *handler) } } +if (setpgid(handler-pid, 0)) { I think this races with the exec in the child. From the setpgid() man page: ERRORS EACCES An attempt was made to change the process group ID of one of the children of the calling process and the child had already per‐ formed an execve(2) (setpgid(), setpgrp()). You may be able to fix this by also doing setpgid() in the child before the exec. Acked-by: Matt Helsley matth...@us.ibm.com Thanks for the review. Isn't the race excluded by lxc_sync_barrier_child(handler, LXC_SYNC_POST_CONFIGURE) being on the following lines, not before? Then I was misled by the comment before it. -- Cheers, Feri. -- ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo ___ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel