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

[Glenn Washburn]

On Thu, 31 Aug 2023 20:36:43 +0200
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote:

> I see little value in using enum here and too much compiler variance about
> how it's handled. 

Are you specifically talking about GCC, or the diversity of compilers
that GRUB might be compiled with?

> Compiler is pretty much allowed to choose any type for enum.

If you're talking about GCC, then with the packed attribute this
statement seems false[1].

> If we go this route at all we should at very least do a compile time
> assert.

Sounds like a good idea.

> Ideally I would keep it as-is. C unlike C++ does nothing to enforce
> enum. 

Yes, this is unfortunate. I made this change because I think it looks
nicer. I was thinking that the compiler might do obvious checking to
fail if say a literal value outside the enum range is passed as a
function argument that is an enum type. But you're right it doesn't do
that. Using the enum type then obscures the fact that the boolean type
should be 8 bits as specified by the spec. So typedef'ing as a char,
as is currently done, seems to be a clearer implementation.

I was also hoping that this change would provide a little more detail
for backtraces in GDB, but I've yet to check this.

> Feel free to put GRUB_EFI_TRUE and FALSE into unnamed enum

Yes, I do like this instead of the macros. Now I'm questioning if its
worth the change though.

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

> Le lun. 14 août 2023, 23:39, Glenn Washburn <development@efficientek.com> a
> écrit :
> 
> > 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.
> >
> > 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,
> > +    GRUB_EFI_TRUE
> > +  };
> > +typedef enum grub_efi_boolean grub_efi_boolean_t;
> > +
> >  #if GRUB_CPU_SIZEOF_VOID_P == 8
> >  typedef grub_int64_t grub_efi_intn_t;
> >  typedef grub_uint64_t grub_efi_uintn_t;
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >