On Thu, Aug 31, 2023 at 11:58:42PM -0500, Glenn Washburn wrote:
> On Thu, 31 Aug 2023 20:05:37 +0200
> Daniel Kiper <dki...@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...
> > 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 <developm...@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...
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
