Re: [libvirt] PATCH: 5/5: Make all code use virExec
Daniel P. Berrange [EMAIL PROTECTED] wrote:
...
+/* Like virFileReadLimFP, but use a file descriptor rather than a FILE*. */
+int __virFileReadLimFD(int fd_arg, int maxlen, char **buf)
+{
+int fd = dup (fd_arg);
+if (0 = fd) {
Can we stick to 'fd = 0' or 'fd 0' or 'fd == -1'. I find
the reversed constant-first conditionals rather painful to read.
Always have to stop and think about them for too long.
Sure. I've just adjusted it like this:
-if (0 = fd) {
+if (fd = 0) {
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: 5/5: Make all code use virExec
Daniel P. Berrange [EMAIL PROTECTED] wrote:
This final patch switches over all places which do fork()/exec() to use the
new enhanced virExec() code. A fair amount of code is deleted, but that's
not the whole story - the new impl is more robust that most of the existing
code we're deleting here.
Nice clean-up!
diff -r 2591ebc40bd7 src/bridge.c
...
@@ -680,7 +645,10 @@
argv[n++] = NULL;
-retval = brctlSpawn(argv);
+if (virRun(NULL, argv, NULL) 0)
+retval = errno;
Using errno here isn't always kosher, since _virExec doesn't always
preserve it when things go wrong (the ReportError call and close
calls after cleanup: can clobber errno).
But one can argue that it doesn't matter too much, since virExec
does diagnose each failure. However, that would suggest not
using errno upon virRun failure.
...
-retval = brctlSpawn(argv);
+if (virRun(NULL, argv, NULL) 0)
+retval = errno;
Likewise.
...
diff -r 2591ebc40bd7 src/qemu_conf.c
--- a/src/qemu_conf.c Tue Aug 12 15:29:29 2008 +0100
+++ b/src/qemu_conf.c Tue Aug 12 15:33:42 2008 +0100
@@ -397,116 +397,88 @@
int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) {
+const char *const qemuarg[] = { qemu, -help, NULL };
+const char *const qemuenv[] = { LANG=C, NULL };
If you have to use just one environment variable, use LC_ALL=C.
It trumps all others, when it comes to locale-related things.
pid_t child;
-int newstdout[2];
+int newstdout = -1;
+char help[8192]; /* Ought to be enough to hold QEMU help screen */
+int got = 0, ret = -1, status;
+int major, minor, micro;
+int ver;
If you make the above 4 variables unsigned
and adjust the %d formats accordingly, the version
parsing will be a little tighter.
if (flags)
*flags = 0;
if (version)
*version = 0;
-if (pipe(newstdout) 0) {
+if (virExec(NULL, qemuarg, qemuenv, child,
+-1, newstdout, NULL, VIR_EXEC_NONE) 0)
return -1;
+
+
+while (got (sizeof(help)-1)) {
+int len;
+if ((len = read(newstdout, help+got, sizeof(help)-got-1)) = 0) {
+if (!len)
+break;
+if (errno == EINTR)
+continue;
+goto cleanup2;
+}
+got += len;
+}
Any reason not to use saferead in place of this while-loop?
+help[got] = '\0';
+
+if (sscanf(help, QEMU PC emulator version %d.%d.%d, major,minor,
micro) != 3) {
+goto cleanup2;
}
-if ((child = fork()) 0) {
-close(newstdout[0]);
-close(newstdout[1]);
-return -1;
+ver = (major * 1000 * 1000) + (minor * 1000) + micro;
+if (version)
+*version = ver;
+if (flags) {
+if (strstr(help, -no-kqemu))
+*flags |= QEMUD_CMD_FLAG_KQEMU;
+if (strstr(help, -no-reboot))
+*flags |= QEMUD_CMD_FLAG_NO_REBOOT;
+if (strstr(help, -name))
+*flags |= QEMUD_CMD_FLAG_NAME;
+if (strstr(help, -drive))
+*flags |= QEMUD_CMD_FLAG_DRIVE;
+if (strstr(help, boot=on))
+*flags |= QEMUD_CMD_FLAG_DRIVE_BOOT;
+if (ver = 9000)
+*flags |= QEMUD_CMD_FLAG_VNC_COLON;
+}
+ret = 0;
+
+qemudDebug(Version %d %d %d Cooked version: %d, with flags ? %d,
If version can really be NULL, as the above if (version) tests
suggest, then you'll want to change *version here, to e.g.,
version ? *version : NA
Same for *flags.
+ major, minor, micro, *version, *flags);
+
+cleanup2:
+if (close(newstdout) 0)
+ret = -1;
+
+rewait:
+if (waitpid(child, status, 0) != child) {
+if (errno == EINTR)
+goto rewait;
+
+qemudLog(QEMUD_ERR,
+ _(Unexpected exit status from qemu %d pid %lu),
+ status, (unsigned long)child);
You've probably already fixed this, but status here is a
combination of exit status and signal number.
+ret = -1;
+}
+/* Check log unexpected exit status, but don't fail,
+ * as there's really no need to throw an error if we did
+ * actually read a valid version number above */
+if (WEXITSTATUS(status) != 0) {
+qemudLog(QEMUD_WARN,
+ _(Unexpected exit status '%d', qemu probably failed),
+ status);
}
...
int qemudExtractVersion(virConnectPtr conn,
diff -r 2591ebc40bd7 src/remote_internal.c
--- a/src/remote_internal.c Tue Aug 12 15:29:29 2008 +0100
+++ b/src/remote_internal.c Tue Aug 12 15:33:42 2008 +0100
@@ -72,6 +72,7 @@
#include remote_internal.h
#include remote_protocol.h
#include memory.h
+#include util.h
#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt,__VA_ARGS__)
#define DEBUG0(msg) VIR_DEBUG(__FILE__, %s, msg)
@@ -221,61 +222,21 @@
remoteForkDaemon(virConnectPtr conn)
{
const char
Re: [libvirt] PATCH: 5/5: Make all code use virExec
This final patch switches over all places which do fork()/exec() to use the
new enhanced virExec() code. A fair amount of code is deleted, but that's
not the whole story - the new impl is more robust that most of the existing
code we're deleting here.
bridge.c | 51 +++-
proxy_internal.c | 25 +---
qemu_conf.c | 168 ++
remote_internal.c | 88
4 files changed, 100 insertions(+), 232 deletions(-)
Daniel
diff -r 2591ebc40bd7 src/bridge.c
--- a/src/bridge.c Tue Aug 12 15:29:29 2008 +0100
+++ b/src/bridge.c Tue Aug 12 15:33:42 2008 +0100
@@ -46,6 +46,7 @@
#include internal.h
#include memory.h
+#include util.h
#define MAX_BRIDGE_ID 256
@@ -596,42 +597,6 @@
return brGetInetAddr(ctl, ifname, SIOCGIFNETMASK, addr, maxlen);
}
-static int
-brctlSpawn(char * const *argv)
-{
-pid_t pid, ret;
-int status;
-int null = -1;
-
-if ((null = open(_PATH_DEVNULL, O_RDONLY)) 0)
-return errno;
-
-pid = fork();
-if (pid == -1) {
-int saved_errno = errno;
-close(null);
-return saved_errno;
-}
-
-if (pid == 0) { /* child */
-dup2(null, STDIN_FILENO);
-dup2(null, STDOUT_FILENO);
-dup2(null, STDERR_FILENO);
-close(null);
-
-execvp(argv[0], argv);
-
-_exit (1);
-}
-
-close(null);
-
-while ((ret = waitpid(pid, status, 0) == -1) errno == EINTR);
-if (ret == -1)
-return errno;
-
-return (WIFEXITED(status) WEXITSTATUS(status) == 0) ? 0 : EINVAL;
-}
/**
* brSetForwardDelay:
@@ -649,7 +614,7 @@
const char *bridge,
int delay)
{
-char **argv;
+const char **argv;
int retval = ENOMEM;
int n;
char delayStr[30];
@@ -680,7 +645,10 @@
argv[n++] = NULL;
-retval = brctlSpawn(argv);
+if (virRun(NULL, argv, NULL) 0)
+retval = errno;
+else
+retval = 0;
error:
if (argv) {
@@ -709,7 +677,7 @@
const char *bridge,
int enable)
{
-char **argv;
+const char **argv;
int retval = ENOMEM;
int n;
@@ -737,7 +705,10 @@
argv[n++] = NULL;
-retval = brctlSpawn(argv);
+if (virRun(NULL, argv, NULL) 0)
+retval = errno;
+else
+retval = 0;
error:
if (argv) {
diff -r 2591ebc40bd7 src/proxy_internal.c
--- a/src/proxy_internal.c Tue Aug 12 15:29:29 2008 +0100
+++ b/src/proxy_internal.c Tue Aug 12 15:33:42 2008 +0100
@@ -162,6 +162,7 @@
{
const char *proxyPath = virProxyFindServerPath();
int ret, pid, status;
+const char *proxyarg[2];
if (!proxyPath) {
fprintf(stderr, failed to find libvirt_proxy\n);
@@ -171,27 +172,11 @@
if (debug)
fprintf(stderr, Asking to launch %s\n, proxyPath);
-/* Become a daemon */
-pid = fork();
-if (pid == 0) {
-long open_max;
-long i;
+proxyarg[0] = proxyPath;
+proxyarg[1] = NULL;
-/* don't hold open fd opened from the client of the library */
-open_max = sysconf (_SC_OPEN_MAX);
-for (i = 0; i open_max; i++)
-fcntl (i, F_SETFD, FD_CLOEXEC);
-
-setsid();
-if (fork() == 0) {
-execl(proxyPath, proxyPath, NULL);
-fprintf(stderr, _(failed to exec %s\n), proxyPath);
-}
-/*
- * calling exit() generate troubles for termination handlers
- */
-_exit(0);
-}
+if (virExec(NULL, proxyarg, NULL, pid, -1, NULL, NULL, VIR_EXEC_DAEMON)
0)
+fprintf(stderr, Failed to fork libvirt_proxy\n);
/*
* do a waitpid on the intermediate process to avoid zombies.
diff -r 2591ebc40bd7 src/qemu_conf.c
--- a/src/qemu_conf.c Tue Aug 12 15:29:29 2008 +0100
+++ b/src/qemu_conf.c Tue Aug 12 15:33:42 2008 +0100
@@ -397,116 +397,88 @@
int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) {
+const char *const qemuarg[] = { qemu, -help, NULL };
+const char *const qemuenv[] = { LANG=C, NULL };
pid_t child;
-int newstdout[2];
+int newstdout = -1;
+char help[8192]; /* Ought to be enough to hold QEMU help screen */
+int got = 0, ret = -1, status;
+int major, minor, micro;
+int ver;
if (flags)
*flags = 0;
if (version)
*version = 0;
-if (pipe(newstdout) 0) {
+if (virExec(NULL, qemuarg, qemuenv, child,
+-1, newstdout, NULL, VIR_EXEC_NONE) 0)
return -1;
+
+
+while (got (sizeof(help)-1)) {
+int len;
+if ((len = read(newstdout, help+got, sizeof(help)-got-1)) = 0) {
+if (!len)
+break;
+if (errno == EINTR)
+continue;
+goto cleanup2;
+}
+got += len;
+}
+help[got] = '\0';
+
+if (sscanf(help, QEMU PC emulator version %d.%d.%d,
