On Tue, 27 Oct 2020 19:03:36 +0100
Daniel Kiper <dki...@net-space.pl> wrote:
> On Mon, Oct 19, 2020 at 06:09:55PM -0500, Glenn Washburn wrote:
> > This should improve readability of code by providing clues as to
> > what the value represents.
> >
> > Signed-off-by: Glenn Washburn <developm...@efficientek.com>
> > ---
> > grub-core/disk/cryptodisk.c | 12 +++++++-----
> > include/grub/types.h | 3 +++
> > 2 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index 623f0f396..1a91c2d55 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -297,19 +297,21 @@ grub_cryptodisk_endecrypt (struct
> > grub_cryptodisk *dev, }
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> > - iv[1] = grub_cpu_to_le32 (sector >> (32 -
> > log_sector_size));
> > + /* The IV is the 64 bit byte offset of the sector. */
> > + iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BIT(iv[0])
>
> Missing space between macro name and "(".
>
> > + - log_sector_size));
> > iv[0] = grub_cpu_to_le32 ((sector << log_sector_size)
> > - & 0xFFFFFFFF);
> > + & GRUB_TYPE_MAX(iv[0]));
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_BENBI:
> > {
> > grub_uint64_t num = (sector << dev->benbi_log) + 1;
> > - iv[sz - 2] = grub_cpu_to_be32 (num >> 32);
> > - iv[sz - 1] = grub_cpu_to_be32 (num & 0xFFFFFFFF);
> > + iv[sz - 2] = grub_cpu_to_be32 (num >>
> > GRUB_TYPE_BIT(iv[0]));
>
> Ditto.
>
> > + iv[sz - 1] = grub_cpu_to_be32 (num &
> > GRUB_TYPE_MAX(iv[0]));
>
> Ditto.
>
> > }
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_ESSIV:
> > - iv[0] = grub_cpu_to_le32 (sector & 0xFFFFFFFF);
> > + iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_MAX(iv[0]));
>
> Ditto.
>
> > err = grub_crypto_ecb_encrypt (dev->essiv_cipher, iv, iv,
> > dev->cipher->cipher->blocksize);
> > if (err)
> > 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.
> 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.
> 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?
> > +#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.
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
