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

Ian Campbell
2010-07-21T08:29:53Z

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@cn.fujitsu.com>
> >
> > 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@lists.xensource.com
> > http://lists.xensource.com/xen-devel