Re: [PATCH] xen: Modify domain_crash() to take a print string

[Andrew Cooper <amc96@xxxxxxxx>]

On 03/02/2022 15:06, Jan Beulich wrote:
> On 03.02.2022 15:41, Andrew Cooper wrote:
>> On 03/02/2022 14:19, Jan Beulich wrote:
>>> On 03.02.2022 15:11, Andrew Cooper wrote:
>>>> On 03/02/2022 13:48, Julien Grall wrote:
>>>>> On 03/02/2022 13:38, Andrew Cooper wrote:
>>>>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>>>>> index 37f78cc4c4c9..38b390d20371 100644
>>>>>> --- a/xen/include/xen/sched.h
>>>>>> +++ b/xen/include/xen/sched.h
>>>>>> @@ -736,10 +736,15 @@ void vcpu_end_shutdown_deferral(struct vcpu *v);
>>>>>>    * from any processor.
>>>>>>    */
>>>>>>   void __domain_crash(struct domain *d);
>>>>>> -#define domain_crash(d) do
>>>>>> {                                              \
>>>>>> -    printk("domain_crash called from %s:%d\n", __FILE__,
>>>>>> __LINE__);       \
>>>>>> -   
>>>>>> __domain_crash(d);                                                    \
>>>>>> -} while (0)
>>>>>> +#define domain_crash(d, ...)                            \
>>>>>> +    do {                                                \
>>>>>> +        if ( count_args(__VA_ARGS__) == 0 )             \
>>>>>> +            printk("domain_crash called from %s:%d\n",  \
>>>>>> +                   __FILE__, __LINE__);                 \
>>>>> I find a bit odd that here you are using a normal printk
>>>> That's unmodified from before.  Only reformatted.
>>>>
>>>>> but...
>>>>>
>>>>>
>>>>>> +        else                                            \
>>>>>> +            printk(XENLOG_G_ERR __VA_ARGS__);           \
>>>>> here it is XENLOG_G_ERR. In fact, isn't it ratelimited? If so,
>>>>> wouldn't it be better to only use XENLOG_ERR so they can always be
>>>>> seen? (A domain shouldn't be able to abuse it).
>>>> Perhaps.  I suppose it is more important information than pretty much
>>>> anything else about the guest.
>>> Indeed, but then - is this really an error in all cases?
>> Yes.  It is always a fatal event for the VM.
> Which may or may not be Xen's fault. If the guest put itself in a bad
> state, I don't see why we shouldn't consider such just a warning.

Log level is the severity of the action, not who's potentially to blame
for causing the situation.

Furthermore, almost all callers who do emit appropriate diagnostics
before domain_crash() already use ERR.

And, as already pointed out, it doesn't matter.  The line is going out
on the console however you want to try and bikeshed the internals.

>  IOW
> I continue to think a log level, if so wanted, should be supplied by
> the user of the macro.

That undermines the whole purpose of preventing callers from being able
to omit diagnostics.

~Andrew