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

Commit Message

Denis Mukhin via B4 Relay Dec. 6, 2024, 4:41 a.m. UTC

From: Denis Mukhin <[e-mail]>

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.

Signed-off-by: Denis Mukhin <[e-mail]>
---
 xen/arch/arm/vuart.c       | 3 +--
 xen/arch/x86/hvm/hvm.c     | 3 +--
 xen/drivers/char/console.c | 2 +-
 xen/include/xen/ctype.h    | 3 +++
 4 files changed, 6 insertions(+), 5 deletions(-)

Comments

Jan Beulich Dec. 10, 2024, 1:22 p.m. UTC | #1

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
...

> --- 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().

> --- 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?

> @@ -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.

Jan

Patch

diff --git a/xen/arch/arm/vuart.c b/xen/arch/arm/vuart.c
index d5ba483f1e63245e545346ad5045098152b8c152..ac76e2327bb84f05ea5716c6f5550f94812d2827 100644
--- 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 ;
 
     spin_lock(&uart->lock);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 74e58c653e6f697e7e563fd076bbbafaf257137d..493b699c708949b2109c26573a107565543f5d45 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -561,8 +561,7 @@  static int cf_check hvm_print_line(
     if ( dir != IOREQ_WRITE )
         return X86EMUL_UNHANDLEABLE;
 
-    /* Accept only printable characters, newline, and horizontal tab. */
-    if ( !isprint(c) && (c != '\n') && (c != '\t') )
+    if ( !isconsole(c) )
         return X86EMUL_OKAY;
 
     spin_lock(&cd->pbuf_lock);
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 7da8c5296f3b62c6c45131c58fe5cf0e393e9ef3..bb56953bab681a13da8d41431aba4632f1919df9 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -674,7 +674,7 @@  static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
                 c = *kin++;
                 if ( c == '\n' )
                     break;
-                if ( isprint(c) || c == '\t' )
+                if ( isconsole(c) )
                     *kout++ = c;
             } while ( --kcount > 0 );
 
diff --git a/xen/include/xen/ctype.h b/xen/include/xen/ctype.h
index 773ac27aa44ac65e76e87cdec960450804310249..deebaad96ede34f16f61ece862c788232c1d1efd 100644
--- 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.
  */
 
 #define _U	0x01	/* upper */
@@ -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')
 
 #define isascii(c) (((unsigned char)(c))<=0x7f)
 #define toascii(c) (((unsigned char)(c))&0x7f)