On 04/15/2016 09:22 AM, Cole Robinson wrote:
> On 04/15/2016 08:59 AM, Ján Tomko wrote:
>> On Fri, Apr 15, 2016 at 08:54:23AM -0400, Cole Robinson wrote:
>>> We perform the same check several lines earlier
>>> ---
>>> src/qemu/qemu_driver.c | 6 ------
>>> 1 file changed, 6 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index e795062..ced808a 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -18403,32 +18403,26 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
>>>
>>> if (!virDomainObjIsActive(vm)) {
>>> virReportError(VIR_ERR_OPERATION_INVALID,
>>> "%s", _("domain is not running"));
>>> goto cleanup;
>>> }
>>>
>>> if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>>> goto cleanup;
>>>
>>> if (!qemuDomainAgentAvailable(vm, true))
>>> goto endjob;
>>>
>>
>> This is not redundant, the domain might stop running while we wait for
>> the job with the domain lock unlocked.
>
> Okay, I dug a bit deeper.
>
> I grepped through all "domain is not running" errors and the only functions
> that have duplicate IsActive calls are qemuDomainSuspend, InjectNMI,
> qemuDomainQemuMonitorCommand, qemuDomainPMSuspendForDuration,
> qemuDomainFSTrim, qemuDomainSetTime, which all have an additional one _before_
> the BeginJob check, which AFAICT only errors a bit earlier instead of waiting
> for the job lock, which isn't that useful. I looked at git history for
> DomainSuspend and DomainQemuMonitorCommand and it looks like both were added
> basically accidentally without any explicit purpose.
>
> The pattern that most functions call, and which seems ideal to me, is:
>
> - ACL check
> - BeginJob (if needed)
> - AgentAvailable (if needed, which btw checks domain is RUNNING btw)
> - !IsActive
>
> I'll send a patch to fix those up, seems like most cases were cargo culted anyways
>
Hmm, a couple have arguably valid use cases. I'll add a separate patch with
comments for those
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list