Re: [lxc-devel] [PATCH 1/5] start child in its own process group, and put it into the foreground

2010-06-14 Thread Greg Kurz 
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

2010-06-11 Thread Matt Helsley 
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

2010-06-10 Thread Daniel Lezcano 
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

2010-06-10 Thread Ferenc Wagner 
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

2010-06-09 Thread Ferenc Wagner 
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