[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/3] qemu: don't leave vm in table on failure to attach



On 28.08.2013 22:56, Eric Blake wrote:
> Failure to attach to a domain during 'virsh qemu-attach' left
> the list of domains in an odd state:
> 
> $ virsh qemu-attach 4176
> error: An error occurred, but the cause is unknown
> 
> $ virsh list --all
>  Id    Name                           State
> ----------------------------------------------------
>  2     foo                            shut off
> 
> $ virsh qemu-attach 4176
> error: Requested operation is not valid: domain is already active as 'foo'
> 
> $ virsh undefine foo
> error: Failed to undefine domain foo
> error: Requested operation is not valid: cannot undefine transient domain
> 
> $ virsh shutdown foo
> error: Failed to shutdown domain foo
> error: invalid argument: monitor must not be NULL
> 
> It all stems from leaving the list of domains unmodified on
> the initial failure; we should follow the lead of createXML
> which removes vm on failure (the actual initial failure still
> needs to be fixed in a later patch, but at least this patch
> gets us to the point where we aren't getting stuck with an
> unremovable "shut off" transient domain).
> 
> * src/qemu/qemu_driver.c (qemuDomainQemuAttach): Match cleanup of
> qemuDomainCreateXML.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>  src/qemu/qemu_driver.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ed29373..a52c954 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13623,8 +13623,11 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
> 
>      if (qemuProcessAttach(conn, driver, vm, pid,
>                            pidfile, monConfig, monJSON) < 0) {
> +        if (qemuDomainObjEndJob(driver, vm) > 0)
> +            qemuDomainRemoveInactive(driver, vm);
> +        vm = NULL;
>          monConfig = NULL;
> -        goto endjob;
> +        goto cleanup;
>      }
> 

While this part looks okay ...

>      monConfig = NULL;
> @@ -13632,7 +13635,6 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
>      dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
>      if (dom) dom->id = vm->def->id;
> 
> -endjob:
>      if (qemuDomainObjEndJob(driver, vm) == 0) {
>          vm = NULL;
>          goto cleanup;
> 

... I am not convinced about this part. I mean, in the context, we
create Domain and store it into @dom. But if qemu process dies
meanwhile, so the qemuDomainObjEndJob() returns zero, the @dom refers to
a stale process. Or am I missing something here? I think these lines
should be swapped.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]