Re: [PATCH v5 2/6] hw/loongarch/virt: Fix error handling in cpu plug

[bibo mao <maobibo@loongson.cn>]

On 2025/3/20 下午3:25, Markus Armbruster wrote:
> bibo mao <maobibo@loongson.cn> writes:
> 
> On 2025/3/20 下午2:16, Markus Armbruster wrote:
>>> Bibo Mao <maobibo@loongson.cn> writes:
>>>
>>>> In function virt_cpu_plug(), it will send cpu plug message to interrupt
>>>> controller extioi and ipi irqchip. If there is problem in this function,
>>>> system should continue to run and keep state the same before cpu is
>>>> added.
>>>>
>>>> Object cpuslot::cpu is set at last only when there is no any error.
>>>> If there is, send cpu unplug message to extioi and ipi irqchip, and then
>>>> return immediately.
>>>>
>>>> Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
>>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> 
> [...]
> 
>>> Hmm.
>>>
>>> You're right about the problem: virt_cpu_plug() neglects to revert
>>> changes when it fails.
>>>
>>> You're probably right to move the assignment to cpu_slot->cpu to the
>>> end.  Anything you can delay until success is assured you don't have to
>>> revert.  I say "probably" because the code that now runs before the
>>> assignment might theoretically "see" the assignment, and I didn't
>>> examine it to exclude that.
>>>
>>> Where I have doubts is the code to revert changes.
>>>
>>> The hotplug_handler_plug() error checkign suggests it can fail.
>>>
>>> Can hotplug_handler_unplug() fail, too?  The error checking in
>>> virt_cpu_unplug() right above suggests it can.
>>
>> Basically from existing code about ipi/extioi hotplug handler, it is
>> impossible to there is error, here is only for future use.
> 
> Aha.  More at the end of my reply.
> 
>> If there is error in function virt_cpu_plug(), undo() such as
>> hotplug_handler_unplug() should be called. However if undo() reports
>> error, I do not know how to handle it, here just discard error in
>> function undo().
> 
> Steinbach's Guideline for Systems Programming: Never test for an error
> condition you don't know how to handle.
> 
> This old quip is a funny way to say that errors we don't know how to
> handle are *bad*, and should be avoided.
> 
>> Regards
>> Bibo Mao
>>>
>>> What happens if it fails in virt_cpu_plug()?
> 
> You assure us this can't happen today.  Because of that, broken error
> recovery is not an actual problem.
> 
> However, if things change some day so it can happen, broken error
> recovery becomes an actual problem.
> 
> so, broken error recovery just "for future use" is actually just for
> silent future breakage.
> 
> But is it broken?  This is what I'm trying to find out with my "what
> happens if" question.
> 
> If it is broken, then passing &error_abort would likely be less bad:
> crash instead of silent breakage.  Also makes it completely obvious in
> the code that these errors are not handled, whereas broken error
> handling looks like it is until you actually think about it.
> 
yes, it seems that &error_abort is better than NULL, it is better than 
slient and do nothing. If error really happens, we had to solve it then.

And I will refresh the patch in next version.

Regards
Bibo Mao