Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.04.19 at 18:42, <julien.grall@xxxxxxx> wrote:
> @@ -345,15 +345,15 @@ void console_giveback(int id)
>          serial_steal_fn = NULL;
>  }
>  
> -static void sercon_puts(const char *s)
> +static void sercon_puts(const char *s, unsigned int nr)
>  {
>      if ( serial_steal_fn != NULL )
> -        (*serial_steal_fn)(s);
> +        (*serial_steal_fn)(s, nr);

May I ask that you take the opportunity and drop the unnecessary
decoration as well? A call through a function pointer is fine to make
without * and parentheses.

Also a more general naming related remark: I think this and its
various sibling functions are named *_puts() because of their
similarity to the C library's puts(). I'm not convinced it is a good
move to add a length parameter without also renaming the function
to be less similar to puts().

> @@ -575,19 +572,20 @@ static long 
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
>              char *kin = kbuf, *kout = kbuf, c;
>  
>              /* Strip non-printable characters */
> -            for ( ; ; )
> +            do
>              {
>                  c = *kin++;
> -                if ( c == '\0' || c == '\n' )
> +                if ( c == '\n' )
>                      break;
>                  if ( isprint(c) || c == '\t' )
>                      *kout++ = c;

How does this fit with your goal of handing through nul characters?
isprint('\0') pretty certainly produces false, I would think? I realize
this is the DomU case, but shouldn't we try to treat this similarly? I
can't really decide why the isprint() limitation here exists in the first
place.

In any event, if you mean to treat hwdom and DomU differently,
then I think title and/or description should actually say so, and why.

> -            }
> +            } while ( --kcount > 0 );

Personally I find it odd to use "> 0" or " != 0" on variables of
unsigned type. At least for the latter we indeed commonly omit it,
so I think here and elsewhere the "> 0" would better be omitted in
a similar fashion.

> @@ -1257,14 +1256,15 @@ void debugtrace_printk(const char *fmt, ...)
>      ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>  
>      va_start(args, fmt);
> -    vsnprintf(buf, sizeof(buf), fmt, args);
> +    nr = vscnprintf(buf, sizeof(buf), fmt, args);
>      va_end(args);
>  
>      if ( debugtrace_send_to_console )
>      {
> -        snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
> -        serial_puts(sercon_handle, cntbuf);
> -        serial_puts(sercon_handle, buf);
> +        unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);

To be on the safe side, perhaps better to use scnprintf() here,
just like vscnprintf() gets used above?

> @@ -103,13 +104,14 @@ void lfb_redraw_puts(const char *s)
>  }
>  
>  /* Slower line-based scroll mode which interacts better with dom0. */
> -void lfb_scroll_puts(const char *s)
> +void lfb_scroll_puts(const char *s, unsigned int nr)
>  {
>      unsigned int i;
> -    char c;
>  
> -    while ( (c = *s++) != '\0' )
> +    for ( i = 0; i < nr; i++ )

Note how i is already used in an inner loop.

Jan



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