libxl: execute command by execvp()

PATCH In review Yu Zhiguo 3 participants 6 messages Jul 19, 2010 – Jul 22, 2010

lore

Share & tools

lore

Yu Zhiguo

author

2010-07-19 10:11 UTC #0 lore raw

PATCH 1/1 libxl: execute command by execvp()

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

Signed-off-by Yu Zhiguo <[email protected]>

tools/libxl/libxl_exec.c +1 -1

diff -r 12f0618400de -r da4c3756920e tools/libxl/libxl_exec.c
@@ -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);
 }

Yu Zhiguo

author

2010-07-21 07:11 UTC #1 lore raw

Hi Ian,

Quoted 2 lines · Yu Zhiguo

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.

Quoted 23 lines

Signed-off-by Yu Zhiguo <[email protected]>
diff -r 12f0618400de -r da4c3756920e tools/libxl/libxl_exec.c
tools/libxl/libxl_exec.c +1 -1
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2010-07-21 08:29 UTC #2 lore raw

Quoted 11 lines · Yu Zhiguo

Hi Ian,
Quoted 2 lines · Yu Zhiguo
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:

Quoted 6 lines

    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.

Quoted 27 lines

Yu
Quoted 23 lines
Signed-off-by Yu Zhiguo <[email protected]>
diff -r 12f0618400de -r da4c3756920e tools/libxl/libxl_exec.c
tools/libxl/libxl_exec.c +1 -1
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

Yu Zhiguo

author

2010-07-22 02:46 UTC #3 lore raw

Quoted 13 lines · Ian Campbell

Quoted 13 lines
Quoted 2 lines · Yu Zhiguo
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

Quoted 7 lines

Quoted 1 line
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").

Quoted 8 lines

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.

Quoted 4 lines

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

Quoted 33 lines

Ian.
Quoted 25 lines
Yu
Quoted 23 lines
Signed-off-by Yu Zhiguo <[email protected]>
diff -r 12f0618400de -r da4c3756920e tools/libxl/libxl_exec.c
tools/libxl/libxl_exec.c +1 -1
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2010-07-22 08:34 UTC #4 lore raw

Quoted 16 lines · Yu Zhiguo

Quoted 15 lines · Ian Campbell
Quoted 14 lines
Quoted 2 lines · Yu Zhiguo
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.

Quoted 8 lines

Quoted 5 lines
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 ;-)

Quoted 10 lines

Quoted 8 lines
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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 4 lines
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.

Stefano Stabellini

2010-07-21 11:59 UTC #5 lore raw

In reply to Yu Zhiguo

Quoted 7 lines · Yu Zhiguo

Hi Ian,
Quoted 2 lines · Yu Zhiguo
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.