Re: [PATCH v2 03/35] xen/ctype: introduce isconsole()

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Tuesday, December 10th, 2024 at 5:22 AM, Jan Beulich <jbeulich@xxxxxxxx> 
wrote:

>
>
> On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote:
>
> > There are several console drivers which have same checks w.r.t. printable
> > characters. The check is moved to new isconsole() macro and re-used in
> > the console drivers.
>
>
> Something named isconsole() imo can't be expected to do what is checked for

I tried to follow the "short" naming notation in ctype.h.
I agree; changed the name to is_console_printable() as per below suggestion.

> ...
>
> > --- a/xen/arch/arm/vuart.c
> > +++ b/xen/arch/arm/vuart.c
> > @@ -79,8 +79,7 @@ static void vuart_print_char(struct vcpu *v, char c)
> > struct domain *d = v->domain;
> > struct vuart *uart = &d->arch.vuart;
> >
> > - /* Accept only printable characters, newline, and horizontal tab. */
> > - if ( !isprint(c) && (c != '\n') && (c != '\t') )
> > + if ( !isconsole(c) )
> > return ;
>
>
> ... e.g. here. If we really want such a further abstraction (of which I'm
> unconvinced), then maybe isprintable() or (getting ling-ish)
> is_console_printable().

Reworked to is_console_printable()

>
> > --- a/xen/include/xen/ctype.h
> > +++ b/xen/include/xen/ctype.h
> > @@ -4,6 +4,8 @@
> > /*
> > * NOTE! This ctype does not handle EOF like the standard C
> > * library is required to.
> > + *
> > + * See Rule 21.13 in docs/misra/rules.rst.
> > */
>
>
> How's this change related to the purpose of the patch?

Only because the very first version of the macro was failing
an ECLAIR job for me because of Rule 21.13 violation.

Updated the commit message (v3).

>
> > @@ -30,6 +32,7 @@ extern const unsigned char _ctype[];
> > #define isspace(c) ((__ismask(c)&(_S)) != 0)
> > #define isupper(c) ((__ismask(c)&(_U)) != 0)
> > #define isxdigit(c) ((__ismask(c)&(_D|_X)) != 0)
> > +#define isconsole(c) (isprint(c) || (c) == '\n' || (c) == '\t')
>
>
> In a pretty general purpose macro like this one I'm afraid I view it as
> risky to evaluate the parameter more than once.

Fixed.

>
> Jan