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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list