On 9/5/19 11:47 AM, Thomas Lamprecht wrote:
On 23.08.19 10:55, Dominik Csapak wrote:
Signed-off-by: Dominik Csapak <d.csa...@proxmox.com>
---
new in v2
trigger is normally something which is done actively, i.e., gets triggered
like "pulling the trigger" says.

Maybe call it a flag and the file "$vmid.reboot-request"?
fine with me, i merely copied the naming from container
(i wanted it to be consistent across vm/ct; we can rename it there also ofc)

  PVE/QemuServer.pm | 12 ++++++++++++
  1 file changed, 12 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 9f5bf56..d1767a9 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7369,6 +7369,18 @@ sub nbd_stop {
      vm_mon_cmd($vmid, 'nbd-server-stop');
  }
+sub create_reboot_trigger {
create_reboot_request?

sounds fine

+    my ($vmid) = @_;
please add a newline here
ok

+    open(my $fh, '>', "/run/qemu-server/$vmid.reboot")
+       or die "failed to create reboot trigger file: $!\n";
maybe indentation issue for or
what do you mean?

    open[..]
<tab>or die[..]

is our indentation scheme?

+    close($fh);
+}
+
+sub remove_reboot_trigger {
maybe clear_reboot_request?
ok

+    my ($vmid) = @_;
+    return unlink("/run/qemu-server/$vmid.reboot");
FYI, unlink can also fail for other reasons that ENOENT,
which you use as return value, see `man 2 unlink`

Maybe do a bit more and actively check if the file is here first?
(This will be always racy and thus should only be done in locked
context anyway)

Or explicitly check if the error was ENOENT ?
was also copied from container, but yeah it makes to check if
it exists and log an error in case we cannot remove it

it would only reboot it in case we can remove the file though,
otherwise we risk that the user cannot shutdown the vm...

+}
+
  # bash completion helper
sub complete_backup_archives {

_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Reply via email to