[v3,01/24] xen/ctype: introduce is_console_printable()

Commit Message

Denis Mukhin via B4 Relay Jan. 4, 2025, 1:58 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 is_console_printable() function and
re-used in the UART emulation / guest logging code.

Also, MISRA rule 21.13 for ctype.h has been exploited while working on
the code change, reference the rule from ctype.h for future engineers.

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    | 7 +++++++
 4 files changed, 10 insertions(+), 5 deletions(-)

Comments

Jason Andryuk Jan. 21, 2025, 10:56 p.m. UTC | #1

On 2025-01-03 20:58, Denis Mukhin via B4 Relay wrote:
> 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 is_console_printable() function and
> re-used in the UART emulation / guest logging code.
> 
> Also, MISRA rule 21.13 for ctype.h has been exploited while working on
> the code change, reference the rule from ctype.h for future engineers.
> 
> Signed-off-by: Denis Mukhin <[e-mail]>

> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 7da8c5296f3b62c6c45131c58fe5cf0e393e9ef3..4cb397116b44935214801c496b30e44c9399c59a 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 ( is_console_printable(c) )
>                       *kout++ = c;

This `if` now accepts newline, but newline is already handled above.  So 
it seems okay to me.

Reviewed-by: Jason Andryuk <[e-mail]>

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

Jan Beulich Jan. 22, 2025, 7:19 a.m. UTC | #2

On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> --- 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.
>   */

As previously indicated, I object to such comments. I think I said so before:
_All_ Misra rules are relevant _everywhere_ anyway.

> @@ -51,4 +53,9 @@ static inline unsigned char __toupper(unsigned char c)
>  #define tolower(c) ((char)__tolower(c))
>  #define toupper(c) ((char)__toupper(c))
>  
> +static inline unsigned is_console_printable(unsigned char c)
> +{
> +	return isprint(c) || c == '\n' || c == '\t';
> +}

Why a return type of unsigned (and then not even "unsigned int")? I can't
spot anything in the file which would serve as a reference for this, and
by its nature the function clearly wants to return bool.

I further question the placement of this function in ctype.h: Only console
related code cares about this function, so exposure is far too wide this
way.

Jan

Denis Mukhin Jan. 23, 2025, 9:11 p.m. UTC | #3

On Tuesday, January 21st, 2025 at 11:19 PM, Jan Beulich <[e-mail]> wrote:

> 
> 
> On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> 
> > --- 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.
> > */
> 
> 
> As previously indicated, I object to such comments. I think I said so before:
> All Misra rules are relevant everywhere anyway.

Correct, I received that feedback in v2 comments, but after I posted v3
on the mailing list.

That will be addressed in the next iteration.

> 
> > @@ -51,4 +53,9 @@ static inline unsigned char __toupper(unsigned char c)
> > #define tolower(c) ((char)__tolower(c))
> > #define toupper(c) ((char)__toupper(c))
> > 
> > +static inline unsigned is_console_printable(unsigned char c)
> > +{
> > + return isprint(c) || c == '\n' || c == '\t';
> > +}
> 
> 
> Why a return type of unsigned (and then not even "unsigned int")? I can't
> spot anything in the file which would serve as a reference for this, and
> by its nature the function clearly wants to return bool.
> 
> I further question the placement of this function in ctype.h: Only console
> related code cares about this function, so exposure is far too wide this
> way.

I will move that to console.h in v4.
Thanks.

> 
> Jan

Denis Mukhin Feb. 7, 2025, 1:01 a.m. UTC | #4

On Thursday, January 23rd, 2025 at 1:11 PM, Denis Mukhin <[e-mail]> wrote:

> 
> 
> On Tuesday, January 21st, 2025 at 11:19 PM, Jan Beulich [e-mail] wrote:
> 
> > On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> > 
> > > --- 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.
> > > */
> > 
> > As previously indicated, I object to such comments. I think I said so before:
> > All Misra rules are relevant everywhere anyway.
> 
> 
> Correct, I received that feedback in v2 comments, but after I posted v3
> on the mailing list.
> 
> That will be addressed in the next iteration.

Moved to 
  https://lore.kernel.org/xen-devel/[e-mail]

> 
> > > @@ -51,4 +53,9 @@ static inline unsigned char __toupper(unsigned char c)
> > > #define tolower(c) ((char)__tolower(c))
> > > #define toupper(c) ((char)__toupper(c))
> > > 
> > > +static inline unsigned is_console_printable(unsigned char c)
> > > +{
> > > + return isprint(c) || c == '\n' || c == '\t';
> > > +}
> > 
> > Why a return type of unsigned (and then not even "unsigned int")? I can't
> > spot anything in the file which would serve as a reference for this, and
> > by its nature the function clearly wants to return bool.
> > 
> > I further question the placement of this function in ctype.h: Only console
> > related code cares about this function, so exposure is far too wide this
> > way.
> 
> 
> I will move that to console.h in v4.
> Thanks.
> 
> > Jan

Patch

diff --git a/xen/arch/arm/vuart.c b/xen/arch/arm/vuart.c
index d5ba483f1e63245e545346ad5045098152b8c152..98a65b99385a2a119725bab8634ed7cf9d926d68 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 ( !is_console_printable(c) )
         return ;
 
     spin_lock(&uart->lock);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 922c9b3af64d9132022d37627c54af092275e9cf..c4f1df248c1a7b2b3e5c45cef154e7ca80018dfc 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 ( !is_console_printable(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..4cb397116b44935214801c496b30e44c9399c59a 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 ( is_console_printable(c) )
                     *kout++ = c;
             } while ( --kcount > 0 );
 
diff --git a/xen/include/xen/ctype.h b/xen/include/xen/ctype.h
index 773ac27aa44ac65e76e87cdec960450804310249..ceb8f028ddc80b3b688f13422c0362199a03ba9e 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 */
@@ -51,4 +53,9 @@  static inline unsigned char __toupper(unsigned char c)
 #define tolower(c) ((char)__tolower(c))
 #define toupper(c) ((char)__toupper(c))
 
+static inline unsigned is_console_printable(unsigned char c)
+{
+	return isprint(c) || c == '\n' || c == '\t';
+}
+
 #endif