Re: [PATCH v7 09/17] cryptodisk: Add macros GRUB_TYPE_U_MAX/MIN(type) to replace literals

[Glenn Washburn]

On Mon, 7 Dec 2020 21:22:09 +0100
Daniel Kiper  wrote:

> On Fri, Dec 04, 2020 at 10:43:38AM -0600, Glenn Washburn wrote:
> > Add GRUB_TYPE_U_MAX/MIN(type) macros to get the max/min values for
> > an unsigned number with size of type.
> >
> > Signed-off-by: Glenn Washburn 
> 
> Reviewed-by: Daniel Kiper 
> 
> But one nit below...
> 
> > ---
> >  grub-core/disk/cryptodisk.c | 8 
> >  include/grub/types.h| 7 +++
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index 0e955a020..5aa0c4720 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -284,23 +284,23 @@ grub_cryptodisk_endecrypt (struct
> > grub_cryptodisk *dev, iv[1] = grub_cpu_to_le32 (sector >>
> > GRUB_TYPE_BITS (iv[0])); /* FALLTHROUGH */
> > case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> > - iv[0] = grub_cpu_to_le32 (sector & 0x);
> > + iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX
> > (iv[0])); break;
> > case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> >   iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BITS
> > (iv[1])
> >-
> > dev->log_sector_size)); iv[0] = grub_cpu_to_le32 ((sector <<
> > dev->log_sector_size)
> > -   & 0x);
> > +   & GRUB_TYPE_U_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 >> GRUB_TYPE_BITS
> > (iv[0]));
> > -   iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
> > +   iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_U_MAX
> > (iv[0])); }
> >   break;
> > case GRUB_CRYPTODISK_MODE_IV_ESSIV:
> > - iv[0] = grub_cpu_to_le32 (sector & 0x);
> > + iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX
> > (iv[0])); 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 9989e3a16..0542011cc 100644
> > --- a/include/grub/types.h
> > +++ b/include/grub/types.h
> > @@ -161,6 +161,13 @@ typedef grub_int32_t   grub_ssize_t;
> >  #endif
> >  # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1)
> >
> > +/*
> > +  Cast to unsigned long long so that the "return value" is always
> > a consistent
> > +  type and to ensure an unsigned value regardless of type
> > parameter.
> > + */
> 
> Incorrect comment formatting...

I'll fix it, and get it right one of these days.

> > +#define GRUB_TYPE_U_MAX(type) ((unsigned long long)((typeof
> > (type))(~0))) +#define GRUB_TYPE_U_MIN(type) 0ULL
> > +
> >  typedef grub_uint64_t grub_properly_aligned_t;
> >
> >  #define GRUB_PROPERLY_ALIGNED_ARRAY(name, size)
> > grub_properly_aligned_t name[((size) + sizeof
> > (grub_properly_aligned_t) - 1) / sizeof (grub_properly_aligned_t)]
> 
> Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v7 09/17] cryptodisk: Add macros GRUB_TYPE_U_MAX/MIN(type) to replace literals

[Daniel Kiper]

On Fri, Dec 04, 2020 at 10:43:38AM -0600, Glenn Washburn wrote:
> Add GRUB_TYPE_U_MAX/MIN(type) macros to get the max/min values for an
> unsigned number with size of type.
>
> Signed-off-by: Glenn Washburn 

Reviewed-by: Daniel Kiper 

But one nit below...

> ---
>  grub-core/disk/cryptodisk.c | 8 
>  include/grub/types.h| 7 +++
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 0e955a020..5aa0c4720 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -284,23 +284,23 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> iv[1] = grub_cpu_to_le32 (sector >> GRUB_TYPE_BITS (iv[0]));
> /* FALLTHROUGH */
>   case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> -   iv[0] = grub_cpu_to_le32 (sector & 0x);
> +   iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX (iv[0]));
> break;
>   case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BITS (iv[1])
>  - dev->log_sector_size));
> iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
> - & 0x);
> + & GRUB_TYPE_U_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 >> GRUB_TYPE_BITS (iv[0]));
> - iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
> + iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_U_MAX (iv[0]));
> }
> break;
>   case GRUB_CRYPTODISK_MODE_IV_ESSIV:
> -   iv[0] = grub_cpu_to_le32 (sector & 0x);
> +   iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX (iv[0]));
> 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 9989e3a16..0542011cc 100644
> --- a/include/grub/types.h
> +++ b/include/grub/types.h
> @@ -161,6 +161,13 @@ typedef grub_int32_t grub_ssize_t;
>  #endif
>  # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1)
>
> +/*
> +  Cast to unsigned long long so that the "return value" is always a 
> consistent
> +  type and to ensure an unsigned value regardless of type parameter.
> + */

Incorrect comment formatting...

> +#define GRUB_TYPE_U_MAX(type) ((unsigned long long)((typeof (type))(~0)))
> +#define GRUB_TYPE_U_MIN(type) 0ULL
> +
>  typedef grub_uint64_t grub_properly_aligned_t;
>
>  #define GRUB_PROPERLY_ALIGNED_ARRAY(name, size) grub_properly_aligned_t 
> name[((size) + sizeof (grub_properly_aligned_t) - 1) / sizeof 
> (grub_properly_aligned_t)]

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v7 09/17] cryptodisk: Add macros GRUB_TYPE_U_MAX/MIN(type) to replace literals

[Patrick Steinhardt]

On Fri, Dec 04, 2020 at 10:43:38AM -0600, Glenn Washburn wrote:
> Add GRUB_TYPE_U_MAX/MIN(type) macros to get the max/min values for an
> unsigned number with size of type.
> 
> Signed-off-by: Glenn Washburn 
> ---
>  grub-core/disk/cryptodisk.c | 8 
>  include/grub/types.h| 7 +++
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 0e955a020..5aa0c4720 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -284,23 +284,23 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> iv[1] = grub_cpu_to_le32 (sector >> GRUB_TYPE_BITS (iv[0]));
> /* FALLTHROUGH */
>   case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> -   iv[0] = grub_cpu_to_le32 (sector & 0x);
> +   iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX (iv[0]));
> break;
>   case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BITS (iv[1])
>  - dev->log_sector_size));
> iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
> - & 0x);
> + & GRUB_TYPE_U_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 >> GRUB_TYPE_BITS (iv[0]));
> - iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
> + iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_U_MAX (iv[0]));
> }
> break;
>   case GRUB_CRYPTODISK_MODE_IV_ESSIV:
> -   iv[0] = grub_cpu_to_le32 (sector & 0x);
> +   iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX (iv[0]));
> 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 9989e3a16..0542011cc 100644
> --- a/include/grub/types.h
> +++ b/include/grub/types.h
> @@ -161,6 +161,13 @@ typedef grub_int32_t grub_ssize_t;
>  #endif
>  # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1)
>  
> +/*
> +  Cast to unsigned long long so that the "return value" is always a 
> consistent
> +  type and to ensure an unsigned value regardless of type parameter.
> + */
> +#define GRUB_TYPE_U_MAX(type) ((unsigned long long)((typeof (type))(~0)))
> +#define GRUB_TYPE_U_MIN(type) 0ULL
> +

Nit: multi-line comments do use the following format:

/*
 * Foo bar
 * bar foo
 /

Other than that:

Reviewed-by: Patrick Steinhardt 

>  typedef grub_uint64_t grub_properly_aligned_t;
>  
>  #define GRUB_PROPERLY_ALIGNED_ARRAY(name, size) grub_properly_aligned_t 
> name[((size) + sizeof (grub_properly_aligned_t) - 1) / sizeof 
> (grub_properly_aligned_t)]
> -- 
> 2.27.0
> 


signature.asc
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v7 09/17] cryptodisk: Add macros GRUB_TYPE_U_MAX/MIN(type) to replace literals

[Glenn Washburn]

Add GRUB_TYPE_U_MAX/MIN(type) macros to get the max/min values for an
unsigned number with size of type.

Signed-off-by: Glenn Washburn 
---
 grub-core/disk/cryptodisk.c | 8 
 include/grub/types.h| 7 +++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 0e955a020..5aa0c4720 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -284,23 +284,23 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
  iv[1] = grub_cpu_to_le32 (sector >> GRUB_TYPE_BITS (iv[0]));
  /* FALLTHROUGH */
case GRUB_CRYPTODISK_MODE_IV_PLAIN:
- iv[0] = grub_cpu_to_le32 (sector & 0x);
+ iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX (iv[0]));
  break;
case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
  iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BITS (iv[1])
   - dev->log_sector_size));
  iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
-   & 0x);
+   & GRUB_TYPE_U_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 >> GRUB_TYPE_BITS (iv[0]));
-   iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
+   iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_U_MAX (iv[0]));
  }
  break;
case GRUB_CRYPTODISK_MODE_IV_ESSIV:
- iv[0] = grub_cpu_to_le32 (sector & 0x);
+ iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX (iv[0]));
  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 9989e3a16..0542011cc 100644
--- a/include/grub/types.h
+++ b/include/grub/types.h
@@ -161,6 +161,13 @@ typedef grub_int32_t   grub_ssize_t;
 #endif
 # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1)
 
+/*
+  Cast to unsigned long long so that the "return value" is always a consistent
+  type and to ensure an unsigned value regardless of type parameter.
+ */
+#define GRUB_TYPE_U_MAX(type) ((unsigned long long)((typeof (type))(~0)))
+#define GRUB_TYPE_U_MIN(type) 0ULL
+
 typedef grub_uint64_t grub_properly_aligned_t;
 
 #define GRUB_PROPERLY_ALIGNED_ARRAY(name, size) grub_properly_aligned_t 
name[((size) + sizeof (grub_properly_aligned_t) - 1) / sizeof 
(grub_properly_aligned_t)]
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel