4:42 a.m.
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(a)virtuozzo.com>
---
src/qemu/qemu_driver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
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)
--
1.8.3.1
9:55 a.m.
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(a)virtuozzo.com>
---
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.
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...
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)
5:19 a.m.
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(a)virtuozzo.com>
> ---
> 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)
>
3084
days inactive
3098
days old
2 comments
2 participants
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy