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
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
