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

[Glenn Washburn]

On Sat, 2 Sep 2023 20:45:34 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Aug 31, 2023 at 11:58:42PM -0500, Glenn Washburn wrote:
> > 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").
> 
> Does it work on clang? I, as Valdimir, am afraid we are entering into
> shakey grounds...

I didn't realize that clang was the concern. No I've not tested with
clang and not setup to test it right now. So I'll table this for now.

> > > 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?
> 
> I am OK with doing this cleanup but after the release...

Sounds good. I think its pretty low priority.

Glenn