Re: [Libvir] [patch 6/9] Move iptablesSpawn()
On Fri, Jan 04, 2008 at 03:57:31PM +0000, Mark McLoughlin wrote:
The next patch requires iptablesSpawn() higher up in the file. This patch just moves the code around; there is no functional change.
The util.c file has a virExec function, although its a little more cumbersome to use than iptablesSpawn. So for the storage APIs I've got a new virRun() function in util.c which is very similar to your iptablesSpawn code. How about we just make iptables use the virRun method in the attached patch ? diff -r a0ae4f87480b src/util.c --- a/src/util.c Thu Dec 06 15:35:43 2007 -0500 +++ b/src/util.c Thu Dec 06 15:39:34 2007 -0500 @@ -34,6 +34,7 @@ #include <errno.h> #include <sys/types.h> #include <sys/stat.h> +#include <sys/wait.h> #include <string.h> #include "libvirt/virterror.h" @@ -196,6 +197,43 @@ virExecNonBlock(virConnectPtr conn, int *retpid, int infd, int *outfd, int *errfd) { return(_virExec(conn, argv, retpid, infd, outfd, errfd, 1)); +} + +/** + * @conn connection to report errors against + * @argv NULL terminated argv to run + * @status optional variable to return exit status in + * + * Run a command without using the shell. + * + * If status is NULL, then return 0 if the command run and + * exited with 0 status; Otherwise return -1 + * + * If status is not-NULL, then return 0 if the command ran. + * The status variable is filled with the command exit status + * and should be checked by caller for success. Return -1 + * only if the command could not be run. + */ +int +virRun(virConnectPtr conn, + char **argv, + int *status) { + int childpid, exitstatus, ret; + + if ((ret = virExec(conn, argv, &childpid, -1, NULL, NULL)) < 0) + return ret; + + while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR); + if (ret == -1) + return -1; + + if (status == NULL) { + errno = EINVAL; + return (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) ? 0 : -1; + } else { + *status = exitstatus; + return 0; + } } /* Like read(), but restarts after EINTR */ diff -r a0ae4f87480b src/util.h --- a/src/util.h Thu Dec 06 15:35:43 2007 -0500 +++ b/src/util.h Thu Dec 06 15:39:34 2007 -0500 @@ -28,6 +28,7 @@ int virExec(virConnectPtr conn, char **argv, int *retpid, int infd, int *outfd, int *errfd); int virExecNonBlock(virConnectPtr conn, char **argv, int *retpid, int infd, int *outfd, int *errfd); +int virRun(virConnectPtr conn, char **argv, int *status); int saferead(int fd, void *buf, size_t count); ssize_t safewrite(int fd, const void *buf, size_t count); Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Sat, 2008-01-05 at 00:11 +0000, Daniel P. Berrange wrote:
On Fri, Jan 04, 2008 at 03:57:31PM +0000, Mark McLoughlin wrote:
The next patch requires iptablesSpawn() higher up in the file. This patch just moves the code around; there is no functional change.
The util.c file has a virExec function, although its a little more cumbersome to use than iptablesSpawn. So for the storage APIs I've got a new virRun() function in util.c which is very similar to your iptablesSpawn code. How about we just make iptables use the virRun method in the attached patch ?
Okay, the only thing lost with this is that when using WITH_ERRORS, iptables error spew would go to libvirtd stderr, now it goes to /dev/null. No big deal. (I'll fold the attached patch into the "use utils.[ch]" patch) Cheers, Mark.
On Mon, Jan 07, 2008 at 10:02:20AM +0000, Mark McLoughlin wrote:
On Sat, 2008-01-05 at 00:11 +0000, Daniel P. Berrange wrote:
On Fri, Jan 04, 2008 at 03:57:31PM +0000, Mark McLoughlin wrote:
The next patch requires iptablesSpawn() higher up in the file. This patch just moves the code around; there is no functional change.
The util.c file has a virExec function, although its a little more cumbersome to use than iptablesSpawn. So for the storage APIs I've got a new virRun() function in util.c which is very similar to your iptablesSpawn code. How about we just make iptables use the virRun method in the attached patch ?
Okay, the only thing lost with this is that when using WITH_ERRORS, iptables error spew would go to libvirtd stderr, now it goes to /dev/null. No big deal.
Well when we start libvirtd from the init script, its stderr gets pointed to /dev/null anyway, so you end up with same effect. The only difference would be for developers debugging. Perhaps we could make virRun spew stuff to stderr if libvirt is built with --enable-debug. Regards Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Sat, Jan 05, 2008 at 12:11:04AM +0000, Daniel P. Berrange wrote:
On Fri, Jan 04, 2008 at 03:57:31PM +0000, Mark McLoughlin wrote:
The next patch requires iptablesSpawn() higher up in the file. This patch just moves the code around; there is no functional change.
The util.c file has a virExec function, although its a little more cumbersome to use than iptablesSpawn. So for the storage APIs I've got a new virRun() function in util.c which is very similar to your iptablesSpawn code. How about we just make iptables use the virRun method in the attached patch ?
diff -r a0ae4f87480b src/util.c --- a/src/util.c Thu Dec 06 15:35:43 2007 -0500 +++ b/src/util.c Thu Dec 06 15:39:34 2007 -0500 @@ -34,6 +34,7 @@ #include <errno.h> #include <sys/types.h> #include <sys/stat.h> +#include <sys/wait.h> #include <string.h>
#include "libvirt/virterror.h" @@ -196,6 +197,43 @@ virExecNonBlock(virConnectPtr conn, int *retpid, int infd, int *outfd, int *errfd) {
return(_virExec(conn, argv, retpid, infd, outfd, errfd, 1)); +} + +/** + * @conn connection to report errors against + * @argv NULL terminated argv to run + * @status optional variable to return exit status in + * + * Run a command without using the shell. + * + * If status is NULL, then return 0 if the command run and + * exited with 0 status; Otherwise return -1 + * + * If status is not-NULL, then return 0 if the command ran. + * The status variable is filled with the command exit status + * and should be checked by caller for success. Return -1 + * only if the command could not be run. + */ +int +virRun(virConnectPtr conn, + char **argv, + int *status) { + int childpid, exitstatus, ret; + + if ((ret = virExec(conn, argv, &childpid, -1, NULL, NULL)) < 0) + return ret; + + while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR); + if (ret == -1) + return -1; + + if (status == NULL) { + errno = EINVAL; + return (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) ? 0 : -1; + } else { + *status = exitstatus; + return 0; + } }
/* Like read(), but restarts after EINTR */ diff -r a0ae4f87480b src/util.h --- a/src/util.h Thu Dec 06 15:35:43 2007 -0500 +++ b/src/util.h Thu Dec 06 15:39:34 2007 -0500 @@ -28,6 +28,7 @@
int virExec(virConnectPtr conn, char **argv, int *retpid, int infd, int *outfd, int *errfd); int virExecNonBlock(virConnectPtr conn, char **argv, int *retpid, int infd, int *outfd, int *errfd); +int virRun(virConnectPtr conn, char **argv, int *status);
int saferead(int fd, void *buf, size_t count); ssize_t safewrite(int fd, const void *buf, size_t count);
Looks fine to me, if this can be used now, no need to wait for the other parts of the storage patch, push it now. In general util stuff is best pushed earlier than later precisely to try to share as much as possible. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Mark McLoughlin