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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 05/04/2019 11:00, Jan Beulich wrote:
> > > > 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.
Sure.

> 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().
It is not like our puts is similar to the C version. As suggested by the man 
page, we don't add a newline when printing.
So I don't see any issue here to extend the number of parameters.

> > @@ -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?
The point of this commit is to avoid treating NUL as end of the buffer. How NU 
will be treated afterwards is up to the rest of the code.
In DomU, this may be added in the output buffer (see below more explanation).

> I
> can't really decide why the isprint() limitation here exists in the first
> place.
Per the discussion on the previous version, escape characters would mess up with 
the console if they are not properly matched. This would result to unreliable 
log and therefore not ideal.
> 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.
I don't see how this is treated differently. This code does exactly what commit 
title state. I.e the NUL does not indicate the end of the buffer anymore.
So I don't see what else can be said in the commit message. Feel free to 
suggest it.

> > -            }
> > +            } 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.
This makes the code just easier to read. You don't have to think whether the 
variable is signed or not. I am not planning to change that unless someone else 
think it is better to avoid "> 0".
> > @@ -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.
I will have a look.

> Jan
--
Julien Grall

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