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