Re: [Xen-devel] [PATCH v5 1/4] xen: fix debugtrace clearing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.09.2019 13:39, Juergen Gross wrote:
> After dumping the debugtrace buffer it is cleared. This results in some
> entries not being printed in case the buffer is dumped again before
> having wrapped.
> 
> While at it remove the trailing zero byte in the buffer as it is no
> longer needed. Commit b5e6e1ee8da59f introduced passing the number of
> chars to be printed in the related interfaces, so the trailing 0 byte
> is no longer required.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

Technically this is fine, so it can have my
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
However, ...

> @@ -1173,6 +1175,7 @@ static char        *debugtrace_buf; /* Debug-trace 
> buffer */
>  static unsigned int debugtrace_prd; /* Producer index     */
>  static unsigned int debugtrace_kilobytes = 128, debugtrace_bytes;
>  static unsigned int debugtrace_used;
> +static char debugtrace_last_entry_buf[DEBUG_TRACE_ENTRY_SIZE];

... this is what I was afraid would happen, but I admit I didn't
reply in a way previously indicating that I dislike such a
solution. This is also why, when noticing the issue, I didn't put
together a patch myself right away. In particular I'm of the
opinion that the three last_* values would better all stay
together, and then would better stay inside the only function
using them.

> @@ -1279,11 +1280,11 @@ void debugtrace_printk(const char *fmt, ...)
>      }
>      else
>      {
> -        if ( strcmp(buf, last_buf) )
> +        if ( strcmp(buf, debugtrace_last_entry_buf) )

Wouldn't moving count to file scope and latching its value into
a new dump_count when dumping work:

        if ( count == dump_count || strcmp(buf, last_buf) )

work?

Jan

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