[pve-devel] [PATCH qemu-server v2 1/5] add create and remove reboot trigger subs

[Dominik Csapak]

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.csapak at 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 {
>>
>