Re: [PATCH v2 2/2] ui: add precondition for dpy_get_ui_info()

[Albert Esteve <aesteve@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Tue, Sep 12, 2023 at 8:28 AM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Ensure that it only get called when dpy_ui_info_supported(). The
> function should always return a result. There should be a non-null
> console or active_console.
>
> Modify the argument to be const as well.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/ui/console.h | 2 +-
>  ui/console.c         | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 031e5d5194..08c0f0dc70 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -329,7 +329,7 @@ void
> update_displaychangelistener(DisplayChangeListener *dcl,
>                                    uint64_t interval);
>  void unregister_displaychangelistener(DisplayChangeListener *dcl);
>
> -bool dpy_ui_info_supported(QemuConsole *con);
> +bool dpy_ui_info_supported(const QemuConsole *con);
>  const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con);
>  int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info, bool delay);
>
> diff --git a/ui/console.c b/ui/console.c
> index 0fbec4d0bd..1c710a6d5e 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -803,7 +803,7 @@ static void dpy_set_ui_info_timer(void *opaque)
>      con->hw_ops->ui_info(con->hw, head, &con->ui_info);
>  }
>
> -bool dpy_ui_info_supported(QemuConsole *con)
> +bool dpy_ui_info_supported(const QemuConsole *con)
>  {
>      if (con == NULL) {
>          con = active_console;
> @@ -817,6 +817,8 @@ bool dpy_ui_info_supported(QemuConsole *con)
>
>  const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
>  {
> +    assert(dpy_ui_info_supported(con));
> +
>

I wonder if it would be better to avoid the assertion and return NULL
if not supported, and let the caller handle (logging an error for example).

But there are many other similar assertions in this file, so it is probably
good as it is...

Reviewed-by: Albert Esteve <aesteve@redhat.com>


>      if (con == NULL) {
>          con = active_console;
>      }
> --
> 2.41.0
>
>

[-- Attachment #2: Type: text/html, Size: 3326 bytes --]