Re: [Xen-devel] [PATCH 1/3] xen: move debugtrace coding to common/debugtrace.c

[Juergen Gross <jgross@xxxxxxxx>]

On 29.07.19 16:00, Jan Beulich wrote:
> On 29.07.2019 15:30, Juergen Gross wrote:
> > On 29.07.19 14:45, Jan Beulich wrote:
> > > On 28.07.2019 10:40, Juergen Gross wrote:
> > > > -#endif /* !CONFIG_DEBUG_TRACE */
> > > > -
> > > > -
> > > >     /*
> > > >      * **************************************************************
> > > >      * *************** Debugging/tracing/error-report ***************
> > > ... what about this one? There's only panic() between it and the next
> > > such comment, and I don't think the "Debugging/tracing" part of it
> > > are applicable (anymore).
> > True. I'll remove the "Debugging/tracing" part.
> >
> > > > --- a/xen/include/xen/console.h
> > > > +++ b/xen/include/xen/console.h
> > > > @@ -48,4 +48,8 @@ int console_resume(void);
> > > >     extern int8_t opt_console_xen;
> > > > +/* Issue string via serial line. */
> > > > +extern int sercon_handle;
> > > > +void sercon_puts(const char *s);
> > > I guess avoiding their exposure was one of the reasons the debug trace
> > > code lived in the place you move it from. I'm unconvinced non-console
> > > code is actually supposed to make use of either, but I'm not opposed
> > > enough to nak the change. I don't think though the comment fits well
> > > with the variable declaration.
> > sercon_handle is used for calling serial_puts(), so maybe instead of
> > directly using serial_puts() with sercon_handle I should add a wrapper
> > to console.c (e.g. console_serial_puts())? It should be noted that
> > serial_puts() is called only in case of debugtrace output toggled to go
> > to the console. I guess using serial_puts() in that case is meant to
> > avoid too many software layers when doing the output.
> Hmm, I'd rather expect this to be used to avoid doing anything else
> sercon_puts() does besides calling serial_puts(). These other steps
> are also why I think this is to remain a console internal interface.
To me it seems a little bit strange to have the buffer dumping using
sercon_puts() while issuing the actual trace entries to console isn't
using it.

> > It would be
> > possible to use sercon_puts() for that case, too, resulting in the
> > inability to use debugtrace_printk() in the then additionally needed
> > paths (or better: to use it with output redirected to console).
> >
> > sercon_puts() could use another wrapper, e.g. console_debug_puts().
> >
> > Would you like that better?
> Probably not. I wonder whether splitting out this code is really a
> good step.
I'm not fighting for it, I just thought it would better be put into a
file of its own.

In case you disagree and others are not pushing for separation I'm fine
to drop this patch.


Juergen


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