Re: [PATCH v2 1/2] xen/riscv: initialize bootinfo from dtb

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/10/2024 8:50 am, oleksii.kurochko@xxxxxxxxx wrote:
> On Tue, 2024-10-15 at 14:32 +0200, Jan Beulich wrote:
>> On 15.10.2024 11:11, oleksii.kurochko@xxxxxxxxx wrote:
>>> On Tue, 2024-10-15 at 08:33 +0200, Jan Beulich wrote:
>>>> On 10.10.2024 17:30, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/riscv/setup.c
>>>>> +++ b/xen/arch/riscv/setup.c
>>>>> @@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long
>>>>> bootcpu_id,
>>>>>                            _end - _start, false) )
>>>>>          panic("Failed to add BOOTMOD_XEN\n");
>>>>>  
>>>>> +    BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));
>>>> We generally aim at avoiding side effects in BUG_ON() (or
>>>> ASSERT()).
>>>> With
>>>>
>>>>     if (!boot_fdt_info(device_tree_flattened, dtb_addr))
>>>>         BUG();
>>>>
>>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>
>>>> I can make the adjustment while committing, if desired.
>>> It would be great if these changes could be made during the commit.
>> I've committed it with the adjustment, 
> Thanks!
>
>> yet once again I wondered: Why not
>> panic()?
> It could be panic() here; there's no specific reason. I agree that
> using BUG() here isn't logically correct, as technically, a size of
> zero for the FDT isn't a bug but rather indicates that someone provided
> an incorrect FDT to Xen.
>
> I will use panic() in the future for such cases.
>
> It’s not always clear what should be used and where. Perhaps it would
> be helpful to add some explanation somewhere.

panic() gets you a single line diagnostic.  BUG() gets you a backtrace,
but no diagnostic information at all.

BUG() should be a last resort, because it requires someone to go digging
in the source code to even figure out what went wrong.  panic() (with a
good diagnostic message, e.g. "unexpected $foo in device tree") is far
more meaningful.

There are ways of getting a backtrace in combination with a panic(), if
the situation warrants it.

~Andrew