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

[Thomas Lamprecht]

On 05.09.19 12:27, Dominik Csapak wrote:
> 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>
>>
>>> +    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?

sorry, misinterpreted the tabulator in my MTA, ignore that

> 
>     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


I nowhere saw anything mentioning that this all is just copied over from CT.
It makes sense that it's there too, but maybe we could add the common code
then to guest-common? (and fix it for both)

> 
> it would only reboot it in case we can remove the file though,
> otherwise we risk that the user cannot shutdown the vm...
> 
yes, all else is an error, but I would warn and say that reboot was ignored
because of "blah".

Another possible alternative, in combination with the separate vm_reboot
method from my 3/5 review, could be to remove this early in the locked context,
before doing the stop, and in the case of a error on removal just abort and
do nothing - then the VM would keep running, maybe a better option?
As a reboot always has the end goal to have a running machine again, if you
stop it in case of error the goal is violated, if you do nothing it isn't fully
violated (as it may still be outdated on pending changes)


>>
>>> +}
>>> +
>>>   # bash completion helper
>>>     sub complete_backup_archives {
>>>
>>