Re: [libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive

On Thu, Mar 28, 2019 at 08:43:46 +0000, Nikolay Shirokovskiy wrote: > > > On 28.03.2019 11:27, Peter Krempa wrote: > > On Thu, Mar 28, 2019 at 10:29:01 +0300, Nikolay Shirokovskiy wrote: > >> Mgmt can not track if domain is already inactive before > >> calling destroy because domain can become inactive because > >> of crash/shutdown from guest. Thus it is make sense to > > > > Well mgmt apps can use events emitted by libvirt precisely for this > > case. > > This is still racy. > > > > >> report success in this case. Another option is to return > >> special error code but this is a bit more complicated. > >> > >> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > >> --- > >> src/qemu/qemu_driver.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > >> index 62d8d97..0789efc 100644 > >> --- a/src/qemu/qemu_driver.c > >> +++ b/src/qemu/qemu_driver.c > >> @@ -2172,8 +2172,10 @@ qemuDomainDestroyFlags(virDomainPtr dom, > >> if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) > >> goto cleanup; > >> > >> - if (virDomainObjCheckActive(vm) < 0) > >> + if (!virDomainObjIsActive(vm)) { > >> + ret = 0; > >> goto cleanup; > >> + } > > > > I'm not persuaded we want this. The commit message does not provide > > enough means to justify it. Every other API we have returns error in > > case when the domain is in the state the API will change it to so I'm > > not in favor of making this api behave differently. > > > > Ok then here is the usecase. We want to shutdown domain and unfortunately > this operation failed to bring domain to shutoff state in time. Thus mgmt try > to call destroy as it wants domain to be shutoff. Destroy returns quite > general VIR_ERR_OPERATION_INVALID error code so mgmt need to face > the problem but in reality everything is ok. I understand the problem here, but I disagree that the API should return success if it didn't do anything when it previously was returning errors. You can choose to implement a new error code to be used instead of VIR_ERR_OPERATION_INVALID in virDomainObjCheckActive. E.g. VIR_ERR_OBJECT_INACTIVE (to be generic enough to work with networks/storage pools/etc.)