On Fri, Sep 11, 2020 at 01:39:32PM +0200, Ján Tomko wrote: > On a Friday in 2020, Erik Skultety wrote: > > On Fri, Sep 11, 2020 at 03:06:08PM +0800, Lin Ma wrote: > > > It requires a guest agent configured and running in the domain's guest > > > OS, So check qemu agent during qemuDomainPMSuspendForDuration(). > > > > > > Signed-off-by: Lin Ma <lma@xxxxxxx> > > > --- > > > > qemuDomainPMSuspendAgent later in that same function already checks it, we > > don't need to check it twice. > > > > It only does so after calling qemuDomainQueryWakeupSuspendSupport, which > requires grabbing a MODIFY job and executing a QMP command. Why does a query grab a MODIFY job anyway? > > I think doing this check upfront is reasonable. We will error out as > soon as we know it, just for the price for two extra duplicit lines. True, it will save a fraction of time, I guess I could live with that. More importantly though, this patch uncovered a bug in the code where we report a successful suspend even when the agent is not connected and we logged an error accordingly. Erik > > Jano > > > NACK > > > > > src/qemu/qemu_driver.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > > > index 2e46931c71..bd287f259e 100644 > > > --- a/src/qemu/qemu_driver.c > > > +++ b/src/qemu/qemu_driver.c > > > @@ -16820,6 +16820,9 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, > > > if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0) > > > goto cleanup; > > > > > > + if (!qemuDomainAgentAvailable(vm, true)) > > > + goto cleanup; > > > + > > > /* > > > * The case we want to handle here is when QEMU has the API (i.e. > > > * QEMU_CAPS_QUERY_CURRENT_MACHINE is set). Otherwise, do not interfere > > > -- > > > 2.26.0 > > > > >
