Re: [PATCH v3 07/10] cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt.

[Daniel Kiper]

On Thu, Oct 29, 2020 at 04:51:37PM -0500, Glenn Washburn wrote:
> On Tue, 27 Oct 2020 19:03:36 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Mon, Oct 19, 2020 at 06:09:55PM -0500, Glenn Washburn wrote:

[...]

> > > diff --git a/include/grub/types.h b/include/grub/types.h
> > > index 035a4b528..8b4267ebd 100644
> > > --- a/include/grub/types.h
> > > +++ b/include/grub/types.h
> > > @@ -319,4 +319,7 @@ static inline void grub_set_unaligned64 (void
> > > *ptr, grub_uint64_t val)
> > >
> > >  #define GRUB_CHAR_BIT 8
> >
> > I think we should not change the name of this constant. It seems to
> > me that it was named in similar way to C/gcc/clang CHAR_BIT and/or
> > __CHAR_BIT__. However, I think we should change its definition. It
> > should be
> >
> >   #define GRUB_CHAR_BIT __CHAR_BIT__
>
> I can add that into this commit.

It has to be separate commit because it is logically different change.

> > Additionally, the build should stop if __CHAR_BIT__ is not defined nor
> > GRUB_CHAR_BIT is not equal 8.
>
> I'm not sure exactly how you want that done.

#ifndef __CHAR_BIT__
#error __CHAR_BIT__ is not defined
#else __CHAR_BIT__ != 8
#error __CHAR_BIT__ is not equal 8
#else
#define GRUB_CHAR_BIT __CHAR_BIT__
#endif

> > Last but not least, the GRUB_CHAR_BIT definition should be right
> > behind includes in the include/grub/types.h.
> >
> > > +#define GRUB_TYPE_BIT(type) (sizeof(type) * GRUB_CHAR_BIT)
> >
> > I agree with Patrick, it should be named GRUB_TYPE_BITS.
>
> Patrick says my logic makes sense for him.  I can still change it, yes?

Ugh... I missed his last email somehow. Sorry about that. I still think
it should be GRUB_TYPE_BITS(). So, please rename GRUB_TYPE_BIT() to
GRUB_TYPE_BITS(). GRUB_CHAR_BIT name should stay as is.

> > > +#define GRUB_TYPE_MAX(type) ((2 * ((1ULL << (GRUB_TYPE_BIT(type) -
> > > 1)) - 1)) + 1)
> >
> > This only works for unsigned types. So, I think we should have three
> > (four?) macros here: (GRUB_TYPE_U_MIN? (which is obviously 0)),
> > GRUB_TYPE_U_MAX, GRUB_TYPE_S_MIN and GRUB_TYPE_S_MAX.
>
> How about I just rename GRUB_TYPE_MAX to GRUB_TYPE_U_MAX and someone
> (you?) can add in the rest?  I'm not sure how I'd do the signed
> variants that works across all architectures.

I am not going to insist on adding GRUB_TYPE_S_MIN/GRUB_TYPE_S_MAX if
you think it is complicated. Though please rename GRUB_TYPE_MAX to
GRUB_TYPE_U_MAX.

Daniel