[libvirt] [RFC PATCH] check whether domain is active after qemuDomainObjExitMonitor* returns

[Daniel P. Berrange]

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