On Fri, Mar 16, 2012 at 02:37:42PM +0800, Wen Congyang wrote:
> When qemu cannot start, we may call qemuProcessStop() twice.
> We have check whether the vm is running at the beginning of
> qemuProcessStop() to avoid libvirt deadlock. We call
> qemuProcessStop() with driver and vm locked. It seems that
> we can avoid libvirt deadlock. But unfortunately we may
> unlock driver and vm in the function qemuProcessKill() while
> vm->def->id is not -1. So qemuProcessStop() will be run twice,
> and monitor will be freed unexpectedly. So we should set
> vm->def->id to -1 at the beginning of qemuProcessStop().
>
> ---
> src/qemu/qemu_process.c | 20 ++++++++++++++------
> src/qemu/qemu_process.h | 2 ++
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 0af3751..44814df 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3633,10 +3633,11 @@ qemuProcessKill(struct qemud_driver *driver,
> VIR_DEBUG("vm=%s pid=%d flags=%x",
> vm->def->name, vm->pid, flags);
>
> - if (!virDomainObjIsActive(vm)) {
> - VIR_DEBUG("VM '%s' not active", vm->def->name);
> - return 0;
> - }
> + if (!(flags & VIR_QEMU_PROCESS_KILL_NOCHECK))
> + if (!virDomainObjIsActive(vm)) {
> + VIR_DEBUG("VM '%s' not active", vm->def->name);
> + return 0;
> + }
>
> /* This loop sends SIGTERM (or SIGKILL if flags has
> * VIR_QEMU_PROCESS_KILL_FORCE and VIR_QEMU_PROCESS_KILL_NOWAIT),
> @@ -3739,6 +3740,13 @@ void qemuProcessStop(struct qemud_driver *driver,
> return;
> }
>
> + /*
> + * We may unlock driver and vm in qemuProcessKill(), so the other thread
> + * can lock driver and vm, and then call qemuProcessStop(). So we should
> + * set vm->def->id to -1 here to avoid qemuProcessStop() called twice.
> + */
> + vm->def->id = -1;
> +
> if ((logfile = qemuDomainCreateLog(driver, vm, true)) < 0) {
> /* To not break the normal domain shutdown process, skip the
> * timestamp log writing if failed on opening log file. */
> @@ -3801,7 +3809,8 @@ void qemuProcessStop(struct qemud_driver *driver,
> }
>
> /* shut it off for sure */
> - ignore_value(qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE));
> + ignore_value(qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE|
> + VIR_QEMU_PROCESS_KILL_NOCHECK));
>
> /* Stop autodestroy in case guest is restarted */
> qemuProcessAutoDestroyRemove(driver, vm);
> @@ -3892,7 +3901,6 @@ retry:
>
> vm->taint = 0;
> vm->pid = -1;
> - vm->def->id = -1;
> virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);
> VIR_FREE(priv->vcpupids);
> priv->nvcpupids = 0;
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 761db6f..891f7a5 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -72,6 +72,8 @@ int qemuProcessAttach(virConnectPtr conn,
> typedef enum {
> VIR_QEMU_PROCESS_KILL_FORCE = 1 << 0,
> VIR_QEMU_PROCESS_KILL_NOWAIT = 1 << 1,
> + VIR_QEMU_PROCESS_KILL_NOCHECK = 1 << 2, /* donot check whether the vm is
> + running */
> } virQemuProcessKillMode;
>
> int qemuProcessKill(struct qemud_driver *driver,
Hi Wen,
sorry for the delay in reviewing the patch. I think I understand the
issue, and removing the pid and using an extra flag to qemuProcessKill
to avoid the race sounds reasonable. I rebased the patch a bit, made
some cosmetic changes especially on the comments and pushed it so it is
included in rc2 for more testing,
thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list