Re: [PATCH] qemu: don't set fake reboot if it will not be used

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]