RE: [PATCH v1 03/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, March 20, 2025 4:47 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; 
> Daniel
> P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>; Alejandro Vallejo
> <alejandro.vallejo@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 03/19] xen/sysctl: wrap around
> XEN_SYSCTL_readconsole
>
> On 20.03.2025 09:02, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> >> Sent: Thursday, March 13, 2025 8:03 PM
> >>
> >> Ok, so readconsole is done here. I see how if you're also removing
> >> the console handler for the sysctl that's a bit unwiledly to do in one go.
> >>
> >> I think my earlier remarks still hold in terms of removal of else branches 
> >> of ifdefs.
> >>
> >> On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
> >>> The following functions is to deal with XEN_SYSCTL_readconsole
> >>> sub-op, and shall be wrapped:
> >>> - xsm_readconsole
> >>> - read_console_ring
> >>>
> >>> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> >>> ---
> >>>  xen/drivers/char/console.c |  2 ++
> >>>  xen/include/xen/console.h  |  8 ++++++++
> >>>  xen/include/xsm/dummy.h    | 11 ++++++++---
> >>>  xen/include/xsm/xsm.h      | 11 ++++++++---
> >>>  xen/xsm/dummy.c            |  2 +-
> >>>  xen/xsm/flask/hooks.c      |  4 ++--
> >>>  6 files changed, 29 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> >>> index 2f028c5d44..6e4f3c4659 100644
> >>> --- a/xen/drivers/char/console.c
> >>> +++ b/xen/drivers/char/console.c
> >>> @@ -336,6 +336,7 @@ static void conring_puts(const char *str, size_t len)
> >>>          conringc = conringp - conring_size;  }
> >>>
> >>> +#ifdef CONFIG_SYSCTL
> >>>  long read_console_ring(struct xen_sysctl_readconsole *op)  {
> >>>      XEN_GUEST_HANDLE_PARAM(char) str; @@ -378,6 +379,7 @@ long
> >>> read_console_ring(struct xen_sysctl_readconsole *op)
> >>>
> >>>      return 0;
> >>>  }
> >>> +#endif /* CONFIG_SYSCTL */
> >>>
> >>>
> >>>  /*
> >>> diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> >>> index 83cbc9fbda..e7d5063d82 100644
> >>> --- a/xen/include/xen/console.h
> >>> +++ b/xen/include/xen/console.h
> >>> @@ -7,12 +7,20 @@
> >>>  #ifndef __CONSOLE_H__
> >>>  #define __CONSOLE_H__
> >>>
> >>> +#include <xen/errno.h>
> >>>  #include <xen/inttypes.h>
> >>>  #include <xen/ctype.h>
> >>>  #include <public/xen.h>
> >>>
> >>>  struct xen_sysctl_readconsole;
> >>
> >> That forward declaration should probably be inside the ifdef
> >>
> >>> +#ifdef CONFIG_SYSCTL
> >>>  long read_console_ring(struct xen_sysctl_readconsole *op);
> >>> +#else
> >>> +static inline long read_console_ring(struct xen_sysctl_readconsole
> >>> +*op) {
> >>> +    return -EOPNOTSUPP;
> >>> +}
> >>> +#endif
> >>
> >> This is only called from sysctl.c, which will be compiled out. Why is
> >> the else needed?
> >>
> >
> > Because I wrapped the sysctl.c in the last commit.
> > If removing the else condition here, the compilation will fail on this 
> > commit.
> > So either I add #ifdef into read_console_ring function body, or in the
> > last commit, I draw back all these unnecessary else conditions, or
> > combine all commits into one Any preference? Or any other suggestion?
>
> Munging everything in a single commit may yield unwieldily big a result. 
> Transiently
> adding #ifdef in sysctl.c would seem like the cleanest approach to me. In the 
> final
> commit it'll (hopefully) be obvious enough then why all of the #ifdef-s are 
> dropped
> again.
>

Understood, I'll add #ifdef in sysctl.c and remove them all in the last, and 
also add
description why we drop them in the commit message

> Jan