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

[Jan Beulich <jbeulich@xxxxxxxx>]

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? The prior
printk() simply ended up defaulting to a warning, and I would think
that's what the new one should be doing too. Or even leave the
setting of the log level to the invocation sites of the macro.

Jan