[libvirt] [PATCH 5/5] read saved vm status on libvirtd startup
The earlier patches didn't change libvirtd behaviour (except for kvm/qemu not being teared down on an unexpected daemon crash), all vms were still being shutoff at daemon shutdown. This patch now adds the code to read back the vm status after on daemon startup and reconnects all running vms found in stateDir which are left over from a daemon restart or crash and also disables shutting down of VMs when libvirt quits. --- src/qemu_driver.c | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 158 insertions(+), 7 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d8b87e4..57a396c 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -183,6 +183,46 @@ qemudAutostartConfigs(struct qemud_driver *driver) { } +static int +qemudGetProcFD(pid_t pid, int fdnum) +{ + int fd; + +#ifdef __linux__ + char *path = NULL; + + if (!asprintf(&path, "/proc/%d/fd/%d", pid, fdnum)) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + return -1; + } + + if((fd = open(path, O_RDONLY)) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Unable to open %s"), path); + return -1; + } + if (qemudSetCloseExec(fd) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Unable to set close-on-exec flag on %s"), path); + goto error; + } + + if (qemudSetNonBlock(fd) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Unable to put %s into non-blocking mode"), path); + goto error; + } + + VIR_FREE(path); + return fd; +error: + VIR_FREE(path); + close(fd); +#endif + return -1; +} + + /** * qemudRemoveDomainStatus * @@ -220,6 +260,104 @@ cleanup: } +static int qemudOpenMonitor(virConnectPtr conn, + virDomainObjPtr vm, + const char *monitor, + int reconnect); + +/** + * qemudReconnectVMs + * + * Reconnect running vms to the daemon process + */ +static int +qemudReconnectVMs(struct qemud_driver *driver) +{ + int i; + + for (i = 0 ; i < qemu_driver->domains.count ; i++) { + virDomainObjPtr vm = qemu_driver->domains.objs[i]; + qemudDomainStatusPtr status = NULL; + char *config = NULL; + int rc; + + virDomainObjLock(vm); + /* Read pid */ + if ((rc = virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) == 0) + DEBUG("Found pid %d for '%s'", vm->pid, vm->def->name); + else + goto next; + + if ((config = virDomainConfigFile(NULL, + driver->stateDir, + vm->def->name)) == NULL) { + qemudLog(QEMUD_ERR, _("Failed to read domain status for %s\n"), + vm->def->name); + goto next_error; + } + + status = qemudDomainStatusParseFile(NULL, driver->caps, config, 0); + if (status) { + vm->newDef = vm->def; + vm->def = status->def; + } else { + qemudLog(QEMUD_ERR, _("Failed to parse domain status for %s\n"), + vm->def->name); + goto next_error; + } + + if ((rc = qemudOpenMonitor(NULL, vm, status->monitorpath, 1)) != 0) { + qemudLog(QEMUD_ERR, _("Failed to reconnect monitor for %s: %d\n"), + vm->def->name, rc); + goto next_error; + } else + vm->monitorpath = status->monitorpath; + + vm->stdin_fd = qemudGetProcFD(vm->pid, 0); + vm->stdout_fd = qemudGetProcFD(vm->pid, 1); + vm->stderr_fd = qemudGetProcFD(vm->pid, 2); + if (vm->stdin_fd == -1 || vm->stdout_fd == -1 || vm->stderr_fd == -1) + goto next_error; + + if (((vm->stdout_watch = virEventAddHandle(vm->stdout_fd, + VIR_EVENT_HANDLE_READABLE | + VIR_EVENT_HANDLE_ERROR | + VIR_EVENT_HANDLE_HANGUP, + qemudDispatchVMEvent, + driver, NULL)) < 0) || + ((vm->stderr_watch = virEventAddHandle(vm->stderr_fd, + VIR_EVENT_HANDLE_READABLE | + VIR_EVENT_HANDLE_ERROR | + VIR_EVENT_HANDLE_HANGUP, + qemudDispatchVMEvent, + driver, NULL)) < 0)) { + + qemudLog(QEMUD_ERR, _("Failed to reconnect to stdout/stderr\n")); + goto next_error; + } + + if (vm->def->id >= driver->nextvmid) + driver->nextvmid = vm->def->id + 1; + + vm->state = status->state; + goto next; + +next_error: + /* we failed to reconnect the vm so remove it's traces */ + vm->def->id = -1; + qemudRemoveDomainStatus(NULL, driver, vm); + virDomainDefFree(vm->def); + vm->def = vm->newDef; + vm->newDef = NULL; +next: + virDomainObjUnlock(vm); + VIR_FREE(status); + VIR_FREE(config); + } + return 0; +} + + /** * qemudStartup: * @@ -316,6 +454,7 @@ qemudStartup(void) { qemu_driver->autostartDir, NULL, NULL) < 0) goto error; + qemudReconnectVMs(qemu_driver); qemudAutostartConfigs(qemu_driver); qemuDriverUnlock(qemu_driver); @@ -418,11 +557,14 @@ qemudShutdown(void) { /* shutdown active VMs */ for (i = 0 ; i < qemu_driver->domains.count ; i++) { + /* FIXME: don't shutdown VMs on daemon shutdown for now */ +#if 0 virDomainObjPtr dom = qemu_driver->domains.objs[i]; virDomainObjLock(dom); if (virDomainIsActive(dom)) qemudShutdownVMDaemon(NULL, qemu_driver, dom); virDomainObjUnlock(dom); +#endif } virDomainObjListFree(&qemu_driver->domains); @@ -543,7 +685,8 @@ qemudCheckMonitorPrompt(virConnectPtr conn ATTRIBUTE_UNUSED, static int qemudOpenMonitor(virConnectPtr conn, virDomainObjPtr vm, - const char *monitor) { + const char *monitor, + int reconnect) { int monfd; char buf[1024]; int ret = -1; @@ -564,11 +707,19 @@ static int qemudOpenMonitor(virConnectPtr conn, goto error; } - ret = qemudReadMonitorOutput(conn, - vm, monfd, - buf, sizeof(buf), - qemudCheckMonitorPrompt, - "monitor", 10000); + if (!reconnect) { + ret = qemudReadMonitorOutput(conn, + vm, monfd, + buf, sizeof(buf), + qemudCheckMonitorPrompt, + "monitor", 10000); + } else { + vm->monitor = monfd; + ret = 0; + } + + if (ret != 0) + goto error; if (!(vm->monitorpath = strdup(monitor))) { qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, @@ -669,7 +820,7 @@ qemudFindCharDevicePTYs(virConnectPtr conn, } /* Got them all, so now open the monitor console */ - ret = qemudOpenMonitor(conn, vm, monitor); + ret = qemudOpenMonitor(conn, vm, monitor, 0); cleanup: VIR_FREE(monitor); -- 1.6.0.2
On Fri, 2008-12-12 at 19:27 +0100, Guido Günther wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d8b87e4..57a396c 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -183,6 +183,46 @@ qemudAutostartConfigs(struct qemud_driver *driver) { }
+static int +qemudGetProcFD(pid_t pid, int fdnum) +{ + int fd; + +#ifdef __linux__ + char *path = NULL; + + if (!asprintf(&path, "/proc/%d/fd/%d", pid, fdnum)) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + return -1; + }
One more use of asprintf that needs to set path = NULL on error. Cheers, David
On Fri, Dec 12, 2008 at 11:38:59AM -0800, David Lutterkort wrote:
On Fri, 2008-12-12 at 19:27 +0100, Guido Günther wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d8b87e4..57a396c 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -183,6 +183,46 @@ qemudAutostartConfigs(struct qemud_driver *driver) { }
+static int +qemudGetProcFD(pid_t pid, int fdnum) +{ + int fd; + +#ifdef __linux__ + char *path = NULL; + + if (!asprintf(&path, "/proc/%d/fd/%d", pid, fdnum)) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + return -1; + }
One more use of asprintf that needs to set path = NULL on error.
This is crying out for us to write a virASprintf() that explicitly always sets path = NULL upon failure. And then blacklist 'asprintf' in make syntax-check. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Dec 12, 2008 at 08:30:26PM +0000, Daniel P. Berrange wrote:
This is crying out for us to write a virASprintf() that explicitly always sets path = NULL upon failure. And then blacklist 'asprintf' in make syntax-check. Possible virAsprintf attached. I've already moved the posted patch series over to it. I'll blacklist it once this is in and we have more calles moved over. O.k.? Cheers, -- Guido
On Fri, 2008-12-12 at 23:21 +0100, Guido Günther wrote:
On Fri, Dec 12, 2008 at 08:30:26PM +0000, Daniel P. Berrange wrote:
This is crying out for us to write a virASprintf() that explicitly always sets path = NULL upon failure. And then blacklist 'asprintf' in make syntax-check. Possible virAsprintf attached. I've already moved the posted patch series over to it. I'll blacklist it once this is in and we have more calles moved over. O.k.?
ACK .. very nice. David
On Fri, Dec 12, 2008 at 11:21:09PM +0100, Guido Günther wrote:
On Fri, Dec 12, 2008 at 08:30:26PM +0000, Daniel P. Berrange wrote:
This is crying out for us to write a virASprintf() that explicitly always sets path = NULL upon failure. And then blacklist 'asprintf' in make syntax-check. Possible virAsprintf attached. I've already moved the posted patch series over to it. I'll blacklist it once this is in and we have more calles moved over. O.k.?
Looks fine to me, +1 thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Dec 12, 2008 at 11:21:09PM +0100, Guido G?nther wrote:
On Fri, Dec 12, 2008 at 08:30:26PM +0000, Daniel P. Berrange wrote:
This is crying out for us to write a virASprintf() that explicitly always sets path = NULL upon failure. And then blacklist 'asprintf' in make syntax-check. Possible virAsprintf attached. I've already moved the posted patch series over to it. I'll blacklist it once this is in and we have more calles moved over. O.k.?
ACK. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Dec 15, 2008 at 11:23:15AM +0000, Daniel P. Berrange wrote:
On Fri, Dec 12, 2008 at 11:21:09PM +0100, Guido G?nther wrote:
On Fri, Dec 12, 2008 at 08:30:26PM +0000, Daniel P. Berrange wrote:
This is crying out for us to write a virASprintf() that explicitly always sets path = NULL upon failure. And then blacklist 'asprintf' in make syntax-check. Possible virAsprintf attached. I've already moved the posted patch series over to it. I'll blacklist it once this is in and we have more calles moved over. O.k.?
ACK. Applied now. -- Guido
On Fri, Dec 12, 2008 at 07:27:55PM +0100, Guido G?nther wrote:
The earlier patches didn't change libvirtd behaviour (except for kvm/qemu not being teared down on an unexpected daemon crash), all vms were still being shutoff at daemon shutdown. This patch now adds the code to read back the vm status after on daemon startup and reconnects all running vms found in stateDir which are left over from a daemon restart or crash and also disables shutting down of VMs when libvirt quits.
--- src/qemu_driver.c | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 158 insertions(+), 7 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d8b87e4..57a396c 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -183,6 +183,46 @@ qemudAutostartConfigs(struct qemud_driver *driver) { }
+static int +qemudGetProcFD(pid_t pid, int fdnum) +{ + int fd; + +#ifdef __linux__ + char *path = NULL; + + if (!asprintf(&path, "/proc/%d/fd/%d", pid, fdnum)) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + return -1; + } + + if((fd = open(path, O_RDONLY)) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Unable to open %s"), path); + return -1; + } + if (qemudSetCloseExec(fd) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Unable to set close-on-exec flag on %s"), path); + goto error; + } + + if (qemudSetNonBlock(fd) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Unable to put %s into non-blocking mode"), path); + goto error; + } + + VIR_FREE(path); + return fd; +error: + VIR_FREE(path); + close(fd); +#endif + return -1; +} + + /** * qemudRemoveDomainStatus * @@ -220,6 +260,104 @@ cleanup: }
+static int qemudOpenMonitor(virConnectPtr conn, + virDomainObjPtr vm, + const char *monitor, + int reconnect); + +/** + * qemudReconnectVMs + * + * Reconnect running vms to the daemon process + */ +static int +qemudReconnectVMs(struct qemud_driver *driver) +{ + int i; + + for (i = 0 ; i < qemu_driver->domains.count ; i++) { + virDomainObjPtr vm = qemu_driver->domains.objs[i]; + qemudDomainStatusPtr status = NULL; + char *config = NULL; + int rc; + + virDomainObjLock(vm); + /* Read pid */ + if ((rc = virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) == 0) + DEBUG("Found pid %d for '%s'", vm->pid, vm->def->name); + else + goto next; + + if ((config = virDomainConfigFile(NULL, + driver->stateDir, + vm->def->name)) == NULL) { + qemudLog(QEMUD_ERR, _("Failed to read domain status for %s\n"), + vm->def->name); + goto next_error; + } + + status = qemudDomainStatusParseFile(NULL, driver->caps, config, 0); + if (status) { + vm->newDef = vm->def; + vm->def = status->def; + } else { + qemudLog(QEMUD_ERR, _("Failed to parse domain status for %s\n"), + vm->def->name); + goto next_error; + } + + if ((rc = qemudOpenMonitor(NULL, vm, status->monitorpath, 1)) != 0) { + qemudLog(QEMUD_ERR, _("Failed to reconnect monitor for %s: %d\n"), + vm->def->name, rc); + goto next_error; + } else + vm->monitorpath = status->monitorpath; + + vm->stdin_fd = qemudGetProcFD(vm->pid, 0); + vm->stdout_fd = qemudGetProcFD(vm->pid, 1); + vm->stderr_fd = qemudGetProcFD(vm->pid, 2);
NACK, to these 3 lines - since we go to trouble of creating the statefile, we should store the FD numbers in the statefile too, instead of relying in the linux specific /proc code.
+ if (vm->stdin_fd == -1 || vm->stdout_fd == -1 || vm->stderr_fd == -1) + goto next_error; + + if (((vm->stdout_watch = virEventAddHandle(vm->stdout_fd, + VIR_EVENT_HANDLE_READABLE | + VIR_EVENT_HANDLE_ERROR | + VIR_EVENT_HANDLE_HANGUP, + qemudDispatchVMEvent, + driver, NULL)) < 0) || + ((vm->stderr_watch = virEventAddHandle(vm->stderr_fd, + VIR_EVENT_HANDLE_READABLE | + VIR_EVENT_HANDLE_ERROR | + VIR_EVENT_HANDLE_HANGUP, + qemudDispatchVMEvent, + driver, NULL)) < 0)) {
It is actually not neccessary to add the _ERROR or _HANGUP flags when registering a handle. Those two conditions are implict for every read or write watch. Yes, other code in QEMU driver adding them is bogus too.
/** * qemudStartup: * @@ -316,6 +454,7 @@ qemudStartup(void) { qemu_driver->autostartDir, NULL, NULL) < 0) goto error; + qemudReconnectVMs(qemu_driver); qemudAutostartConfigs(qemu_driver);
qemuDriverUnlock(qemu_driver); @@ -418,11 +557,14 @@ qemudShutdown(void) {
/* shutdown active VMs */ for (i = 0 ; i < qemu_driver->domains.count ; i++) { + /* FIXME: don't shutdown VMs on daemon shutdown for now */ +#if 0 virDomainObjPtr dom = qemu_driver->domains.objs[i]; virDomainObjLock(dom); if (virDomainIsActive(dom)) qemudShutdownVMDaemon(NULL, qemu_driver, dom); virDomainObjUnlock(dom); +#endif
Yep, just remove this block of code entirely - we dont want to shutdown VMs anymore. If someone wants to explicitly save or shutdown VMs during machine shutdown, they're probably better off explicitly using virsh to do it from an initscript. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Dec 15, 2008 at 11:27:27AM +0000, Daniel P. Berrange wrote:
NACK, to these 3 lines - since we go to trouble of creating the statefile, we should store the FD numbers in the statefile too, instead of relying in the linux specific /proc code. This patch actually monitors things in /proc which is not very portable. I'll think about something better. Let's leave out this (and the daemonizing patch for now) until I've cleaned this up.
+ if (vm->stdin_fd == -1 || vm->stdout_fd == -1 || vm->stderr_fd == -1) + goto next_error; + + if (((vm->stdout_watch = virEventAddHandle(vm->stdout_fd, + VIR_EVENT_HANDLE_READABLE | + VIR_EVENT_HANDLE_ERROR | + VIR_EVENT_HANDLE_HANGUP, + qemudDispatchVMEvent, + driver, NULL)) < 0) || + ((vm->stderr_watch = virEventAddHandle(vm->stderr_fd, + VIR_EVENT_HANDLE_READABLE | + VIR_EVENT_HANDLE_ERROR | + VIR_EVENT_HANDLE_HANGUP, + qemudDispatchVMEvent, + driver, NULL)) < 0)) {
It is actually not neccessary to add the _ERROR or _HANGUP flags when registering a handle. Those two conditions are implict for every read or write watch. Yes, other code in QEMU driver adding them is bogus too. Ahh, good to know.
/** * qemudStartup: * @@ -316,6 +454,7 @@ qemudStartup(void) { qemu_driver->autostartDir, NULL, NULL) < 0) goto error; + qemudReconnectVMs(qemu_driver); qemudAutostartConfigs(qemu_driver);
qemuDriverUnlock(qemu_driver); @@ -418,11 +557,14 @@ qemudShutdown(void) {
/* shutdown active VMs */ for (i = 0 ; i < qemu_driver->domains.count ; i++) { + /* FIXME: don't shutdown VMs on daemon shutdown for now */ +#if 0 virDomainObjPtr dom = qemu_driver->domains.objs[i]; virDomainObjLock(dom); if (virDomainIsActive(dom)) qemudShutdownVMDaemon(NULL, qemu_driver, dom); virDomainObjUnlock(dom); +#endif
Yep, just remove this block of code entirely - we dont want to shutdown VMs anymore. If someone wants to explicitly save or shutdown VMs during machine shutdown, they're probably better off explicitly using virsh to do it from an initscript. Great, I'll simply remove it. -- Guido
On Mon, Dec 15, 2008 at 11:27:27AM +0000, Daniel P. Berrange wrote: [..snip..]
+ vm->stdin_fd = qemudGetProcFD(vm->pid, 0); + vm->stdout_fd = qemudGetProcFD(vm->pid, 1); + vm->stderr_fd = qemudGetProcFD(vm->pid, 2);
NACK, to these 3 lines - since we go to trouble of creating the statefile, we should store the FD numbers in the statefile too, instead of relying in the linux specific /proc code. This is (as far as I know) the last blocker for merging the forking of qemus/kvms and reattaching them after libvirtd restart (so the vms survive a libvirtd crash/restart)[1]. While I can safe the path to std{in,out,err} in the domain status file easily this would still be /proc/<pid>/fd/{1,2,3} which is not portable.
After reading the monitor path on vm startup we only need the above fds to decect vm shutdown in qemudDispatchVMEvent(). This could be replaced by simply dup'ing the fds and using inotify on the pid file in /var/run/libvirt/qemu/ to detect vm shutdown instead - but this isn't portable outside of Linux either. Any other options I'm overlooking? Note that the current code doesn't break any non Linux port, it's just that the VMs won't get reattached after daemon restart. -- Guido [1] http://honk.sigxcpu.org/projects/libvirt/daemon-restart/
On Sun, Jan 11, 2009 at 02:09:26PM +0100, Guido G?nther wrote:
On Mon, Dec 15, 2008 at 11:27:27AM +0000, Daniel P. Berrange wrote: [..snip..]
+ vm->stdin_fd = qemudGetProcFD(vm->pid, 0); + vm->stdout_fd = qemudGetProcFD(vm->pid, 1); + vm->stderr_fd = qemudGetProcFD(vm->pid, 2);
NACK, to these 3 lines - since we go to trouble of creating the statefile, we should store the FD numbers in the statefile too, instead of relying in the linux specific /proc code. This is (as far as I know) the last blocker for merging the forking of qemus/kvms and reattaching them after libvirtd restart (so the vms survive a libvirtd crash/restart)[1]. While I can safe the path to std{in,out,err} in the domain status file easily this would still be /proc/<pid>/fd/{1,2,3} which is not portable.
After reading the monitor path on vm startup we only need the above fds to decect vm shutdown in qemudDispatchVMEvent(). This could be replaced by simply dup'ing the fds and using inotify on the pid file in /var/run/libvirt/qemu/ to detect vm shutdown instead - but this isn't portable outside of Linux either.
Any other options I'm overlooking? Note that the current code doesn't break any non Linux port, it's just that the VMs won't get reattached after daemon restart.
There's different needs for each file descriptor - stdin_fd - this is only ever used for incoming migration data all other times it is hooked up to /dev/null. So we don't need to keep this FD around at all - stdout_fd - AFAIK, the only time we get any data on stdout is when we run qemu -help to check support args. Current code will read & log all data on stdout, but I believe this is effectively nothing and so we could just hook it to /dev/null & ignore it - stderr_fd - libvirtd reads this FD & logs the data to the domain logfile in /var/log/libvirt/qemu/$NAME.log. Instead of having the daemon process this logging, we can just dup() the file straight onto the logfile FD, so data will be logged even when libvirtd is not running. The only minor complication is that we need to parse the logfile to get the monitor TTY path. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Jan 12, 2009 at 11:30:19AM +0000, Daniel P. Berrange wrote:
There's different needs for each file descriptor
- stdin_fd - this is only ever used for incoming migration data all other times it is hooked up to /dev/null. So we don't need to keep this FD around at all Agreed.
- stdout_fd - AFAIK, the only time we get any data on stdout is when we run qemu -help to check support args. Current code will read & log all data on stdout, but I believe this is effectively nothing and so we could just hook it to /dev/null & ignore it We can just dup that one too while at it in case qemu/kvm should dump anything else there.
- stderr_fd - libvirtd reads this FD & logs the data to the domain logfile in /var/log/libvirt/qemu/$NAME.log. Instead of having the daemon process this logging, we can just dup() the file straight onto the logfile FD, so data will be logged even when libvirtd is not running. The only minor complication is that we need to parse the logfile to get the monitor TTY path. There's one more thing (which was my intial reason for asking): we also use this fd in qemudDispatchVMEvent() to detect vm shutdown:
if (events & VIR_EVENT_HANDLE_READABLE) { if (qemudVMData(driver, vm, fd) < 0) failed = 1; } else { quit = 1; } } if (failed || quit) { event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, quit ? VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN : VIR_DOMAIN_EVENT_STOPPED_FAILED); qemudShutdownVMDaemon(NULL, driver, vm); if (!vm->persistent) { virDomainRemoveInactive(&driver->domains, vm); vm = NULL; } Using the monitor fd for this instead should work. Would that be o.k.? -- Guido
On Mon, Jan 12, 2009 at 04:37:50PM +0100, Guido G?nther wrote:
On Mon, Jan 12, 2009 at 11:30:19AM +0000, Daniel P. Berrange wrote:
There's different needs for each file descriptor
- stdin_fd - this is only ever used for incoming migration data all other times it is hooked up to /dev/null. So we don't need to keep this FD around at all Agreed.
- stdout_fd - AFAIK, the only time we get any data on stdout is when we run qemu -help to check support args. Current code will read & log all data on stdout, but I believe this is effectively nothing and so we could just hook it to /dev/null & ignore it We can just dup that one too while at it in case qemu/kvm should dump anything else there.
- stderr_fd - libvirtd reads this FD & logs the data to the domain logfile in /var/log/libvirt/qemu/$NAME.log. Instead of having the daemon process this logging, we can just dup() the file straight onto the logfile FD, so data will be logged even when libvirtd is not running. The only minor complication is that we need to parse the logfile to get the monitor TTY path. There's one more thing (which was my intial reason for asking): we also use this fd in qemudDispatchVMEvent() to detect vm shutdown:
if (events & VIR_EVENT_HANDLE_READABLE) { if (qemudVMData(driver, vm, fd) < 0) failed = 1; } else { quit = 1; } }
if (failed || quit) { event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, quit ? VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN : VIR_DOMAIN_EVENT_STOPPED_FAILED); qemudShutdownVMDaemon(NULL, driver, vm); if (!vm->persistent) { virDomainRemoveInactive(&driver->domains, vm); vm = NULL; }
Using the monitor fd for this instead should work. Would that be o.k.?
Yeah, I thought it was already using the monitor FD for this ! Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
David Lutterkort -
Guido Günther