Re: [pve-devel] [PATCH qemu-server v3 3/5] await and kill lingering KVM thread when VM start reaches timeout

[Daniel Tschlatscher <d.tschlatscher@proxmox.com>]

On 12/22/22 14:20, Fabian Grünbichler wrote:
> On December 22, 2022 1:58 pm, Daniel Tschlatscher wrote:
> 
>>>>  
>>>> -	    my $exitcode = run_command($cmd, %run_params);
>>>> -	    if ($exitcode) {
>>>> -		if ($tpmpid) {
>>>> -		    warn "stopping swtpm instance (pid $tpmpid) due to QEMU startup error\n";
>>>> -		    kill 'TERM', $tpmpid;
>>>> +	    eval {
>>>> +		my $exitcode = run_command($cmd, %run_params);
>>>> +
>>>> +		if ($exitcode) {
>>>> +		    if ($tpmpid) {
>>>> +			log_warn "stopping swtpm instance (pid $tpmpid) due to QEMU startup
>>> error\n";
>>>
>>> this warn -> log_warn change kind of slipped in, it's not really part of this
>>> patch?
>>
>> Because I changed this line anyway, I changed it to log_warn as it is
>> imported already and, as I understood, the preferable alternative
>> to calling 'warn'.
>> Sourcing this in it's own patch seems overkill to me, or would you
>> rather suggest something like this should be handled in, e.g. a
>> file-encompassing refactoring?
> 
> ideally it could be sent as cleanup patch up-front (then it can be applied even
> if the rest needs another round ;)) or at least mentioned somewhere (e.g., in
> the patch notes). seemingly unrelated changes in a patch always make me wary that
> the patch was generated from some unclean tree/more or less than intended was
> `git add`ed. in this case my guess was that you just changed that (wrapped) call
> site to match your newly introduced ones, but it could also have been an
> unintentional search+replace result, for example, so I'd rather ask :)

As the v2 still used the 'warn' calls, I mentioned it under the "Changes
from v2" section. 'Changed warn to use 'log_warn' instead'. (Albeit
wasn't at my best with this wording here)

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