On 13.07.2016 17:55, John Ferlan wrote: > > > On 06/30/2016 05:42 AM, Nikolay Shirokovskiy wrote: >> The use case is similar to e2b86f580. First call shutdown with >> reboot semantics in agent mode. As a result fake reboot flag is set. >> Next issue shutdown from guest and you will get reboot due to fake >> reboot flag set. >> >> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx> >> --- >> src/qemu/qemu_driver.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> > > In qemuDomainShutdownFlags 'isReboot' is initialized to 'false'; > whereas, in qemuDomainReboot it's initialized to 'true'. Both routines > can change the value based on the domain settings. > > For shutdown, it's what the domain has set for "onPoweroff" - we'll > request a reboot. > > Trying to figure out what I'm missing from your commit message and what > I'm reading in the code. By moving this from where it is now to be > cleared unless domain setting onPoweroff dictates a restart to a more > specific spot where the value only changes if the Agent isn't used or > fails (or perhaps from the old non flags qemuDomainShutdown API), you > could conceivably "leave" the setting as is for Agent powerdown. > > This also seems to undo commit id '8be502fd' - ok your placement is a > bit different, but nonetheless there's an ominous looking commit message > there. I've studied 8be502fd quite thouroughly, more information on it is available in original letter [1]. The reasoning is next: we should set fakeReboot flag whenever SHUTDOWN event will be result of shutdown/reboot process because this event handler checks fakeReboot. Although appoach is fine the patch does not consider the case when the semantics of shutdown operation is changed thru domain configuration. By the way I can't understand how Zhang managed to trigger the problem. Yes reboot sets the flag but then event handler reset it back to false so next shutdown thru agent should not be affected. However I still think the approach to always set the flag if it is later will be checked is supreme. So I'll send next version of the patch. Stay tuned. > > Also of note is in the Reboot code there's some #ifdef's w/ YAJL, but > those aren't present for the shutdown code... That's not part of this > bug, but perhaps someone with more libvirt history can answer... > > Maybe the "shared" code should be combined in some way... True, reboot and shutdown are very much the same. [1] https://www.redhat.com/archives/libvir-list/2015-April/msg00790.html > > John >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 61d184b..f9562bd 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -1997,8 +1997,6 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) >> useAgent = false; >> } >> >> - qemuDomainSetFakeReboot(driver, vm, isReboot); >> - >> if (useAgent) { >> qemuDomainObjEnterAgent(vm); >> ret = qemuAgentShutdown(priv->agent, agentFlag); >> @@ -2018,6 +2016,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) >> goto endjob; >> } >> >> + qemuDomainSetFakeReboot(driver, vm, isReboot); >> qemuDomainObjEnterMonitor(driver, vm); >> ret = qemuMonitorSystemPowerdown(priv->mon); >> if (qemuDomainObjExitMonitor(driver, vm) < 0) >> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list