Re: [PATCH] gfxmenu: resolve false grub_errno disrupting boot process

From: Daniel Kiper <dkiper@net-space.pl>
To: Michael Chang <mchang@suse.com>
Cc: grub-devel@gnu.org
Subject: Re: [PATCH] gfxmenu: resolve false grub_errno disrupting boot process
Date: Thu, 22 Feb 2024 15:57:26 +0100	[thread overview]
Message-ID: <20240222145726.6lb4vofciuow62vt@tomti.i.net-space.pl> (raw)
In-Reply-To: <20240219032911.12730-1-mchang@suse.com>

On Mon, Feb 19, 2024 at 11:29:11AM +0800, Michael Chang via Grub-devel wrote:
> When enabling gfxmenu and choosing to boot the Xen hypervisor from its
> menu, an error occurred:
>
> "error: ../../grub-core/video/bitmap_scale.c:42:null src bitmap in
> grub_video_create_scaled."
>
> The error is returned by grub_video_bitmap_create_scaled() when the
> source pixmap is not there. The init_background() uses it to scale up
> the background image so it can fully fit into the screen resolution.
>
> However not all backgrounds are set by a image, ie the "desktop-image"
> property of the theme file. Instead a color code may be used, for
> example openSUSE's green background uses "desktop-color" property:
>
>  desktop-color: "#0D202F"
>
> So it is absolutely fine to call init_background() without a raw pixmap
> if color code is used. A missing check has to be added to ensure the
> grub_errno will not be erroneously set and gets in the way of ensuing
> boot process.
>
> The reason it happens sporadically is due to grub_errno is reset to
> GRUB_ERR_NONE in other places if a function's error return can be
> ignored. In particular this hunk in grub_gfxmenu_create_box() does the
> majority of the reset of grub_errno returned by init_background(), but
> the path may not be always chosen.
>
>     grub_video_bitmap_load (&box->raw_pixmaps[i], path);
>     grub_free (path);
>
>     /* Ignore missing pixmaps.  */
>     grub_errno = GRUB_ERR_NONE;
>
> In any case, we cannot account on such random behavior and should only
> return grub_errno if it is justified.
>
> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>  grub-core/gfxmenu/view.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/grub-core/gfxmenu/view.c b/grub-core/gfxmenu/view.c
> index 6358004b2..8ed3fd1e6 100644
> --- a/grub-core/gfxmenu/view.c
> +++ b/grub-core/gfxmenu/view.c
> @@ -556,6 +556,11 @@ init_background (grub_gfxmenu_view_t view)
>    if (view->scaled_desktop_image)
>      return;
>
> +  /* You don't have to scale a raw image if it's not present; this prevents
> +   * setting grub_errno and disrupting a command's execution. */
> +  if (view->raw_desktop_image == NULL)
> +    return;

I think it would be more logical to put this check before view->scaled_desktop_image check.
Though Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...
I will fix it myself before push...

> +
>    struct grub_video_bitmap *scaled_bitmap;
>    if (view->desktop_image_scale_method ==
>        GRUB_VIDEO_BITMAP_SELECTION_METHOD_STRETCH)

Thanks,

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel