Re: [Xen-devel] [PATCH] libxl: execute command by execvp()

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Wed, 2010-07-21 at 08:11 +0100, Yu Zhiguo wrote:
> Hi Ian,
> 
> Yu Zhiguo wrote:
> > execute command by execvp() so can search command in PATH.
> > 
> 
>  It is trivial, but can you ack this fix?
>  Before this fix, when create guest, we must use absolute path
> for bootloader, e.g. bootloader = "/usr/bin/pygrub".
>  If not in absolute path now, xl create will block in:
> 
>     pid = fork_exec_bootloader(&bootloader_fd, (char *)info->u.pv.bootloader, 
> args);
>  ...
>     while (1) {
>         fifo_fd = open(fifo, O_RDONLY); ------------> here
> 
>  because pygrub cannot be executed so no data will be outputted into this 
> fifo.

Hmm, perhaps we need some more error handling from fork_exec_bootloader,
probably in addition to switching to execvp()? Perhaps a waitpid(..,..,
WNOHANG) sometime before the fifo open to check the child hasn't gone
away (although I'm not sure how oen makes this non-racey)?

Alternatively, maybe simply opening the FIFO O_NDELAY rather than using
open+fcntl is sufficient stop stop us blocking here? I guess that would
be safe since this is the read end of the FIFO. We would catch the
bootloader failure (to exec or otherwise) later on in
bootloader_interact (which either works already due to select returning
EBADF or probably needs fixing to handle the bootloader failing
regardless).

Also, xend special cases the bare word "pygrub" and searches a specific
list of likely install locations, perhaps libxl should duplicate that
behaviour? (I think I prefer search $PATH to this but maybe consistency
with previous behaviour trumps that?)

Ian.

> 
> Yu
> 
> > Signed-off-by: Yu Zhiguo <yuzg@xxxxxxxxxxxxxx>
> > 
> > diff -r 12f0618400de -r da4c3756920e tools/libxl/libxl_exec.c
> > --- a/tools/libxl/libxl_exec.c      Fri Jul 16 13:54:44 2010 +0100
> > +++ b/tools/libxl/libxl_exec.c      Tue Jul 20 02:14:44 2010 +0800
> > @@ -53,7 +53,7 @@
> >      /* in case our caller set it to IGN.  subprocesses are entitled
> >       * to assume they got DFL. */
> >  
> > -    execv(arg0, args);
> > +    execvp(arg0, args);
> >      _exit(-1);
> >  }
> >  
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxxxxxxxx
> > http://lists.xensource.com/xen-devel
> > 
> > 
> > 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel