[PATCH] libxl: execute command by execvp()

[PATCH] libxl: execute command by execvp()

[Yu Zhiguo]

execute command by execvp() so can search command in PATH.

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);
 }

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

[Yu Zhiguo]

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.

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
> 
> 
> 

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

[Ian Campbell]

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
> > 
> > 
> > 
> 

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

[Yu Zhiguo]

Ian Campbell wrote:
>> 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)?
> 

 Perhaps parent process goes to waitpid (with WNOHANG flag) but child
hasn't gone to execv("pygrub").

> 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).
> 
 We can open fifo in NONBLOCK mode but I'm not sure we'll not meet
the above problem?
 What about waitpid but no WNOHANG, we can catch the exit status of child
and check it.

> 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?)

 I think search PATH is enough...

Yu

> 
> 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
>>>
>>>
>>>
> 
> 
> 
> 
> 

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

[Ian Campbell]

On Thu, 2010-07-22 at 03:46 +0100, Yu Zhiguo wrote:
> Ian Campbell wrote:
> >> 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)?
> > 
> 
>  Perhaps parent process goes to waitpid (with WNOHANG flag) but child
> hasn't gone to execv("pygrub").

Yes, as I said I'm not sure how to make 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).
> > 
>  We can open fifo in NONBLOCK mode but I'm not sure we'll not meet
> the above problem?

open(7) says this regarding O_NONBLOCK:
        Normally, opening the FIFO blocks until the other end is opened
        also.

        A process can open a FIFO in non-blocking mode.  In this case,
        opening for read only will succeed even if no-one has opened on
        the write side yet; opening for write only will fail with  ENXIO
        (no such device or address) unless the other end has already
        been opened.

We would be opening the FIFO read-only and hence we shouldn't meet the
above problem. Once we get to bootloader_interact we will either wait in
the select() until the exec is successful and stuff starts to happen or
we get EOF on the PTY master because the slave has been closed when the
child exits after failing to exec.

>  What about waitpid but no WNOHANG, we can catch the exit status of child
> and check it.

That would hang in the case where the child did exec successfully, or at
least break user-interaction since we wouldn't reach bootloader_interact
in time for the user to do anything.

> > 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?)
> 
>  I think search PATH is enough...

OK.

Ian.

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

[Stefano Stabellini]

On Wed, 21 Jul 2010, 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?

I have already applied your patch.