[Libvir] [patch 0/9] More miscellaneous qemud patches
Hey, Okay, here's another batch of qemud patches I've committed. The most significant ones are the signal handling and error reporting ones at the end. Appreciate someone looking over them. Thanks, Mark. --
Just move qemudFreeVMDef() alonside qemudFreeVM() Signed-off-by: Mark McLoughlin <markmc@redhat.com> Index: libvirt/qemud/conf.h =================================================================== --- libvirt.orig/qemud/conf.h +++ libvirt/qemud/conf.h @@ -30,12 +30,12 @@ int qemudBuildCommandLine(struct qemud_s struct qemud_vm *vm, char ***argv); -void qemudFreeVMDef(struct qemud_vm_def *vm); int qemudScanConfigs(struct qemud_server *server); int qemudDeleteConfig(struct qemud_server *server, const char *configFile, const char *name); +void qemudFreeVMDef(struct qemud_vm_def *vm); void qemudFreeVM(struct qemud_vm *vm); struct qemud_vm *qemudLoadConfigXML(struct qemud_server *server, const char *file, --
Silly typo causing a segv Signed-off-by: Mark McLoughlin <markmc@redhat.com> Index: libvirt/qemud/conf.c =================================================================== --- libvirt.orig/qemud/conf.c +++ libvirt/qemud/conf.c @@ -1558,6 +1558,7 @@ struct qemud_network *qemudLoadNetworkCo return NULL; } + network->def = def; newNetwork = 1; } --
A couple of typos: 1) confusion around the isGuest arg to ScanConfigDir() 2) forgetting a quot in network interface xml output Signed-off-by: Mark McLoughlin <markmc@redhat.com> Index: libvirt/qemud/conf.c =================================================================== --- libvirt.orig/qemud/conf.c +++ libvirt/qemud/conf.c @@ -1663,9 +1663,9 @@ int qemudScanConfigDir(struct qemud_serv /* Scan for all guest and network config files */ int qemudScanConfigs(struct qemud_server *server) { - if (qemudScanConfigDir(server, server->configDir, 0) < 0) + if (qemudScanConfigDir(server, server->configDir, 1) < 0) return -1; - return qemudScanConfigDir(server, server->networkConfigDir, 1); + return qemudScanConfigDir(server, server->networkConfigDir, 0); } /* Simple grow-on-demand string buffer */ @@ -1890,7 +1890,7 @@ char *qemudGenerateXML(struct qemud_serv goto no_memory; if (net->type == QEMUD_NET_NETWORK) { - if (qemudBufferPrintf(&buf, " <network name='%s", net->dst.network.name) < 0) + if (qemudBufferPrintf(&buf, " <network name='%s'", net->dst.network.name) < 0) goto no_memory; if (net->dst.network.tapifname[0] != '\0' && --
It seems we were forgetting to handle QEMUD_GRAPHICS_SDL in qemudGenerateXML() Signed-off-by: Mark McLoughlin <markmc@redhat.com> Index: libvirt/qemud/conf.c =================================================================== --- libvirt.orig/qemud/conf.c +++ libvirt/qemud/conf.c @@ -1907,13 +1907,31 @@ char *qemudGenerateXML(struct qemud_serv net = net->next; } + switch (def->graphicsType) { + case QEMUD_GRAPHICS_VNC: + if (qemudBufferAdd(&buf, " <graphics type='vnc'") < 0) + goto no_memory; + + if (def->vncPort && + qemudBufferPrintf(&buf, " port='%d'", + vm->id >= 0 && live ? def->vncActivePort : def->vncPort) < 0) + goto no_memory; + + if (qemudBufferAdd(&buf, "/>\n") < 0) + goto no_memory; + break; + + case QEMUD_GRAPHICS_SDL: + if (qemudBufferAdd(&buf, " <graphics type='sdl'/>\n") < 0) + goto no_memory; + break; + + case QEMUD_GRAPHICS_NONE: + default: + break; + } + if (def->graphicsType == QEMUD_GRAPHICS_VNC) { - if (def->vncPort) { - qemudBufferPrintf(&buf, " <graphics type='vnc' port='%d'/>\n", - vm->id >= 0 && live ? def->vncActivePort : def->vncPort); - } else { - qemudBufferPrintf(&buf, " <graphics type='vnc'/>\n"); - } } if (qemudBufferAdd(&buf, " </devices>\n") < 0) --
We weren't handling the network interface type correctly and also outputting incorrect XML Signed-off-by: Mark McLoughlin <markmc@redhat.com> Index: libvirt/qemud/conf.c =================================================================== --- libvirt.orig/qemud/conf.c +++ libvirt/qemud/conf.c @@ -1876,6 +1876,7 @@ char *qemudGenerateXML(struct qemud_serv "server", "client", "mcast", + "network", "vde", }; if (qemudBufferPrintf(&buf, " <interface type='%s'>\n", @@ -1890,7 +1891,7 @@ char *qemudGenerateXML(struct qemud_serv goto no_memory; if (net->type == QEMUD_NET_NETWORK) { - if (qemudBufferPrintf(&buf, " <network name='%s'", net->dst.network.name) < 0) + if (qemudBufferPrintf(&buf, " <source network='%s'", net->dst.network.name) < 0) goto no_memory; if (net->dst.network.tapifname[0] != '\0' && --
Don't leak an fd if we fail to write to /proc/sys/net/ipv4/ip_forward Also, fix indentation Signed-off-by: Mark McLoughlin <markmc@redhat.com> Index: libvirt/qemud/qemud.c =================================================================== --- libvirt.orig/qemud/qemud.c +++ libvirt/qemud/qemud.c @@ -982,15 +982,17 @@ qemudEnableIpForwarding(void) { #define PROC_IP_FORWARD "/proc/sys/net/ipv4/ip_forward" - int fd; + int fd, ret; - if ((fd = open(PROC_IP_FORWARD, O_WRONLY|O_TRUNC)) == -1 || - write(fd, "1\n", 2) < 0) - return 0; + if ((fd = open(PROC_IP_FORWARD, O_WRONLY|O_TRUNC)) == -1) + return 0; + + if (write(fd, "1\n", 2) < 0) + ret = 0; - close (fd); + close (fd); - return 1; + return 1; #undef PROC_IP_FORWARD } --
Implement a sane policy around our use of FD_CLOEXEC: 1) Every descriptor which shouldn't be passed to child processes should have the flag set 2) Let exec() do the descriptor closing, rather than us doing it ourselves Signed-off-by: Mark McLoughlin <markmc@redhat.com> Index: libvirt/qemud/qemud.c =================================================================== --- libvirt.orig/qemud/qemud.c +++ libvirt/qemud/qemud.c @@ -87,7 +87,7 @@ static int qemudGoDaemon(void) { { int stdinfd = -1; int stdoutfd = -1; - int i, open_max, nextpid; + int nextpid; if ((stdinfd = open(_PATH_DEVNULL, O_RDONLY)) < 0) goto cleanup; @@ -106,13 +106,6 @@ static int qemudGoDaemon(void) { goto cleanup; stdoutfd = -1; - open_max = sysconf (_SC_OPEN_MAX); - for (i = 0; i < open_max; i++) - if (i != STDIN_FILENO && - i != STDOUT_FILENO && - i != STDERR_FILENO) - close(i); - if (setsid() < 0) goto cleanup; @@ -355,23 +348,8 @@ static int qemudDispatchServer(struct qe static int -qemudLeaveFdOpen(int *openfds, int fd) -{ - int i; - - if (!openfds) - return 0; - - for (i = 0; openfds[i] != -1; i++) - if (fd == openfds[i]) - return 1; - - return 0; -} - -static int qemudExec(struct qemud_server *server, char **argv, - int *retpid, int *outfd, int *errfd, int *openfds) { + int *retpid, int *outfd, int *errfd) { int pid, null; int pipeout[2] = {-1,-1}; int pipeerr[2] = {-1,-1}; @@ -400,11 +378,13 @@ qemudExec(struct qemud_server *server, c if (outfd) { close(pipeout[1]); qemudSetNonBlock(pipeout[0]); + qemudSetCloseExec(pipeout[0]); *outfd = pipeout[0]; } if (errfd) { close(pipeerr[1]); qemudSetNonBlock(pipeerr[0]); + qemudSetCloseExec(pipeerr[0]); *errfd = pipeerr[0]; } *retpid = pid; @@ -425,13 +405,11 @@ qemudExec(struct qemud_server *server, c if (dup2(pipeerr[1] > 0 ? pipeerr[1] : null, STDERR_FILENO) < 0) _exit(1); - int i, open_max = sysconf (_SC_OPEN_MAX); - for (i = 0; i < open_max; i++) - if (i != STDOUT_FILENO && - i != STDERR_FILENO && - i != STDIN_FILENO && - !qemudLeaveFdOpen(openfds, i)) - close(i); + close(null); + if (pipeout[1] > 0) + close(pipeout[1]); + if (pipeerr[1] > 0) + close(pipeerr[1]); execvp(argv[0], argv); @@ -441,13 +419,13 @@ qemudExec(struct qemud_server *server, c cleanup: if (pipeerr[0] > 0) - close(pipeerr[0] > 0); - if (pipeerr[1]) - close(pipeerr[1] > 0); - if (pipeout[0]) - close(pipeout[0] > 0); - if (pipeout[1]) - close(pipeout[1] > 0); + close(pipeerr[0]); + if (pipeerr[1] > 0) + close(pipeerr[1]); + if (pipeout[0] > 0) + close(pipeout[0]); + if (pipeout[1] > 0) + close(pipeout[1]); if (null > 0) close(null); return -1; @@ -467,7 +445,7 @@ int qemudStartVMDaemon(struct qemud_serv if (qemudBuildCommandLine(server, vm, &argv) < 0) return -1; - if (qemudExec(server, argv, &vm->pid, &vm->stdout, &vm->stderr, vm->tapfds) == 0) { + if (qemudExec(server, argv, &vm->pid, &vm->stdout, &vm->stderr) == 0) { vm->id = server->nextvmid++; ret = 0; } @@ -863,7 +841,7 @@ dhcpStartDhcpDaemon(struct qemud_server if (qemudBuildDnsmasqArgv(server, network, &argv) < 0) return -1; - ret = qemudExec(server, argv, &network->dnsmasqPid, NULL, NULL, NULL); + ret = qemudExec(server, argv, &network->dnsmasqPid, NULL, NULL); for (i = 0; argv[i]; i++) free(argv[i]); Index: libvirt/qemud/bridge.c =================================================================== --- libvirt.orig/qemud/bridge.c +++ libvirt/qemud/bridge.c @@ -54,6 +54,7 @@ int brInit(brControl **ctlp) { int fd; + int flags; if (!ctlp || *ctlp) return EINVAL; @@ -62,6 +63,13 @@ brInit(brControl **ctlp) if (fd < 0) return errno; + if ((flags = fcntl(fd, F_GETFD)) < 0 || + fcntl(fd, F_SETFD, flags | FD_CLOEXEC) < 0) { + int err = errno; + close(fd); + return err; + } + *ctlp = (brControl *)malloc(sizeof(struct _brControl)); if (!*ctlp) return ENOMEM; Index: libvirt/qemud/iptables.c =================================================================== --- libvirt.orig/qemud/iptables.c +++ libvirt/qemud/iptables.c @@ -317,15 +317,11 @@ iptablesSpawn(int errors, char * const * } if (pid == 0) { /* child */ - int i, open_max = sysconf(_SC_OPEN_MAX); - - for (i = 0; i < open_max; i++) { - if (i != STDOUT_FILENO && - i != STDERR_FILENO && - i != STDIN_FILENO) - close(i); - else if (errors == NO_ERRORS) - dup2(null, i); + if (errors == NO_ERRORS) { + dup2(null, STDIN_FILENO); + dup2(null, STDOUT_FILENO); + dup2(null, STDERR_FILENO); + close(null); } execvp(argv[0], argv); --
Mark McLoughlin <markmc@redhat.com> wrote:
Implement a sane policy around our use of FD_CLOEXEC:
1) Every descriptor which shouldn't be passed to child processes should have the flag set
2) Let exec() do the descriptor closing, rather than us doing it ourselves ... - for (i = 0; i < open_max; i++) { - if (i != STDOUT_FILENO && - i != STDERR_FILENO && - i != STDIN_FILENO) - close(i); - else if (errors == NO_ERRORS) - dup2(null, i); + if (errors == NO_ERRORS) { + dup2(null, STDIN_FILENO); + dup2(null, STDOUT_FILENO); + dup2(null, STDERR_FILENO); + close(null); }
True, dup2 failure wasn't checked before either, but it *can* fail. Best to diagnose it, just in case.
Handle SIGHUP by shutting down all guests and networks and re-loading configs Handle SIGTERM/SIGINT by cleanly shutting down Signed-off-by: Mark McLoughlin <markmc@redhat.com> Index: libvirt/qemud/internal.h =================================================================== --- libvirt.orig/qemud/internal.h +++ libvirt/qemud/internal.h @@ -275,6 +275,7 @@ struct qemud_server { int qemuVersion; int nclients; struct qemud_client *clients; + int sigread; int nvmfds; int nactivevms; struct qemud_vm *activevms; @@ -291,6 +292,7 @@ struct qemud_server { char networkConfigDir[PATH_MAX]; char errorMessage[QEMUD_MAX_ERROR_LEN]; int errorCode; + unsigned int shutdown : 1; }; int qemudStartVMDaemon(struct qemud_server *server, Index: libvirt/qemud/qemud.c =================================================================== --- libvirt.orig/qemud/qemud.c +++ libvirt/qemud/qemud.c @@ -52,9 +52,85 @@ #include "conf.h" #include "iptables.h" -static void reapchild(int sig ATTRIBUTE_UNUSED) { - /* We explicitly waitpid the child later */ +static int sigwrite = -1; + +static void sig_handler(int sig) { + unsigned char sigc = sig; + int origerrno; + + if (sig == SIGCHLD) /* We explicitly waitpid the child later */ + return; + + origerrno = errno; + write(sigwrite, &sigc, 1); + errno = origerrno; +} + +static int qemudDispatchSignal(struct qemud_server *server) +{ + unsigned char sigc; + struct qemud_vm *vm; + struct qemud_network *network; + int ret; + + if (read(server->sigread, &sigc, 1) != 1) + return -1; + + /* shutdown active VMs */ + vm = server->activevms; + while (vm) { + struct qemud_vm *next = vm->next; + qemudShutdownVMDaemon(server, vm); + vm = next; + } + + /* free inactive VMs */ + vm = server->inactivevms; + while (vm) { + struct qemud_vm *next = vm->next; + qemudFreeVM(vm); + vm = next; + } + server->inactivevms = NULL; + server->ninactivevms = 0; + + /* shutdown active networks */ + network = server->activenetworks; + while (network) { + struct qemud_network *next = network->next; + qemudShutdownNetworkDaemon(server, network); + network = next; + } + + /* free inactive networks */ + network = server->inactivenetworks; + while (network) { + struct qemud_network *next = network->next; + qemudFreeNetwork(network); + network = next; + } + server->inactivenetworks = NULL; + server->ninactivenetworks = 0; + + ret = 0; + + switch (sigc) { + case SIGHUP: + ret = qemudScanConfigs(server); + break; + + case SIGINT: + case SIGTERM: + server->shutdown = 1; + break; + + default: + break; + } + + return ret; } + static int qemudSetCloseExec(int fd) { int flags; if ((flags = fcntl(fd, F_GETFD)) < 0) { @@ -230,7 +306,7 @@ static int qemudListen(struct qemud_serv return 0; } -static struct qemud_server *qemudInitialize(int sys) { +static struct qemud_server *qemudInitialize(int sys, int sigread) { struct qemud_server *server; char libvirtConf[PATH_MAX]; @@ -241,6 +317,7 @@ static struct qemud_server *qemudInitial server->qemuVersion = (0*1000000)+(8*1000)+(0); /* We don't have a dom-0, so start from 1 */ server->nextvmid = 1; + server->sigread = sigread; if (sys) { if (snprintf(libvirtConf, sizeof(libvirtConf), "%s/libvirt", SYSCONF_DIR) >= (int)sizeof(libvirtConf)) { @@ -1141,6 +1218,12 @@ static int qemudDispatchPoll(struct qemu int ret = 0; int fd = 0; + if (fds[fd++].revents && qemudDispatchSignal(server) < 0) + return -1; + + if (server->shutdown) + return 0; + while (sock) { struct qemud_socket *next = sock->next; if (fds[fd].revents) @@ -1249,6 +1332,10 @@ static void qemudPreparePoll(struct qemu struct qemud_client *client; struct qemud_vm *vm; + fds[fd].fd = server->sigread; + fds[fd].events = POLLIN; + fd++; + for (sock = server->sockets ; sock ; sock = sock->next) { fds[fd].fd = sock->fd; fds[fd].events = POLLIN; @@ -1282,7 +1369,7 @@ static void qemudPreparePoll(struct qemu static int qemudOneLoop(struct qemud_server *server, int timeout) { - int nfds = server->nsockets + server->nclients + server->nvmfds; + int nfds = server->nsockets + server->nclients + server->nvmfds + 1; /* server->sigread */ struct pollfd fds[nfds]; int thistimeout = -1; int ret; @@ -1318,7 +1405,7 @@ static int qemudOneLoop(struct qemud_ser static int qemudRunLoop(struct qemud_server *server, int timeout) { int ret; - while ((ret = qemudOneLoop(server, timeout)) == 0) + while ((ret = qemudOneLoop(server, timeout)) == 0 && !server->shutdown) ; return ret == -1 ? -1 : 0; @@ -1326,6 +1413,7 @@ static int qemudRunLoop(struct qemud_ser static void qemudCleanup(struct qemud_server *server) { struct qemud_socket *sock = server->sockets; + close(server->sigread); while (sock) { close(sock->fd); sock = sock->next; @@ -1344,6 +1432,8 @@ int main(int argc, char **argv) { int sys = 0; int timeout = -1; struct qemud_server *server; + struct sigaction sig_action; + int sigpipe[2]; struct option opts[] = { { "verbose", no_argument, &verbose, 1}, @@ -1394,10 +1484,24 @@ int main(int argc, char **argv) { } } - if (signal(SIGPIPE, SIG_IGN) == SIG_ERR) - return 3; - if (signal(SIGCHLD, reapchild) == SIG_ERR) - return 3; + if (pipe(sigpipe) < 0 || + qemudSetNonBlock(sigpipe[0]) < 0 || + qemudSetNonBlock(sigpipe[1]) < 0) + return 1; + + sigwrite = sigpipe[1]; + + sig_action.sa_handler = sig_handler; + sig_action.sa_flags = 0; + sigemptyset(&sig_action.sa_mask); + + sigaction(SIGHUP, &sig_action, NULL); + sigaction(SIGINT, &sig_action, NULL); + sigaction(SIGTERM, &sig_action, NULL); + sigaction(SIGCHLD, &sig_action, NULL); + + sig_action.sa_handler = SIG_IGN; + sigaction(SIGPIPE, &sig_action, NULL); if (godaemon) { int pid = qemudGoDaemon(); @@ -1407,13 +1511,15 @@ int main(int argc, char **argv) { return 0; } - if (!(server = qemudInitialize(sys))) + if (!(server = qemudInitialize(sys, sigpipe[0]))) return 2; qemudRunLoop(server, timeout); qemudCleanup(server); + close(sigwrite); + return 0; } --
Mark McLoughlin <markmc@redhat.com> wrote:
Index: libvirt/qemud/qemud.c ... + write(sigwrite, &sigc, 1); ... + close(sigwrite);
Hi Mark, Diagnosing potential write and close failures might well save someone some time debugging. Jim
On Fri, Feb 16, 2007 at 02:44:46PM +0000, Mark McLoughlin wrote:
Handle SIGHUP by shutting down all guests and networks and re-loading configs
This violates the 'principle of least surprise'. I certainly do not expect a config file reload to terminate all active guests. Since QEMU is a full virt system, there is no graceful shutdown process, and so this is equivalent to ripping the power cable out of all your VMs. IMHO this makes SIGHUP essentially useless. We already have the ability to store a secondary config against any active guest VMs, which will be automatically activated upon next boot of the guest - this is how we let virDefineDomain() override the config of an existing VM. IMHO, sending SIGHUP to the daemon should just scan for config files and if any VMs are active, load into the secondary config.
Handle SIGTERM/SIGINT by cleanly shutting down
Ack, this is desirable. We should also hook in SIGQUIT 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 Fri, 2007-02-16 at 19:08 +0000, Daniel P. Berrange wrote:
On Fri, Feb 16, 2007 at 02:44:46PM +0000, Mark McLoughlin wrote:
Handle SIGHUP by shutting down all guests and networks and re-loading configs
This violates the 'principle of least surprise'. I certainly do not expect a config file reload to terminate all active guests. Since QEMU is a full virt system, there is no graceful shutdown process, and so this is equivalent to ripping the power cable out of all your VMs. IMHO this makes SIGHUP essentially useless.
We already have the ability to store a secondary config against any active guest VMs, which will be automatically activated upon next boot of the guest - this is how we let virDefineDomain() override the config of an existing VM. IMHO, sending SIGHUP to the daemon should just scan for config files and if any VMs are active, load into the secondary config.
All good points, fixed now with the attached patch.
Handle SIGTERM/SIGINT by cleanly shutting down
Ack, this is desirable. We should also hook in SIGQUIT
Yep, done. Cheers, Mark.
On Sat, Feb 17, 2007 at 01:22:35PM +0000, Mark McLoughlin wrote:
On Fri, 2007-02-16 at 19:08 +0000, Daniel P. Berrange wrote:
On Fri, Feb 16, 2007 at 02:44:46PM +0000, Mark McLoughlin wrote:
Handle SIGHUP by shutting down all guests and networks and re-loading configs
This violates the 'principle of least surprise'. I certainly do not expect a config file reload to terminate all active guests. Since QEMU is a full virt system, there is no graceful shutdown process, and so this is equivalent to ripping the power cable out of all your VMs. IMHO this makes SIGHUP essentially useless.
We already have the ability to store a secondary config against any active guest VMs, which will be automatically activated upon next boot of the guest - this is how we let virDefineDomain() override the config of an existing VM. IMHO, sending SIGHUP to the daemon should just scan for config files and if any VMs are active, load into the secondary config.
All good points, fixed now with the attached patch.
Looks good - simpler than I expected it to be - I forgot that the qemudScanConfigs function would already do the right thing if being run over.
Handle SIGTERM/SIGINT by cleanly shutting down
Ack, this is desirable. We should also hook in SIGQUIT
Yep, done.
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 -=|
Add a qemudLog() function which uses syslog() if we're in daemon mode, doesn't output INFO/DEBUG messages unless the verbose flag is set and doesn't output DEBUG messages unless compiled with --enable-debug. Also, make a first pass through fatal errors and add error messages for them. Signed-off-by: Mark McLoughlin <markmc@redhat.com> Index: libvirt/qemud/qemud.c =================================================================== --- libvirt.orig/qemud/qemud.c +++ libvirt/qemud/qemud.c @@ -38,6 +38,8 @@ #include <stdlib.h> #include <pwd.h> #include <stdio.h> +#include <stdarg.h> +#include <syslog.h> #include <string.h> #include <errno.h> #include <getopt.h> @@ -52,6 +54,8 @@ #include "conf.h" #include "iptables.h" +static int godaemon = 0; +static int verbose = 0; static int sigwrite = -1; static void sig_handler(int sig) { @@ -73,8 +77,14 @@ static int qemudDispatchSignal(struct qe struct qemud_network *network; int ret; - if (read(server->sigread, &sigc, 1) != 1) + if (read(server->sigread, &sigc, 1) != 1) { + qemudLog(QEMUD_ERR, "Failed to read from signal pipe: %s", + strerror(errno)); return -1; + } + + qemudLog(QEMUD_INFO, "Received signal %d; shuting down guests and " + "networks and purging config", sigc); /* shutdown active VMs */ vm = server->activevms; @@ -116,11 +126,13 @@ static int qemudDispatchSignal(struct qe switch (sigc) { case SIGHUP: + qemudLog(QEMUD_INFO, "Reloading configuration"); ret = qemudScanConfigs(server); break; case SIGINT: case SIGTERM: + qemudLog(QEMUD_WARN, "Shutting down on signal %d", sigc); server->shutdown = 1; break; @@ -133,27 +145,92 @@ static int qemudDispatchSignal(struct qe static int qemudSetCloseExec(int fd) { int flags; - if ((flags = fcntl(fd, F_GETFD)) < 0) { - return -1; - } + if ((flags = fcntl(fd, F_GETFD)) < 0) + goto error; flags |= FD_CLOEXEC; - if ((fcntl(fd, F_SETFD, flags)) < 0) { - return -1; - } + if ((fcntl(fd, F_SETFD, flags)) < 0) + goto error; return 0; + error: + qemudLog(QEMUD_ERR, "Failed to set close-on-exec file descriptor flag"); + return -1; } static int qemudSetNonBlock(int fd) { int flags; - if ((flags = fcntl(fd, F_GETFL)) < 0) { - return -1; - } + if ((flags = fcntl(fd, F_GETFL)) < 0) + goto error; flags |= O_NONBLOCK; - if ((fcntl(fd, F_SETFL, flags)) < 0) { - return -1; - } + if ((fcntl(fd, F_SETFL, flags)) < 0) + goto error; return 0; + error: + qemudLog(QEMUD_ERR, "Failed to set non-blocking file descriptor flag"); + return -1; +} + +void qemudLog(int priority, const char *fmt, ...) +{ + va_list args; + + va_start(args, fmt); + + if (godaemon) { + int sysprio = -1; + + switch(priority) { + case QEMUD_ERR: + sysprio = LOG_ERR; + break; + case QEMUD_WARN: + sysprio = LOG_WARNING; + break; + case QEMUD_INFO: + if (verbose) + sysprio = LOG_INFO; + break; +#ifdef ENABLE_DEBUG + case QEMUD_DEBUG: + if (verbose) + sysprio = LOG_DEBUG; + break; +#endif + default: + break; + } + + if (sysprio != -1) + vsyslog(sysprio, fmt, args); + } else { + switch(priority) { + case QEMUD_ERR: + case QEMUD_WARN: + vfprintf(stderr, fmt, args); + fputc('\n', stderr); + break; + + case QEMUD_INFO: + if (verbose) { + vprintf(fmt, args); + fputc('\n', stdout); + } + break; + +#ifdef ENABLE_DEBUG + case QEMUD_DEBUG: + if (verbose) { + vprintf(fmt, args); + fputc('\n', stdout); + } + break; +#endif + default: + break; + } + } + + va_end(args); } static int qemudGoDaemon(void) { @@ -229,20 +306,24 @@ static int qemudListenUnix(struct qemud_ struct sockaddr_un addr; mode_t oldmask; - if (!sock) + if (!sock) { + qemudLog(QEMUD_ERR, "Failed to allocate memory for struct qemud_socket"); return -1; + } sock->readonly = readonly; sock->next = server->sockets; server->sockets = sock; server->nsockets++; - if ((sock->fd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) + if ((sock->fd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { + qemudLog(QEMUD_ERR, "Failed to create socket: %s", + strerror(errno)); return -1; + } - if (qemudSetCloseExec(sock->fd) < 0) - return -1; - if (qemudSetNonBlock(sock->fd) < 0) + if (qemudSetCloseExec(sock->fd) < 0 || + qemudSetNonBlock(sock->fd) < 0) return -1; memset(&addr, 0, sizeof(addr)); @@ -256,12 +337,18 @@ static int qemudListenUnix(struct qemud_ oldmask = umask(~(S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH)); else oldmask = umask(~(S_IRUSR | S_IWUSR)); - if (bind(sock->fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) + if (bind(sock->fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) { + qemudLog(QEMUD_ERR, "Failed to bind socket to '%s': %s", + path, strerror(errno)); return -1; + } umask(oldmask); - if (listen(sock->fd, 30) < 0) + if (listen(sock->fd, 30) < 0) { + qemudLog(QEMUD_ERR, "Failed to listen for connections on '%s': %s", + path, strerror(errno)); return -1; + } return 0; } @@ -270,17 +357,16 @@ static int qemudListen(struct qemud_serv char sockname[PATH_MAX]; if (sys) { - if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) { - return -1; - } + if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) + goto snprintf_error; + unlink(sockname); if (qemudListenUnix(server, sockname, 0) < 0) return -1; + if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock-ro", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) + goto snprintf_error; - if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock-ro", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) { - return -1; - } unlink(sockname); if (qemudListenUnix(server, sockname, 1) < 0) return -1; @@ -289,29 +375,38 @@ static int qemudListen(struct qemud_serv int uid; if ((uid = geteuid()) < 0) { + qemudLog(QEMUD_ERR, "You must run the daemon as root to use system mode"); return -1; } - if (!(pw = getpwuid(uid))) - return -1; - - if (snprintf(sockname, sizeof(sockname), "@%s/.libvirt/qemud-sock", pw->pw_dir) >= (int)sizeof(sockname)) { + if (!(pw = getpwuid(uid))) { + qemudLog(QEMUD_ERR, "Failed to find user record for uid '%s': %s", + uid, strerror(errno)); return -1; } + if (snprintf(sockname, sizeof(sockname), "@%s/.libvirt/qemud-sock", pw->pw_dir) >= (int)sizeof(sockname)) + goto snprintf_error; + if (qemudListenUnix(server, sockname, 0) < 0) return -1; } return 0; + + snprintf_error: + qemudLog(QEMUD_ERR, "Resulting path to long for buffer in qemudListen()"); + return -1; } static struct qemud_server *qemudInitialize(int sys, int sigread) { struct qemud_server *server; char libvirtConf[PATH_MAX]; - if (!(server = calloc(1, sizeof(struct qemud_server)))) + if (!(server = calloc(1, sizeof(struct qemud_server)))) { + qemudLog(QEMUD_ERR, "Failed to allocate struct qemud_server"); return NULL; + } /* XXX extract actual version */ server->qemuVersion = (0*1000000)+(8*1000)+(0); @@ -321,47 +416,54 @@ static struct qemud_server *qemudInitial if (sys) { if (snprintf(libvirtConf, sizeof(libvirtConf), "%s/libvirt", SYSCONF_DIR) >= (int)sizeof(libvirtConf)) { - goto cleanup; + goto snprintf_cleanup; } if (mkdir(libvirtConf, 0777) < 0) { if (errno != EEXIST) { + qemudLog(QEMUD_ERR, "Failed to create directory '%s': %s", + libvirtConf, strerror(errno)); goto cleanup; } } if (snprintf(server->configDir, sizeof(server->configDir), "%s/libvirt/qemu", SYSCONF_DIR) >= (int)sizeof(server->configDir)) { - goto cleanup; + goto snprintf_cleanup; } if (snprintf(server->networkConfigDir, sizeof(server->networkConfigDir), "%s/libvirt/qemu/networks", SYSCONF_DIR) >= (int)sizeof(server->networkConfigDir)) { - goto cleanup; + goto snprintf_cleanup; } } else { struct passwd *pw; int uid; if ((uid = geteuid()) < 0) { + qemudLog(QEMUD_ERR, "You must run the daemon as root to use system mode"); goto cleanup; } if (!(pw = getpwuid(uid))) { + qemudLog(QEMUD_ERR, "Failed to find user record for uid '%s': %s", + uid, strerror(errno)); goto cleanup; } if (snprintf(libvirtConf, sizeof(libvirtConf), "%s/.libvirt", pw->pw_dir) >= (int)sizeof(libvirtConf)) { - goto cleanup; + goto snprintf_cleanup; } if (mkdir(libvirtConf, 0777) < 0) { if (errno != EEXIST) { + qemudLog(QEMUD_ERR, "Failed to create directory '%s': %s", + libvirtConf, strerror(errno)); goto cleanup; } } if (snprintf(server->configDir, sizeof(server->configDir), "%s/.libvirt/qemu", pw->pw_dir) >= (int)sizeof(server->configDir)) { - goto cleanup; + goto snprintf_cleanup; } if (snprintf(server->networkConfigDir, sizeof(server->networkConfigDir), "%s/.libvirt/qemu/networks", pw->pw_dir) >= (int)sizeof(server->networkConfigDir)) { - goto cleanup; + goto snprintf_cleanup; } } @@ -376,6 +478,9 @@ static struct qemud_server *qemudInitial return server; + snprintf_cleanup: + qemudLog(QEMUD_ERR, "Resulting path to long for buffer in qemudInitialize()"); + cleanup: if (server) { struct qemud_socket *sock = server->sockets; @@ -399,15 +504,12 @@ static int qemudDispatchServer(struct qe if ((fd = accept(sock->fd, (struct sockaddr *)&addr, &addrlen)) < 0) { if (errno == EAGAIN) return 0; + qemudLog(QEMUD_ERR, "Failed to accept connection: %s", strerror(errno)); return -1; } - if (qemudSetCloseExec(fd) < 0) { - close(fd); - return -1; - } - - if (qemudSetNonBlock(fd) < 0) { + if (qemudSetCloseExec(fd) < 0 || + qemudSetNonBlock(fd) < 0) { close(fd); return -1; } @@ -585,12 +687,12 @@ static int qemudClientRead(struct qemud_ char *buf, size_t want) { int ret; if ((ret = read(client->fd, buf, want)) <= 0) { - QEMUD_DEBUG("Plain read error %d\n", ret); + qemudDebug("Plain read error %d", ret); if (!ret || errno != EAGAIN) qemudDispatchClientFailure(server, client); return -1; } - QEMUD_DEBUG("Plain data read %d\n", ret); + qemudDebug("Plain data read %d", ret); return ret; } @@ -615,17 +717,17 @@ static void qemudDispatchClientRead(stru /* If we've finished header, move onto body */ if (client->incomingReceived == sizeof(struct qemud_packet_header)) { - QEMUD_DEBUG("Type %d, data %d\n", + qemudDebug("Type %d, data %d", client->incoming.header.type, client->incoming.header.dataSize); /* Client lied about dataSize */ if (client->incoming.header.dataSize > sizeof(union qemud_packet_data)) { - QEMUD_DEBUG("Bogus data size %u\n", client->incoming.header.dataSize); + qemudDebug("Bogus data size %u", client->incoming.header.dataSize); qemudDispatchClientFailure(server, client); return; } if (client->incoming.header.dataSize) { - QEMUD_DEBUG("- Restarting recv to process body (%d bytes)\n", + qemudDebug("- Restarting recv to process body (%d bytes)", client->incoming.header.dataSize); goto restart; } @@ -635,7 +737,7 @@ static void qemudDispatchClientRead(stru if (ret == want) { if (qemudDispatchClientRequest(server, client) < 0) qemudDispatchClientFailure(server, client); - QEMUD_DEBUG("Dispatch\n"); + qemudDebug("Dispatch"); } } @@ -645,12 +747,12 @@ static int qemudClientWrite(struct qemud char *buf, size_t want) { int ret; if ((ret = write(client->fd, buf, want)) < 0) { - QEMUD_DEBUG("Plain write error %d\n", ret); + qemudDebug("Plain write error %d", ret); if (errno != EAGAIN) qemudDispatchClientFailure(server, client); return -1; } - QEMUD_DEBUG("Plain data write %d\n", ret); + qemudDebug("Plain data write %d", ret); return ret; } @@ -664,7 +766,7 @@ static void qemudDispatchClientWrite(str return; } client->outgoingSent += ret; - QEMUD_DEBUG("Done %d %d\n", todo, ret); + qemudDebug("Done %d %d", todo, ret); if (todo == ret) client->tx = 0; } @@ -726,12 +828,12 @@ static int qemudVMData(struct qemud_serv close(monfd); return -1; } - QEMUD_DEBUG("[%s]\n", line); + qemudDebug("[%s]", line); } vm->monitor = monfd; } } - QEMUD_DEBUG("[%s]\n", buf); + qemudDebug("[%s]", buf); } } @@ -746,6 +848,8 @@ int qemudShutdownVMDaemon(struct qemud_s struct qemud_vm *prev = NULL, *curr = server->activevms; struct qemud_vm_net_def *net; + qemudLog(QEMUD_INFO, "Shutting down VM '%s'", vm->def->name); + /* Already cleaned-up */ if (vm->pid < 0) return 0; @@ -772,7 +876,7 @@ int qemudShutdownVMDaemon(struct qemud_s } if (!curr) { - QEMUD_DEBUG("Could not find VM to shutdown\n"); + qemudDebug("Could not find VM to shutdown"); return 0; } @@ -797,7 +901,7 @@ int qemudShutdownVMDaemon(struct qemud_s if (waitpid(vm->pid, NULL, WNOHANG) != vm->pid) { kill(vm->pid, SIGKILL); if (waitpid(vm->pid, NULL, 0) != vm->pid) { - QEMUD_DEBUG("Got unexpected pid, damn\n"); + qemudLog(QEMUD_WARN, "Got unexpected pid, damn"); } } @@ -1129,14 +1233,14 @@ int qemudStartNetworkDaemon(struct qemud err_delbr1: if (network->def->ipAddress[0] && (err = brSetInterfaceUp(server->brctl, network->bridge, 0))) { - printf("Damn! Failed to bring down bridge '%s' : %s\n", - network->bridge, strerror(err)); + qemudLog(QEMUD_WARN, "Failed to bring down bridge '%s' : %s", + network->bridge, strerror(err)); } err_delbr: if ((err = brDeleteBridge(server->brctl, network->bridge))) { - printf("Damn! Couldn't delete bridge '%s' : %s\n", - network->bridge, strerror(err)); + qemudLog(QEMUD_WARN, "Failed to delete bridge '%s' : %s\n", + network->bridge, strerror(err)); } return -1; @@ -1148,6 +1252,8 @@ int qemudShutdownNetworkDaemon(struct qe struct qemud_network *prev, *curr; int err; + qemudLog(QEMUD_INFO, "Shutting down network '%s'", network->def->name); + if (!network->active) return 0; @@ -1158,13 +1264,13 @@ int qemudShutdownNetworkDaemon(struct qe if (network->def->ipAddress[0] && (err = brSetInterfaceUp(server->brctl, network->bridge, 0))) { - printf("Damn! Failed to bring down bridge '%s' : %s\n", - network->bridge, strerror(err)); + qemudLog(QEMUD_WARN, "Failed to bring down bridge '%s' : %s\n", + network->bridge, strerror(err)); } if ((err = brDeleteBridge(server->brctl, network->bridge))) { - printf("Damn! Failed to delete bridge '%s' : %s\n", - network->bridge, strerror(err)); + qemudLog(QEMUD_WARN, "Failed to delete bridge '%s' : %s\n", + network->bridge, strerror(err)); } /* Move it to inactive networks list */ @@ -1192,7 +1298,7 @@ int qemudShutdownNetworkDaemon(struct qe waitpid(network->dnsmasqPid, NULL, WNOHANG) != network->dnsmasqPid) { kill(network->dnsmasqPid, SIGKILL); if (waitpid(network->dnsmasqPid, NULL, 0) != network->dnsmasqPid) - printf("Got unexpected pid for dnsmasq, damn\n"); + qemudLog(QEMUD_WARN, "Got unexpected pid for dnsmasq\n"); } network->bridge[0] = '\0'; @@ -1226,6 +1332,7 @@ static int qemudDispatchPoll(struct qemu while (sock) { struct qemud_socket *next = sock->next; + /* FIXME: the daemon shouldn't exit on error here */ if (fds[fd].revents) if (qemudDispatchServer(server, sock) < 0) return -1; @@ -1236,7 +1343,7 @@ static int qemudDispatchPoll(struct qemu while (client) { struct qemud_client *next = client->next; if (fds[fd].revents) { - QEMUD_DEBUG("Poll data normal\n"); + qemudDebug("Poll data normal"); if (fds[fd].revents == POLLOUT) qemudDispatchClientWrite(server, client); else if (fds[fd].revents == POLLIN) @@ -1281,7 +1388,7 @@ static int qemudDispatchPoll(struct qemu } vm = next; if (failed) - ret = -1; + ret = -1; /* FIXME: the daemon shouldn't exit on failure here */ } /* Cleanup any VMs which shutdown & dont have an associated @@ -1389,12 +1496,16 @@ static int qemudOneLoop(struct qemud_ser if (errno == EINTR) { goto retry; } + qemudLog(QEMUD_ERR, "Error polling on file descriptors: %s", + strerror(errno)); return -1; } /* Must have timed out */ - if (ret == 0) + if (ret == 0) { + qemudLog(QEMUD_INFO, "Timed out while polling on file descriptors"); return -1; + } if (qemudDispatchPoll(server, fds) < 0) return -1; @@ -1427,8 +1538,6 @@ static void qemudCleanup(struct qemud_se #define MAX_LISTEN 5 int main(int argc, char **argv) { - int godaemon = 0; - int verbose = 0; int sys = 0; int timeout = -1; struct qemud_server *server; @@ -1484,10 +1593,16 @@ int main(int argc, char **argv) { } } + if (godaemon) + openlog("libvirt-qemud", 0, 0); + if (pipe(sigpipe) < 0 || qemudSetNonBlock(sigpipe[0]) < 0 || - qemudSetNonBlock(sigpipe[1]) < 0) + qemudSetNonBlock(sigpipe[1]) < 0) { + qemudLog(QEMUD_ERR, "Failed to create pipe: %s", + strerror(errno)); return 1; + } sigwrite = sigpipe[1]; @@ -1505,8 +1620,11 @@ int main(int argc, char **argv) { if (godaemon) { int pid = qemudGoDaemon(); - if (pid < 0) + if (pid < 0) { + qemudLog(QEMUD_ERR, "Failed to fork as daemon: %s", + strerror(errno)); return 1; + } if (pid > 0) return 0; } @@ -1520,6 +1638,9 @@ int main(int argc, char **argv) { close(sigwrite); + if (godaemon) + closelog(); + return 0; } Index: libvirt/qemud/dispatch.c =================================================================== --- libvirt.orig/qemud/dispatch.c +++ libvirt/qemud/dispatch.c @@ -812,7 +812,7 @@ int qemudDispatch(struct qemud_server *s struct qemud_packet *in, struct qemud_packet *out) { clientFunc *funcs; unsigned int type = in->header.type; - QEMUD_DEBUG("> Dispatching request %d readonly ? %d\n", type, client->readonly); + qemudDebug("> Dispatching request %d readonly ? %d", type, client->readonly); server->errorCode = 0; server->errorMessage[0] = '\0'; @@ -820,12 +820,12 @@ int qemudDispatch(struct qemud_server *s memset(out, 0, sizeof(struct qemud_packet)); if (type >= QEMUD_PKT_MAX) { - QEMUD_DEBUG("Illegal request type\n"); + qemudDebug("Illegal request type"); return -1; } if (type == QEMUD_PKT_FAILURE) { - QEMUD_DEBUG("Illegal request type\n"); + qemudDebug("Illegal request type"); return -1; } @@ -835,17 +835,17 @@ int qemudDispatch(struct qemud_server *s funcs = funcsTransmitRW; if (!funcs[type]) { - QEMUD_DEBUG("Illegal operation requested\n"); + qemudDebug("Illegal operation requested"); qemudReportError(server, VIR_ERR_OPERATION_DENIED, NULL); qemudDispatchFailure(server, client, out); } else { if ((funcs[type])(server, client, in, out) < 0) { - QEMUD_DEBUG("Dispatch failed\n"); + qemudDebug("Dispatch failed"); return -1; } } - QEMUD_DEBUG("< Returning reply %d (%d bytes)\n", + qemudDebug("< Returning reply %d (%d bytes)", out->header.type, out->header.dataSize); Index: libvirt/qemud/internal.h =================================================================== --- libvirt.orig/qemud/internal.h +++ libvirt/qemud/internal.h @@ -44,14 +44,17 @@ #define ATTRIBUTE_UNUSED #endif -#ifdef DEBUG -#define QEMUD_DEBUG(args...) fprintf(stderr, args) -#else -#define QEMUD_DEBUG(args...) do {} while(0) -#endif - #define UUID_LEN 16 +typedef enum { + QEMUD_ERR, + QEMUD_WARN, + QEMUD_INFO, +#ifdef ENABLE_DEBUG + QEMUD_DEBUG +#endif +} qemudLogPriority; + /* Different types of QEMU acceleration possible */ enum qemud_vm_virt_type { QEMUD_VIRT_QEMU, @@ -307,6 +310,13 @@ int qemudStartNetworkDaemon(struct qemud int qemudShutdownNetworkDaemon(struct qemud_server *server, struct qemud_network *network); +void qemudLog(int priority, const char *fmt, ...); + +#ifdef ENABLE_DEBUG +#define qemudDebug(...) qemudLog(QEMUD_DEBUG, __VA_ARGS__) +#else +#define qemudDebug(fmt, ...) do { } while(0); +#endif #endif Index: libvirt/configure.in =================================================================== --- libvirt.orig/configure.in +++ libvirt/configure.in @@ -78,6 +78,14 @@ dnl CFLAGS="-g -O -W -Wformat -Wunused -Wimplicit -Wreturn-type -Wswitch -Wcomment -Wtrigraphs -Wformat -Wchar-subscripts -Wuninitialized -Wparentheses -Wshadow -Wpointer-arith -Wcast-align -Wwrite-strings -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Winline -Wredundant-decls -Wall" fi +dnl --enable-debug=(yes|no) +AC_ARG_ENABLE(debug, + AC_HELP_STRING([--enable-debug=no/yes], + [enable debugging output])) +if test x"$enable_debug" = x"yes"; then + AC_DEFINE(ENABLE_DEBUG, [], [whether debugging is enabled]) +fi + dnl dnl allow the creation of iptables rules in chains with a dnl specific prefix rather than in the standard toplevel chains Index: libvirt/qemud/conf.c =================================================================== --- libvirt.orig/qemud/conf.c +++ libvirt/qemud/conf.c @@ -1644,6 +1644,8 @@ int qemudScanConfigDir(struct qemud_serv if (!(dir = opendir(configDir))) { if (errno == ENOENT) return 0; + qemudLog(QEMUD_ERR, "Failed to open dir '%s': %s", + configDir, strerror(errno)); return -1; } Index: libvirt/qemud/driver.c =================================================================== --- libvirt.orig/qemud/driver.c +++ libvirt/qemud/driver.c @@ -90,7 +90,7 @@ int qemudMonitorCommand(struct qemud_ser size += got; } if (buf) - QEMUD_DEBUG("Mon [%s]\n", buf); + qemudDebug("Mon [%s]", buf); /* Look for QEMU prompt to indicate completion */ if (buf && ((tmp = strstr(buf, "\n(qemu)")) != NULL)) { tmp[0] = '\0'; @@ -203,7 +203,7 @@ static int qemudGetProcessInfo(unsigned } if (fscanf(pidinfo, "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %lu %lu", &usertime, &systime) != 2) { - QEMUD_DEBUG("not enough arg\n"); + qemudDebug("not enough arg"); return -1; } @@ -214,7 +214,7 @@ static int qemudGetProcessInfo(unsigned */ *cpuTime = 1000 * 1000 * 1000 * (usertime + systime) / sysconf(_SC_CLK_TCK); - QEMUD_DEBUG("Got %lu %lu %lld\n", usertime, systime, *cpuTime); + qemudDebug("Got %lu %lu %lld", usertime, systime, *cpuTime); fclose(pidinfo); @@ -322,7 +322,7 @@ int qemudDomainSuspend(struct qemud_serv qemudReportError(server, VIR_ERR_OPERATION_FAILED, "suspend operation failed"); return -1; } - printf("Reply %s\n", info); + qemudDebug("Reply %s", info); free(info); return 0; } @@ -343,7 +343,7 @@ int qemudDomainResume(struct qemud_serve qemudReportError(server, VIR_ERR_OPERATION_FAILED, "resume operation failed"); return -1; } - printf("Reply %s\n", info); + qemudDebug("Reply %s", info); free(info); return -1; } --
On Fri, Feb 16, 2007 at 02:44:57PM +0000, Mark McLoughlin wrote:
Add a qemudLog() function which uses syslog() if we're in daemon mode, doesn't output INFO/DEBUG messages unless the verbose flag is set and doesn't output DEBUG messages unless compiled with --enable-debug.
The QEMU daemon can run both as root, or non-root. Using syslog in the non-root case is not particularly helpful because the user will have no way to retrieve the log messages. We should have the daemon log to a file under $HOME/.libvirt somewhere, and only use syslog if explicitly turned on via a command line arg to the daemon. I think there's scope for making this more generally useful too, rather than making it just part of the QEMU daemon. The general libvirtd will definitely need it, and I think once we have remote managemnet it'll be important to have a way to do logging in the client library which integrates into apps using libvirt. eg let the app register a callback to receive debug messages. The same core functions The patch itself looks reasonable, but next time there's a patch of this scale please post this stuff for review before committing
if (sys) { - if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) { - return -1; - } + if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) + goto snprintf_error; +
I've just realized the return value checks I did here are incomplete. As well as checking for overflow with >=, we also need to check for general errors which are indicated by a -ve return value.
+ if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock-ro", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) + goto snprintf_error;
- if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock-ro", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) { - return -1; - }
Likewise. Dan
unlink(sockname); if (qemudListenUnix(server, sockname, 1) < 0) return -1; @@ -289,29 +375,38 @@ static int qemudListen(struct qemud_serv int uid;
if ((uid = geteuid()) < 0) { + qemudLog(QEMUD_ERR, "You must run the daemon as root to use system mode"); return -1; }
This error message looks bogus. This codepath is the per-user unprivileged session mode, and the error. And actually checking return value at all is completely unneccesssary because geteuid() is guarenteed not to fail. We do, however, need a check if (geteuid() != 0) { .... } In the first half of the if() block in the qemudListen function to check for root privs in system mode.
if ((uid = geteuid()) < 0) { + qemudLog(QEMUD_ERR, "You must run the daemon as root to use system mode"); goto cleanup; }
Again, same as above.
@@ -1226,6 +1332,7 @@ static int qemudDispatchPoll(struct qemu
while (sock) { struct qemud_socket *next = sock->next; + /* FIXME: the daemon shouldn't exit on error here */ if (fds[fd].revents) if (qemudDispatchServer(server, sock) < 0) return -1;
Yes & no. There are two reasons why qemuDispatchServer can fail. Either it can fail to set CLOSEXEC/NONBLOCK mode on the client socket, in which case we could simply drop the client & continue without exiting. If the accept() call fails for anything other than EAGAIN/EINTR then we arguably should exit, because something serious has gone wrong.
if (fds[fd].revents) { - QEMUD_DEBUG("Poll data normal\n"); + qemudDebug("Poll data normal"); if (fds[fd].revents == POLLOUT) qemudDispatchClientWrite(server, client); else if (fds[fd].revents == POLLIN) @@ -1281,7 +1388,7 @@ static int qemudDispatchPoll(struct qemu } vm = next; if (failed) - ret = -1; + ret = -1; /* FIXME: the daemon shouldn't exit on failure here */ }
The failed flag should only get set if something serious went wrong when trying to cleanup after a dead VM. If cleanup succeeds we stick around, but if it fails I'm not entirely convinced we shouldn't exit. 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 -=|
Hey, On Fri, 2007-02-16 at 19:28 +0000, Daniel P. Berrange wrote:
On Fri, Feb 16, 2007 at 02:44:57PM +0000, Mark McLoughlin wrote:
@@ -289,29 +375,38 @@ static int qemudListen(struct qemud_serv int uid;
if ((uid = geteuid()) < 0) { + qemudLog(QEMUD_ERR, "You must run the daemon as root to use system mode"); return -1; }
This error message looks bogus. This codepath is the per-user unprivileged session mode, and the error. And actually checking return value at all is completely unneccesssary because geteuid() is guarenteed not to fail.
We do, however, need a check
if (geteuid() != 0) { .... }
In the first half of the if() block in the qemudListen function to check for root privs in system mode.
Okay, here's an attempt to clean up all this code - mostly to coalesce the two system vs. user code paths into a single function, but also to recursively create config dirs and fix up the problems you spotted. Cheers, Mark.
On Mon, 2007-02-19 at 18:00 +0000, Mark McLoughlin wrote:
Okay, here's an attempt to clean up all this code - mostly to coalesce the two system vs. user code paths into a single function, but also to recursively create config dirs and fix up the problems you spotted.
I went ahead and committed that assuming no responses meant no-one had a problem with it. Here's a further patch to make us only attempt to create config dirs when configs are being saved. Cheers, Mark.
On Tue, Feb 20, 2007 at 03:08:13PM +0000, Mark McLoughlin wrote:
On Mon, 2007-02-19 at 18:00 +0000, Mark McLoughlin wrote:
Okay, here's an attempt to clean up all this code - mostly to coalesce the two system vs. user code paths into a single function, but also to recursively create config dirs and fix up the problems you spotted.
I went ahead and committed that assuming no responses meant no-one had a problem with it.
Yep, forgot to reply :-)
Here's a further patch to make us only attempt to create config dirs when configs are being saved.
I'm in two minds about this. On the one hand if your starting the system QEMU daemon from an init script, creating the config dirs on startup gives admin immediate feedback of any problems. On the other hand, the per-user instance is auto-started so they'd never see a failure message during startup. I guess if we can reliably feedback errors to the user about failing to create the config dirs, this patch is a plus. 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 Fri, 2007-02-16 at 19:28 +0000, Daniel P. Berrange wrote:
On Fri, Feb 16, 2007 at 02:44:57PM +0000, Mark McLoughlin wrote:
@@ -1226,6 +1332,7 @@ static int qemudDispatchPoll(struct qemu
while (sock) { struct qemud_socket *next = sock->next; + /* FIXME: the daemon shouldn't exit on error here */ if (fds[fd].revents) if (qemudDispatchServer(server, sock) < 0) return -1;
Yes & no. There are two reasons why qemuDispatchServer can fail. Either it can fail to set CLOSEXEC/NONBLOCK mode on the client socket, in which case we could simply drop the client & continue without exiting. If the accept() call fails for anything other than EAGAIN/EINTR then we arguably should exit, because something serious has gone wrong.
Doesn't accept() fail if the client fails to send the final ACK? Do we want the daemon to die in that case? Think of an unprivileged user connecting to the system daemon's readonly socket ... you really want to be paranoid about the daemon exiting as it creates the opportunity for unprivileged users to take down guests and networks. i.e. I'm not sure whether it would be actually possible to exploit it in this way, but I'd tend to be pretty paranoid about any exit point from the daemon. Cheers, Mark.
On Mon, Feb 19, 2007 at 06:13:27PM +0000, Mark McLoughlin wrote:
On Fri, 2007-02-16 at 19:28 +0000, Daniel P. Berrange wrote:
On Fri, Feb 16, 2007 at 02:44:57PM +0000, Mark McLoughlin wrote:
@@ -1226,6 +1332,7 @@ static int qemudDispatchPoll(struct qemu
while (sock) { struct qemud_socket *next = sock->next; + /* FIXME: the daemon shouldn't exit on error here */ if (fds[fd].revents) if (qemudDispatchServer(server, sock) < 0) return -1;
Yes & no. There are two reasons why qemuDispatchServer can fail. Either it can fail to set CLOSEXEC/NONBLOCK mode on the client socket, in which case we could simply drop the client & continue without exiting. If the accept() call fails for anything other than EAGAIN/EINTR then we arguably should exit, because something serious has gone wrong.
Doesn't accept() fail if the client fails to send the final ACK? Do we want the daemon to die in that case? Think of an unprivileged user connecting to the system daemon's readonly socket ... you really want to be paranoid about the daemon exiting as it creates the opportunity for unprivileged users to take down guests and networks.
Yes, accept will fail if the client doesn't complete the handshake. I already (tried) to take care of that case though by not returning an error code if the errno is EAGAIN if ((fd = accept(sock->fd, (struct sockaddr *)&addr, &addrlen)) < 0) { if (errno == EAGAIN) return 0; qemudLog(QEMUD_ERR, "Failed to accept connection: %s", strerror(errno)); return -1; } Looking at the man page, i think I also need to check for ECONNABORTED errno value too. 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 -=|
oneMark McLoughlin wrote:
Add a qemudLog() function which uses syslog() if we're in daemon mode, doesn't output INFO/DEBUG messages unless the verbose flag is set and doesn't output DEBUG messages unless compiled with --enable-debug.
You're all gonna hate this I know, but libvirtd handles syslog by forking an external logger(1) process. Messages sent to stderr go to syslog. This is partly necessary because the SunRPC code within glibc is a bit too happy to send debug messages to stderr & nowhere else. The brief code snippet to enable this is below (and note my original patch which you can also find on this list enables a test for LOGGER in autoconf): #ifdef LOGGER /* Send stderr to syslog using logger. It's a lot simpler * to do this. Note that SunRPC in glibc prints lots of * gumf to stderr and it'd be a load of work to change that. */ int fd[2]; if (pipe (fd) == -1) { perror ("pipe"); exit (2); } int pid = fork (); if (pid == -1) { perror ("fork"); exit (2); } if (pid == 0) { /* Child - logger. */ const char *args[] = { "logger", "-tlibvirtd", "-p", "daemon.notice", NULL }; close (fd[1]); dup2 (fd[0], 0); close (fd[0]); execv (LOGGER, (char *const *) args); perror ("execv"); _exit (1); } close (fd[0]); dup2 (fd[1], 2); close (fd[1]); #endif Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
On Sat, Feb 17, 2007 at 01:11:48PM +0000, Richard W.M. Jones wrote:
oneMark McLoughlin wrote:
Add a qemudLog() function which uses syslog() if we're in daemon mode, doesn't output INFO/DEBUG messages unless the verbose flag is set and doesn't output DEBUG messages unless compiled with --enable-debug.
You're all gonna hate this I know, but libvirtd handles syslog by forking an external logger(1) process. Messages sent to stderr go to syslog. This is partly necessary because the SunRPC code within glibc is a bit too happy to send debug messages to stderr & nowhere else.
Is this just wrt to the server side of SunRPC, or does it apply to the client side too ? If using libvirt from command line tools it won't be nice if SunRPC is spewing crap to STDERR.
#ifdef LOGGER /* Send stderr to syslog using logger. It's a lot simpler * to do this. Note that SunRPC in glibc prints lots of * gumf to stderr and it'd be a load of work to change that. */ int fd[2]; if (pipe (fd) == -1) { perror ("pipe"); exit (2); } int pid = fork (); if (pid == -1) { perror ("fork"); exit (2); } if (pid == 0) { /* Child - logger. */ const char *args[] = { "logger", "-tlibvirtd", "-p", "daemon.notice", NULL }; close (fd[1]); dup2 (fd[0], 0); close (fd[0]); execv (LOGGER, (char *const *) args); perror ("execv"); _exit (1); } close (fd[0]); dup2 (fd[1], 2); close (fd[1]); #endif
BTW, need to make sure all file descriptors are either explicitly closed, or have close-on-exec set 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 -=|
Daniel P. Berrange wrote:
On Sat, Feb 17, 2007 at 01:11:48PM +0000, Richard W.M. Jones wrote:
oneMark McLoughlin wrote:
Add a qemudLog() function which uses syslog() if we're in daemon mode, doesn't output INFO/DEBUG messages unless the verbose flag is set and doesn't output DEBUG messages unless compiled with --enable-debug. You're all gonna hate this I know, but libvirtd handles syslog by forking an external logger(1) process. Messages sent to stderr go to syslog. This is partly necessary because the SunRPC code within glibc is a bit too happy to send debug messages to stderr & nowhere else.
Is this just wrt to the server side of SunRPC, or does it apply to the client side too ? If using libvirt from command line tools it won't be nice if SunRPC is spewing crap to STDERR.
The external logger only applies server-side. On the client side I am very careful to turn all RPC (and other) errors into virterror errors. However it may still be the case that SunRPC code prints additional messages to stderr, although no relevant debug information should be lost (all pertinent information related to an RPC will make it into the virterror mechanism). [The complicated bit, unrelated to this, is virterrors generated on the server side. What the latest version does is to serialise these errors and pass them to the client side, where they are converted into virterror errors on the client side. Thus things like user error handling should work transparently over the remote connection.]
#ifdef LOGGER /* Send stderr to syslog using logger. It's a lot simpler * to do this. Note that SunRPC in glibc prints lots of * gumf to stderr and it'd be a load of work to change that. */ int fd[2]; if (pipe (fd) == -1) { perror ("pipe"); exit (2); } int pid = fork (); if (pid == -1) { perror ("fork"); exit (2); } if (pid == 0) { /* Child - logger. */ const char *args[] = { "logger", "-tlibvirtd", "-p", "daemon.notice", NULL }; close (fd[1]); dup2 (fd[0], 0); close (fd[0]); execv (LOGGER, (char *const *) args); perror ("execv"); _exit (1); } close (fd[0]); dup2 (fd[1], 2); close (fd[1]); #endif
BTW, need to make sure all file descriptors are either explicitly closed, or have close-on-exec set
Do you mean fds > 2 which might be passed across the fork in the code above? Or the new stderr-to-pipe in the parent process? Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
On Fri, Feb 16, 2007 at 02:43:42PM +0000, Mark McLoughlin wrote:
Hey, Okay, here's another batch of qemud patches I've committed.
Usual practice is to post patches for review *before* committing them, unless they're trivial bug fixes. I don't recall us ever deciding to change that policy. Can we please stick with that model.
The most significant ones are the signal handling and error reporting ones at the end. Appreciate someone looking over them.
Will do. 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 Fri, 2007-02-16 at 18:56 +0000, Daniel P. Berrange wrote:
On Fri, Feb 16, 2007 at 02:43:42PM +0000, Mark McLoughlin wrote:
Hey, Okay, here's another batch of qemud patches I've committed.
Usual practice is to post patches for review *before* committing them, unless they're trivial bug fixes. I don't recall us ever deciding to change that policy. Can we please stick with that model.
Point taken, apologies. By way of explanation, I guess I just got a bit carried away by the rush earlier for the 0.2.0 release and my network access being very sketchy all week. Thanks, Mark.
participants (4)
-
Daniel P. Berrange -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones