On 28.03.2019 12:56, Daniel P. Berrangé wrote: > On Thu, Mar 28, 2019 at 09:53:03AM +0000, Nikolay Shirokovskiy wrote: >> >> >> On 28.03.2019 12:44, Daniel P. Berrangé wrote: >>> 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. >>> >> >> Looks inactive domain is the only reason for OPERATION_INVALID right now. >> Still it sounds very general for mgmt to ignore. > > OPERATION_INVALID is a code that is intended to be reported when a guest > is in the wrong running state. So if the operation requires a running > guest & it is shutoff, or if the operation requires an inactive guest > and it is running. > There are a a lot of cases in qemu_driver.c when this code is reported when operation is not applicable for current general domain state/configuration, not only running state. We can specify invalid state restriction for destroy operation I guess... Nikolay -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list