Re: [Xen-devel] [PATCH] xen: Improvements to domain_crash_sync()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.18 at 12:16, <andrew.cooper3@xxxxxxxxxx> wrote:
> The use of __LINE__ in a printk() is problematic for livepatching, as it
> causes unnecessary binary differences.
> 
> Furthermore, diagnostic information around calls is inconsistent and
> occasionally unhelpful.  (e.g. diagnosing logs from the field which might be
> release builds, or likely without exact source code).
> 
> Take the opportunity to improve things.  Shorten the name to
> domain_crash_sync() and require the user to pass a print message in.

First of all I'd like to re-iterate that a long time ago a plan was
formulated to entirely remove synchronous domain crashing. If I
leave aside the three uses in wait.c (which you say you want to
remove in its entirety anyway rather sooner than later), there
are two other call sites. Wouldn't it therefore be more productive
to actually get rid of those?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -627,11 +627,12 @@ void __domain_crash(struct domain *d);
>   * Mark current domain as crashed and synchronously deschedule from the local
>   * processor. This function never returns.
>   */
> -void noreturn __domain_crash_synchronous(void);
> -#define domain_crash_synchronous() do {                                   \
> -    printk("domain_crash_sync called from %s:%d\n", __FILE__, __LINE__);  \
> -    __domain_crash_synchronous();                                         \
> -} while (0)
> +void noreturn __domain_crash_sync(void);
> +#define domain_crash_sync(fmt, args...) do {                            \
> +        printk(XENLOG_G_ERR "domain_crash_sync called from %s: " fmt,   \
> +               __func__, ## args);                                      \
> +        __domain_crash_sync();                                          \
> +    } while (0)

If we really want to keep the functionality, may I then suggest
that you at least avoid retaining name space violation here? E.g.
rename what currently is domain_crash_synchronous() to
domain_crash_sync() as you already do, but rename the backing
__domain_crash_synchronous() to domain_crash_synchronous().

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel