Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum

[Glenn Washburn]

On Thu, 31 Aug 2023 20:05:37 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Aug 14, 2023 at 04:38:40PM -0500, Glenn Washburn wrote:
> > This allows using GRUB_EFI_TRUE and GRUB_EFI_FALSE with proper type and
> > value checking. The UEFI 2.10 specification, in section 2.3.1, table 2.3,
> > says the size of the boolean is 1 byte and may only contain the values 0 or
> > 1. In order to have the enum be 1-byte in size instead of the default
> > int-sized, add the packed attribute to the enum.
> 
> Hmmm... Could you point out us an official doc saying "packed" attribute
> does that?

This has been around since at least gcc 3.3, but this is link for a
more current gcc[1] (search for "packed").

> And I hope you tested that change because we use grub_efi_boolean_t type
> quite often...

I've been using this change for a couple months now with no issues.
Also, no tests are failing because of this change, although this might
not be a great recommendation. I want to do some more testing, just to
be sure.

> 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  include/grub/efi/api.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> > index 5934db1c992b..be7c128dfb42 100644
> > --- a/include/grub/efi/api.h
> > +++ b/include/grub/efi/api.h
> > @@ -552,7 +552,13 @@ enum grub_efi_reset_type
> >  typedef enum grub_efi_reset_type grub_efi_reset_type_t;
> >
> >  /* Types.  */
> > -typedef char grub_efi_boolean_t;
> > +enum GRUB_PACKED grub_efi_boolean
> > +  {
> > +    GRUB_EFI_FALSE,
> 
> I would explicitly do
> 
>   GRUB_EFI_FALSE = 0,

Good idea.

> 
> > +    GRUB_EFI_TRUE
> > +  };
> 
> I would move GRUB_PACKED here to be inline with its other uses.

The following from the GCC manual has me questioning why we do this:
"You can also place [attributes] just past the closing curly brace of
the definition, but this is less preferred because logically the type
should be fully defined at the closing brace."[2] I'll change this in
the next iteration for consistency, but perhaps we should reconsider
the policy?

Glenn

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html
[2] https://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html