On Mon, Apr 11, 2011 at 01:41:55PM +0800, Wen Congyang wrote:
qemu may quited unexpectedly when invoking a monitor command. And priv->mon will be NULL after qemuDomainObjExitMonitor* returns. So we must not use it. Unfortunately we still use it, and it will cause libvirtd crashed.
As Eric suggested, qemuDomainObjExitMonitor* should be made to return the value of vm active after regaining lock, and marked ATTRIBUTE_RETURN_CHECK, to force all other callers to detect the case of a monitor command completing successfully but then the VM disappearing in the window between command completion and regaining the vm lock.
@@ -578,7 +578,7 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) * * Should be paired with an earlier qemuDomainObjEnterMonitor() call */ -void qemuDomainObjExitMonitor(virDomainObjPtr obj) +int qemuDomainObjExitMonitor(virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; int refs; @@ -588,11 +588,20 @@ void qemuDomainObjExitMonitor(virDomainObjPtr obj) if (refs > 0) qemuMonitorUnlock(priv->mon);
+ /* Note: qemu may quited unexpectedly here, and the monitor will be freed. + * If it happened, priv->mon will be null. + */ + virDomainObjLock(obj);
if (refs == 0) { priv->mon = NULL; } + + if (priv->mon == NULL) + return -1;
Perhaps here we should do qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("guest unexpectedly quit"));
+ else + return 0; }
@@ -621,8 +630,8 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, * * Should be paired with an earlier qemuDomainObjEnterMonitorWithDriver() call */ -void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, - virDomainObjPtr obj) +int qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, + virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; int refs; @@ -632,12 +641,21 @@ void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, if (refs > 0) qemuMonitorUnlock(priv->mon);
+ /* Note: qemu may quited unexpectedly here, and the monitor will be freed. + * If it happened, priv->mon will be null. + */ + qemuDriverLock(driver); virDomainObjLock(obj);
if (refs == 0) { priv->mon = NULL; } + + if (priv->mon == NULL) + return -1;
And the same here
+ else + return 0; }
@@ -1452,7 +1452,11 @@ static int qemudDomainShutdown(virDomainPtr dom) { priv = vm->privateData; qemuDomainObjEnterMonitor(vm); ret = qemuMonitorSystemPowerdown(priv->mon); - qemuDomainObjExitMonitor(vm); + if (qemuDomainObjExitMonitor(vm) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + ret = -1; + }
So we can avoid qemuReportError() here, and in every single other place where ExitMonitor is called. It would make the code a bit shorter Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|