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

[Peter Krempa]

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 at virtuozzo.com>
> >> ---
> >>  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.)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190328/97dee801/attachment-0001.sig>