-
-
Notifications
You must be signed in to change notification settings - Fork 149
Simplify qemu shutdown script? #381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Possible extension: we could do a |
When run in a systemd service, systemd will kill after timeout already. |
lib/runners/qemu.nix
Outdated
cat | ||
) | \ | ||
${pkgs.socat}/bin/socat STDIO UNIX:${socket},shut-none | ||
'' | ||
else throw "Cannot shutdown without socket"; | ||
|
||
setBalloonScript = | ||
if socket != null | ||
if socket != null && balloon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line shoud be changed in the other commit which probably should also be an extra commit.
That doesn't read promissing from the docs. We never really want this. |
@@ -331,7 +331,7 @@ lib.warnIf (mem == 2048) '' | |||
then | |||
'' | |||
( | |||
${writeQmp { execute = "qmp_capabilities"; }} | |||
${writeQmp { execute = "query-commands"; }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is your motivation to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as below, I did not know this was required.
if balloon | ||
then | ||
if socket != null then | ||
'' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change this again, we can drop the change in the last commit, to make the history cleaner.
${writeQmp { execute = "qmp_capabilities"; }} | ||
${writeQmp { execute = "balloon"; arguments.value = 987; }} | ||
) | sed -e s/987/$VALUE/ | \ | ||
${pkgs.socat}/bin/socat STDIO UNIX:${socket},shut-none | \ | ||
tail -n 1 | \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you test that this did not break anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's because I removed the echo capabilities line, which doesn't do anything because it gets tailed away. That line doesn't seem to do anything at all because the user won't be able to see the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry I just read the docs; I did not realise you had to execute capabilities to establish a connection, what a strange requirement
https://qemu-project.gitlab.io/qemu/interop/qemu-qmp-ref.html#command-QMP-machine.system_powerdown
I'm assuming this is doing an ACPI shutdown request based on how they describe it. But that should be good enough right? For me the current keystroke sequence doesn't even work.
Also I can't help but to modify the close by balloon script as well