On Thu, Mar 28, 2019 at 09:53:08AM +0100, Peter Krempa wrote:
> 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@xxxxxxxxxxxxx>
> > >> ---
> > >> 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.)
Why can't the mgmt app simply ignore the existing OPERATION_INVALID
error they get from destroy.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
