[PATCH v4 00/13] error: Do compile-time format string checking on grub_error

[PATCH v4 00/13] error: Do compile-time format string checking on grub_error

[Glenn Washburn]

This patch series fixes all compile errors due to format string issues on
grub_error. This was tested against nearly all supported platforms
successfully. This is important because earlier versions of these changes
compiled successfully on x86 platforms, but had issues on other ones not
tested.

All patches except the last fix actual format string issues. The last patch
turns format string issues into errors. This is a good idea because it will
help to prevent introducing new format string issues into the code. Since, I
presume, Daniel does not do not do a build test for all architectures before
committing to master, this will not ensure that no format string issues get
introduced into the code. However, it should flush out any format string
issues when the CI build is done.

Many of these changes are fairly obvious. I tried to use the PRI*GRUB_*_T
macros as much as I could and to not cast arguments. There are some notable
exceptions. There is some code that uses the same grub_error code in both
32 and 64 bit architectures (riscv), so casting was needed to force the larger
storage type. The second to last patch for zfs is still confounding to me
as to why the macro PRIuGRUB_UINT64_T was not expanding to llu, and yet the
compiler was saying the argument was a long long unsigned.

The tests results can be found here:
  https://gitlab.com/gwashburn/grub/-/pipelines/255133408

Glenn

Glenn Washburn (13):
  misc: Format string for grub_error should be a literal
  error: grub_error missing format string argument
  error: grub_error format string add missing format code
  dmraid_nvidia: Format string error in grub_error
  grub_error: Use format code PRIuGRUB_SIZE for variables of type
    grub_size_t
  pgp: Format code for grub_error is incorrect
  efi: Format string error in grub_error
  error: Use %p format code for pointer types
  error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in
    grub_error
  error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error
  error: Use format code PRIuGRUB_UINT64_T for 64-bit typed fileblock in
    grub_error
  error: Use format code llu for 64-bit uint bp->blk_prop in grub_error
  error: Do compile-time format string checking on grub_error

 grub-core/commands/pgp.c           |  2 +-
 grub-core/disk/ata.c               |  4 ++--
 grub-core/disk/cryptodisk.c        | 12 ++++++++----
 grub-core/disk/dmraid_nvidia.c     |  2 +-
 grub-core/efiemu/i386/loadcore64.c |  3 ++-
 grub-core/fs/hfsplus.c             |  3 ++-
 grub-core/fs/zfs/zfs.c             |  4 ++--
 grub-core/kern/arm64/dl.c          |  3 ++-
 grub-core/kern/efi/efi.c           |  2 +-
 grub-core/kern/efi/mm.c            |  5 +++--
 grub-core/kern/ia64/dl.c           |  3 ++-
 grub-core/kern/riscv/dl.c          |  5 +++--
 grub-core/kern/sparc64/dl.c        |  3 ++-
 grub-core/kern/x86_64/dl.c         |  3 ++-
 grub-core/loader/efi/chainloader.c |  4 ++--
 grub-core/loader/i386/bsd.c        |  3 ++-
 grub-core/loader/i386/pc/linux.c   |  4 ++--
 grub-core/net/tftp.c               |  2 +-
 grub-core/parttool/msdospart.c     |  4 ++--
 grub-core/script/lexer.c           |  2 +-
 grub-core/video/bochs.c            |  4 ++--
 include/grub/err.h                 |  3 ++-
 22 files changed, 47 insertions(+), 33 deletions(-)

-- 
2.27.0

[PATCH v4 01/13] misc: Format string for grub_error should be a literal

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/loader/efi/chainloader.c | 2 +-
 grub-core/net/tftp.c               | 2 +-
 grub-core/script/lexer.c           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index 7b31c3fb9..2e98947a0 100644
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -90,7 +90,7 @@ grub_chainloader_boot (void)
 	      *grub_utf16_to_utf8 ((grub_uint8_t *) buf,
 				   exit_data, exit_data_size) = 0;
 
-	      grub_error (GRUB_ERR_BAD_OS, buf);
+	      grub_error (GRUB_ERR_BAD_OS, "%s", buf);
 	      grub_free (buf);
 	    }
 	}
diff --git a/grub-core/net/tftp.c b/grub-core/net/tftp.c
index 7e659a78b..592bfee34 100644
--- a/grub-core/net/tftp.c
+++ b/grub-core/net/tftp.c
@@ -252,7 +252,7 @@ tftp_receive (grub_net_udp_socket_t sock __attribute__ ((unused)),
     case TFTP_ERROR:
       data->have_oack = 1;
       grub_netbuff_free (nb);
-      grub_error (GRUB_ERR_IO, (char *) tftph->u.err.errmsg);
+      grub_error (GRUB_ERR_IO, "%s", tftph->u.err.errmsg);
       grub_error_save (&data->save_err);
       return GRUB_ERR_NONE;
     default:
diff --git a/grub-core/script/lexer.c b/grub-core/script/lexer.c
index 57778f881..52004d059 100644
--- a/grub-core/script/lexer.c
+++ b/grub-core/script/lexer.c
@@ -349,7 +349,7 @@ void
 grub_script_yyerror (struct grub_parser_param *state, const char *err)
 {
   if (err)
-    grub_error (GRUB_ERR_INVALID_COMMAND, err);
+    grub_error (GRUB_ERR_INVALID_COMMAND, "%s", err);
 
   grub_print_error ();
   state->err++;
-- 
2.27.0

[PATCH v4 02/13] error: grub_error missing format string argument

[Glenn Washburn]

Its obvious from the error message that the variable named "type" was
accidentally omitted.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/parttool/msdospart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/parttool/msdospart.c b/grub-core/parttool/msdospart.c
index dcbf74e3b..2e2e9d527 100644
--- a/grub-core/parttool/msdospart.c
+++ b/grub-core/parttool/msdospart.c
@@ -127,8 +127,8 @@ static grub_err_t grub_pcpart_type (const grub_device_t dev,
     {
       dev->disk->partition = part;
       return grub_error (GRUB_ERR_BAD_ARGUMENT,
-			 N_("the partition type 0x%x isn't "
-			    "valid"));
+			 N_("the partition type 0x%x isn't valid"),
+			    type);
     }
 
   mbr.entries[index].type = type;
-- 
2.27.0

[PATCH v4 03/13] error: grub_error format string add missing format code

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/video/bochs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/video/bochs.c b/grub-core/video/bochs.c
index 7a249eb21..30ea1bd82 100644
--- a/grub-core/video/bochs.c
+++ b/grub-core/video/bochs.c
@@ -249,11 +249,11 @@ grub_video_bochs_setup (unsigned int width, unsigned int height,
     }
 
   if (width > BOCHS_MAX_WIDTH)
-    return grub_error (GRUB_ERR_IO, "width must be at most",
+    return grub_error (GRUB_ERR_IO, "width must be at most %d",
 		       BOCHS_MAX_WIDTH);
 
   if (height > BOCHS_MAX_HEIGHT)
-    return grub_error (GRUB_ERR_IO, "height must be at most",
+    return grub_error (GRUB_ERR_IO, "height must be at most %d",
 		       BOCHS_MAX_HEIGHT);
 
   if (width & (BOCHS_WIDTH_ALIGN - 1))
-- 
2.27.0

[PATCH v4 04/13] dmraid_nvidia: Format string error in grub_error

[Glenn Washburn]

The grub_error has a format string expecting two arguments, but only one
provided. According to the comments in the struct grub_nv_super definition,
the version field looks like a version number where major.minor is encoded
as each a byte in the two-byte short.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/dmraid_nvidia.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/disk/dmraid_nvidia.c b/grub-core/disk/dmraid_nvidia.c
index 4d2fb04d1..6372663e6 100644
--- a/grub-core/disk/dmraid_nvidia.c
+++ b/grub-core/disk/dmraid_nvidia.c
@@ -122,7 +122,7 @@ grub_dmraid_nv_detect (grub_disk_t disk,
   if (sb.version != NV_VERSION)
     {
       grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-		  "unknown version: %d.%d", sb.version);
+		  "unknown version: %d.%d", sb.version >> 8, sb.version & 0xFF);
       return NULL;
     }
 
-- 
2.27.0

[PATCH v4 05/13] grub_error: Use format code PRIuGRUB_SIZE for variables of type grub_size_t

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c        | 12 ++++++++----
 grub-core/kern/efi/efi.c           |  2 +-
 grub-core/loader/efi/chainloader.c |  2 +-
 grub-core/loader/i386/bsd.c        |  3 ++-
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b62835acc..4ff6654a0 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -484,13 +484,15 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
       }
       if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
 	{
-	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
+	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			    "Unsupported XTS block size: %"PRIuGRUB_SIZE,
 			    cipher->cipher->blocksize);
 	  goto err;
 	}
       if (secondary_cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
 	{
-	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
+	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			    "Unsupported XTS block size: %"PRIuGRUB_SIZE,
 			    secondary_cipher->cipher->blocksize);
 	  goto err;
 	}
@@ -501,7 +503,8 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
       cipheriv = ciphermode + sizeof ("lrw-") - 1;
       if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
 	{
-	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported LRW block size: %d",
+	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			    "Unsupported LRW block size: %"PRIuGRUB_SIZE,
 			    cipher->cipher->blocksize);
 	  goto err;
 	}
@@ -523,7 +526,8 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
     {
       if (cipher->cipher->blocksize & (cipher->cipher->blocksize - 1)
 	  || cipher->cipher->blocksize == 0)
-	grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported benbi blocksize: %d",
+	grub_error (GRUB_ERR_BAD_ARGUMENT,
+		    "Unsupported benbi blocksize: %"PRIuGRUB_SIZE,
 		    cipher->cipher->blocksize);
 	/* FIXME should we return an error here? */
       for (benbi_log = 0;
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 2942b8e35..d45cf67a9 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -503,7 +503,7 @@ grub_efi_duplicate_device_path (const grub_efi_device_path_t *dp)
       if (len < 4)
 	{
 	  grub_error (GRUB_ERR_OUT_OF_RANGE,
-		      "malformed EFI Device Path node has length=%d", len);
+		      "malformed EFI Device Path node has length=%"PRIuGRUB_SIZE, len);
 	  return NULL;
 	}
 
diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index 2e98947a0..b8d35f41e 100644
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -164,7 +164,7 @@ make_file_path (grub_efi_device_path_t *dp, const char *filename)
       if (len < 4)
 	{
 	  grub_error (GRUB_ERR_OUT_OF_RANGE,
-		      "malformed EFI Device Path node has length=%d", len);
+		      "malformed EFI Device Path node has length=%"PRIuGRUB_SIZE, len);
 	  return NULL;
 	}
 
diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index f5bf7f89e..fe20b339b 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -2110,7 +2110,8 @@ grub_cmd_openbsd_ramdisk (grub_command_t cmd __attribute__ ((unused)),
     {
       grub_file_close (file);
       return grub_error (GRUB_ERR_BAD_OS, "your kOpenBSD supports ramdisk only"
-			 " up to %u bytes, however you supplied a %u bytes one",
+			 " up to %"PRIuGRUB_SIZE" bytes, however you supplied"
+			 " a %"PRIuGRUB_SIZE" bytes one",
 			 openbsd_ramdisk.max_size, size);
     }
 
-- 
2.27.0

[PATCH v4 06/13] pgp: Format code for grub_error is incorrect

[Glenn Washburn]

The format code is for a 32-bit int, but the argument, keyid, is declared as
a 64 bit int. The comment above says keyid is 32-bit. I'm not sure if the
comment or declaration is wrong, so force the display of a 64-bit int for
now.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/pgp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
index bbf6871fe..150cb6fe9 100644
--- a/grub-core/commands/pgp.c
+++ b/grub-core/commands/pgp.c
@@ -633,7 +633,7 @@ grub_verify_signature_real (struct grub_pubkey_context *ctxt,
   if (!sk)
     {
       /* TRANSLATORS: %08x is 32-bit key id.  */
-      grub_error (GRUB_ERR_BAD_SIGNATURE, N_("public key %08x not found"),
+      grub_error (GRUB_ERR_BAD_SIGNATURE, N_("public key %08"PRIxGRUB_UINT64_T" not found"),
 		  keyid);
       goto fail;
     }
-- 
2.27.0

[PATCH v4 07/13] efi: Format string error in grub_error

[Glenn Washburn]

The second format string argument, GRUB_EFI_MAX_USABLE_ADDRESS, is a macro
to a number literal. However depending on what the target architecture, the
type can be 32 or 64 bits. Cast to a 64-bit integer.  Also, change the
format string literals %llx to use PRIxGRUB_UINT64_T.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/efi/mm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 457772d57..9d3d279fd 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -127,8 +127,9 @@ grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
   if (address > GRUB_EFI_MAX_USABLE_ADDRESS)
     {
       grub_error (GRUB_ERR_BAD_ARGUMENT,
-		  N_("invalid memory address (0x%llx > 0x%llx)"),
-		  address, GRUB_EFI_MAX_USABLE_ADDRESS);
+		  N_("invalid memory address (0x%"PRIxGRUB_UINT64_T
+                     " > 0x%"PRIxGRUB_UINT64_T")"),
+		  address, (grub_efi_uint64_t) GRUB_EFI_MAX_USABLE_ADDRESS);
       return NULL;
     }
 
-- 
2.27.0

[PATCH v4 08/13] error: Use %p format code for pointer types

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/loader/i386/pc/linux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/loader/i386/pc/linux.c b/grub-core/loader/i386/pc/linux.c
index 814988ab9..0bc5d6807 100644
--- a/grub-core/loader/i386/pc/linux.c
+++ b/grub-core/loader/i386/pc/linux.c
@@ -230,9 +230,9 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
       && GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size
       > grub_linux_real_target)
     {
-      grub_error (GRUB_ERR_BAD_OS, "too big zImage (0x%x > 0x%x), use bzImage instead",
+      grub_error (GRUB_ERR_BAD_OS, "too big zImage (%p > %p), use bzImage instead",
 		  (char *) GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size,
-		  (grub_size_t) grub_linux_real_target);
+		  (void *) grub_linux_real_target);
       goto fail;
     }
 
-- 
2.27.0

Re: [PATCH v4 08/13] error: Use %p format code for pointer types

[Daniel Kiper]

On Thu, Feb 18, 2021 at 08:47:09PM -0600, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/loader/i386/pc/linux.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/loader/i386/pc/linux.c b/grub-core/loader/i386/pc/linux.c
> index 814988ab9..0bc5d6807 100644
> --- a/grub-core/loader/i386/pc/linux.c
> +++ b/grub-core/loader/i386/pc/linux.c
> @@ -230,9 +230,9 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>        && GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size
>        > grub_linux_real_target)
>      {
> -      grub_error (GRUB_ERR_BAD_OS, "too big zImage (0x%x > 0x%x), use bzImage instead",
> +      grub_error (GRUB_ERR_BAD_OS, "too big zImage (%p > %p), use bzImage instead",
>  		  (char *) GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size,
> -		  (grub_size_t) grub_linux_real_target);
> +		  (void *) grub_linux_real_target);

I would use PRIxGRUB_SIZE and PRIxGRUB_ADDR respectively.

Daniel

Re: [PATCH v4 08/13] error: Use %p format code for pointer types

[Glenn Washburn]

On Sat, 27 Feb 2021 12:43:06 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Feb 18, 2021 at 08:47:09PM -0600, Glenn Washburn wrote:
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/loader/i386/pc/linux.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/grub-core/loader/i386/pc/linux.c
> > b/grub-core/loader/i386/pc/linux.c index 814988ab9..0bc5d6807 100644
> > --- a/grub-core/loader/i386/pc/linux.c
> > +++ b/grub-core/loader/i386/pc/linux.c
> > @@ -230,9 +230,9 @@ grub_cmd_linux (grub_command_t cmd
> > __attribute__ ((unused)), && GRUB_LINUX_ZIMAGE_ADDR +
> > grub_linux16_prot_size
> >        > grub_linux_real_target)
> >      {
> > -      grub_error (GRUB_ERR_BAD_OS, "too big zImage (0x%x > 0x%x),
> > use bzImage instead",
> > +      grub_error (GRUB_ERR_BAD_OS, "too big zImage (%p > %p), use
> > bzImage instead", (char *) GRUB_LINUX_ZIMAGE_ADDR +
> > grub_linux16_prot_size,
> > -		  (grub_size_t) grub_linux_real_target);
> > +		  (void *) grub_linux_real_target);
> 
> I would use PRIxGRUB_SIZE and PRIxGRUB_ADDR respectively.

IIRC, The reason I chose the %p format string was because the compiler
considers it an error to use a pointer for an integer format string
code. Does it make more sense to have both be PRIxGRUB_ADDR and for
PRIxGRUB_ADDR to be defined as "p"?

Since they are both pointers, I'm not following why the code should be
semantically different (ie PRIxGRUB_SIZE and PRIxGRUB_ADDR, instead of
just one of those). If you do want to use a "*x" format code, I think
we'd need to cast those arguments.

Glenn

Re: [PATCH v4 08/13] error: Use %p format code for pointer types

[Daniel Kiper]

On Sun, Feb 28, 2021 at 02:02:20PM -0600, Glenn Washburn wrote:
> On Sat, 27 Feb 2021 12:43:06 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Thu, Feb 18, 2021 at 08:47:09PM -0600, Glenn Washburn wrote:
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  grub-core/loader/i386/pc/linux.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/grub-core/loader/i386/pc/linux.c
> > > b/grub-core/loader/i386/pc/linux.c index 814988ab9..0bc5d6807 100644
> > > --- a/grub-core/loader/i386/pc/linux.c
> > > +++ b/grub-core/loader/i386/pc/linux.c
> > > @@ -230,9 +230,9 @@ grub_cmd_linux (grub_command_t cmd
> > > __attribute__ ((unused)), && GRUB_LINUX_ZIMAGE_ADDR +
> > > grub_linux16_prot_size
> > >        > grub_linux_real_target)
> > >      {
> > > -      grub_error (GRUB_ERR_BAD_OS, "too big zImage (0x%x > 0x%x),
> > > use bzImage instead",
> > > +      grub_error (GRUB_ERR_BAD_OS, "too big zImage (%p > %p), use
> > > bzImage instead", (char *) GRUB_LINUX_ZIMAGE_ADDR +
> > > grub_linux16_prot_size,
> > > -		  (grub_size_t) grub_linux_real_target);
> > > +		  (void *) grub_linux_real_target);
> >
> > I would use PRIxGRUB_SIZE and PRIxGRUB_ADDR respectively.
>
> IIRC, The reason I chose the %p format string was because the compiler
> considers it an error to use a pointer for an integer format string

If you look a bit above you can see:
  static grub_size_t grub_linux16_prot_size;
  static grub_addr_t grub_linux_real_target;

So, they are not pointers.

> code. Does it make more sense to have both be PRIxGRUB_ADDR and for
> PRIxGRUB_ADDR to be defined as "p"?

Nope.

> Since they are both pointers, I'm not following why the code should be

No, they are not.

> semantically different (ie PRIxGRUB_SIZE and PRIxGRUB_ADDR, instead of
> just one of those). If you do want to use a "*x" format code, I think
> we'd need to cast those arguments.

If you drop (void/char *) casting the you should be able to use
PRIxGRUB_SIZE and PRIxGRUB_ADDR without any issues.

Daniel

[PATCH v4 09/13] error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in grub_error

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/ata.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/ata.c b/grub-core/disk/ata.c
index 685f33a19..c8f350ed3 100644
--- a/grub-core/disk/ata.c
+++ b/grub-core/disk/ata.c
@@ -219,7 +219,7 @@ grub_ata_setaddress (struct grub_ata *dev,
 	if (dev->sectors_per_track == 0
 	    || dev->heads == 0)
 	  return grub_error (GRUB_ERR_OUT_OF_RANGE,
-			     "sector %d cannot be addressed "
+			     "sector %"PRIxGRUB_UINT64_T" cannot be addressed "
 			     "using CHS addressing", sector);
 
 	/* Calculate the sector, cylinder and head to use.  */
@@ -232,7 +232,7 @@ grub_ata_setaddress (struct grub_ata *dev,
 	    || cylinder > dev->cylinders
 	    || head > dev->heads)
 	  return grub_error (GRUB_ERR_OUT_OF_RANGE,
-			     "sector %d cannot be addressed "
+			     "sector %"PRIxGRUB_UINT64_T" cannot be addressed "
 			     "using CHS addressing", sector);
 	
 	parms->taskfile.disk = 0xE0 | head;
-- 
2.27.0

[PATCH v4 10/13] error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error

[Glenn Washburn]

The macro ELF_R_TYPE does not change the underlying type. Here its argument
is a 64-bit Elf64_Xword. Make sure the format code matches.

For the riscv architecture, rel->r_info could be either Elf32_Xword or
Elf64_Xword depending on if 32 or 64-bit risc is being built. So cast to
64-bit value regardless.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/efiemu/i386/loadcore64.c | 3 ++-
 grub-core/kern/arm64/dl.c          | 3 ++-
 grub-core/kern/ia64/dl.c           | 3 ++-
 grub-core/kern/riscv/dl.c          | 5 +++--
 grub-core/kern/sparc64/dl.c        | 3 ++-
 grub-core/kern/x86_64/dl.c         | 3 ++-
 6 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/grub-core/efiemu/i386/loadcore64.c b/grub-core/efiemu/i386/loadcore64.c
index 18facf47f..3e9a71cfd 100644
--- a/grub-core/efiemu/i386/loadcore64.c
+++ b/grub-core/efiemu/i386/loadcore64.c
@@ -122,7 +122,8 @@ grub_arch_efiemu_relocate_symbols64 (grub_efiemu_segment_t segs,
                     break;
 		  default:
 		    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-				       N_("relocation 0x%x is not implemented yet"),
+				       N_("relocation 0x%"PRIxGRUB_UINT64_T
+					  " is not implemented yet"),
 				       ELF_R_TYPE (rel->r_info));
 		  }
 	      }
diff --git a/grub-core/kern/arm64/dl.c b/grub-core/kern/arm64/dl.c
index fb0337319..b04fed18a 100644
--- a/grub-core/kern/arm64/dl.c
+++ b/grub-core/kern/arm64/dl.c
@@ -184,7 +184,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
+				" is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
diff --git a/grub-core/kern/ia64/dl.c b/grub-core/kern/ia64/dl.c
index ebcf31629..ddf240976 100644
--- a/grub-core/kern/ia64/dl.c
+++ b/grub-core/kern/ia64/dl.c
@@ -137,7 +137,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 	  break;
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
+				" is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
diff --git a/grub-core/kern/riscv/dl.c b/grub-core/kern/riscv/dl.c
index 6fb8385ef..77f5b7648 100644
--- a/grub-core/kern/riscv/dl.c
+++ b/grub-core/kern/riscv/dl.c
@@ -331,8 +331,9 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 	  break;
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
-			     ELF_R_TYPE (rel->r_info));
+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
+				" is not implemented yet"),
+			     (grub_uint64_t)ELF_R_TYPE (rel->r_info));
 	}
     }
 
diff --git a/grub-core/kern/sparc64/dl.c b/grub-core/kern/sparc64/dl.c
index 739be4717..60ce183fb 100644
--- a/grub-core/kern/sparc64/dl.c
+++ b/grub-core/kern/sparc64/dl.c
@@ -177,7 +177,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 	  break;
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
+				" is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
diff --git a/grub-core/kern/x86_64/dl.c b/grub-core/kern/x86_64/dl.c
index 3a73e6e6c..8d2800c9e 100644
--- a/grub-core/kern/x86_64/dl.c
+++ b/grub-core/kern/x86_64/dl.c
@@ -107,7 +107,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
+			        " is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
-- 
2.27.0

Re: [PATCH v4 10/13] error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error

[Daniel Kiper]

On Thu, Feb 18, 2021 at 08:47:11PM -0600, Glenn Washburn wrote:
> The macro ELF_R_TYPE does not change the underlying type. Here its argument
> is a 64-bit Elf64_Xword. Make sure the format code matches.
>
> For the riscv architecture, rel->r_info could be either Elf32_Xword or
> Elf64_Xword depending on if 32 or 64-bit risc is being built. So cast to
> 64-bit value regardless.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/efiemu/i386/loadcore64.c | 3 ++-
>  grub-core/kern/arm64/dl.c          | 3 ++-
>  grub-core/kern/ia64/dl.c           | 3 ++-
>  grub-core/kern/riscv/dl.c          | 5 +++--
>  grub-core/kern/sparc64/dl.c        | 3 ++-
>  grub-core/kern/x86_64/dl.c         | 3 ++-
>  6 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/efiemu/i386/loadcore64.c b/grub-core/efiemu/i386/loadcore64.c
> index 18facf47f..3e9a71cfd 100644
> --- a/grub-core/efiemu/i386/loadcore64.c
> +++ b/grub-core/efiemu/i386/loadcore64.c
> @@ -122,7 +122,8 @@ grub_arch_efiemu_relocate_symbols64 (grub_efiemu_segment_t segs,
>                      break;
>  		  default:
>  		    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> -				       N_("relocation 0x%x is not implemented yet"),
> +				       N_("relocation 0x%"PRIxGRUB_UINT64_T

Next time please add space between '"' and PRIxGRUB_UINT64_T.
I will fix this before committing.

> +					  " is not implemented yet"),
>  				       ELF_R_TYPE (rel->r_info));
>  		  }
>  	      }
> diff --git a/grub-core/kern/arm64/dl.c b/grub-core/kern/arm64/dl.c
> index fb0337319..b04fed18a 100644
> --- a/grub-core/kern/arm64/dl.c
> +++ b/grub-core/kern/arm64/dl.c
> @@ -184,7 +184,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
>
>  	default:
>  	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> -			     N_("relocation 0x%x is not implemented yet"),
> +			     N_("relocation 0x%"PRIxGRUB_UINT64_T

Ditto and below...

> +				" is not implemented yet"),
>  			     ELF_R_TYPE (rel->r_info));
>  	}
>      }
> diff --git a/grub-core/kern/ia64/dl.c b/grub-core/kern/ia64/dl.c
> index ebcf31629..ddf240976 100644
> --- a/grub-core/kern/ia64/dl.c
> +++ b/grub-core/kern/ia64/dl.c
> @@ -137,7 +137,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
>  	  break;
>  	default:
>  	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> -			     N_("relocation 0x%x is not implemented yet"),
> +			     N_("relocation 0x%"PRIxGRUB_UINT64_T
> +				" is not implemented yet"),
>  			     ELF_R_TYPE (rel->r_info));
>  	}
>      }
> diff --git a/grub-core/kern/riscv/dl.c b/grub-core/kern/riscv/dl.c
> index 6fb8385ef..77f5b7648 100644
> --- a/grub-core/kern/riscv/dl.c
> +++ b/grub-core/kern/riscv/dl.c
> @@ -331,8 +331,9 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
>  	  break;
>  	default:
>  	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> -			     N_("relocation 0x%x is not implemented yet"),
> -			     ELF_R_TYPE (rel->r_info));
> +			     N_("relocation 0x%"PRIxGRUB_UINT64_T
> +				" is not implemented yet"),
> +			     (grub_uint64_t)ELF_R_TYPE (rel->r_info));

Missing space between "(grub_uint64_t)" and ELF_R_TYPE().

Daniel

Re: [PATCH v4 10/13] error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error

[Glenn Washburn]

On Sat, 27 Feb 2021 12:48:11 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Feb 18, 2021 at 08:47:11PM -0600, Glenn Washburn wrote:
> > The macro ELF_R_TYPE does not change the underlying type. Here its
> > argument is a 64-bit Elf64_Xword. Make sure the format code matches.
> >
> > For the riscv architecture, rel->r_info could be either Elf32_Xword
> > or Elf64_Xword depending on if 32 or 64-bit risc is being built. So
> > cast to 64-bit value regardless.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/efiemu/i386/loadcore64.c | 3 ++-
> >  grub-core/kern/arm64/dl.c          | 3 ++-
> >  grub-core/kern/ia64/dl.c           | 3 ++-
> >  grub-core/kern/riscv/dl.c          | 5 +++--
> >  grub-core/kern/sparc64/dl.c        | 3 ++-
> >  grub-core/kern/x86_64/dl.c         | 3 ++-
> >  6 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/grub-core/efiemu/i386/loadcore64.c
> > b/grub-core/efiemu/i386/loadcore64.c index 18facf47f..3e9a71cfd
> > 100644 --- a/grub-core/efiemu/i386/loadcore64.c
> > +++ b/grub-core/efiemu/i386/loadcore64.c
> > @@ -122,7 +122,8 @@ grub_arch_efiemu_relocate_symbols64
> > (grub_efiemu_segment_t segs, break;
> >  		  default:
> >  		    return grub_error
> > (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > -				       N_("relocation 0x%x is not
> > implemented yet"),
> > +				       N_("relocation
> > 0x%"PRIxGRUB_UINT64_T
> 
> Next time please add space between '"' and PRIxGRUB_UINT64_T.
> I will fix this before committing.

Okay, hadn't noticed that rule. I'll see about adding a patch that
cleans that up in a few other places in the code too.

> > diff --git a/grub-core/kern/riscv/dl.c b/grub-core/kern/riscv/dl.c
> > index 6fb8385ef..77f5b7648 100644
> > --- a/grub-core/kern/riscv/dl.c
> > +++ b/grub-core/kern/riscv/dl.c
> > @@ -331,8 +331,9 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod,
> > void *ehdr, break;
> >  	default:
> >  	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > -			     N_("relocation 0x%x is not
> > implemented yet"),
> > -			     ELF_R_TYPE (rel->r_info));
> > +			     N_("relocation 0x%"PRIxGRUB_UINT64_T
> > +				" is not implemented yet"),
> > +			     (grub_uint64_t)ELF_R_TYPE
> > (rel->r_info));
> 
> Missing space between "(grub_uint64_t)" and ELF_R_TYPE().

Hadn't noticed this rule. Grepping the source shows that it is super
common to not have a space after cast. Perhaps this would be good to
add to the developer documentation.

Glenn

Re: [PATCH v4 10/13] error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error

[Daniel Kiper]

On Sun, Feb 28, 2021 at 02:13:06PM -0600, Glenn Washburn wrote:
> On Sat, 27 Feb 2021 12:48:11 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Thu, Feb 18, 2021 at 08:47:11PM -0600, Glenn Washburn wrote:
> > > The macro ELF_R_TYPE does not change the underlying type. Here its
> > > argument is a 64-bit Elf64_Xword. Make sure the format code matches.
> > >
> > > For the riscv architecture, rel->r_info could be either Elf32_Xword
> > > or Elf64_Xword depending on if 32 or 64-bit risc is being built. So
> > > cast to 64-bit value regardless.
> > >
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  grub-core/efiemu/i386/loadcore64.c | 3 ++-
> > >  grub-core/kern/arm64/dl.c          | 3 ++-
> > >  grub-core/kern/ia64/dl.c           | 3 ++-
> > >  grub-core/kern/riscv/dl.c          | 5 +++--
> > >  grub-core/kern/sparc64/dl.c        | 3 ++-
> > >  grub-core/kern/x86_64/dl.c         | 3 ++-
> > >  6 files changed, 13 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/grub-core/efiemu/i386/loadcore64.c
> > > b/grub-core/efiemu/i386/loadcore64.c index 18facf47f..3e9a71cfd
> > > 100644 --- a/grub-core/efiemu/i386/loadcore64.c
> > > +++ b/grub-core/efiemu/i386/loadcore64.c
> > > @@ -122,7 +122,8 @@ grub_arch_efiemu_relocate_symbols64
> > > (grub_efiemu_segment_t segs, break;
> > >  		  default:
> > >  		    return grub_error
> > > (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > > -				       N_("relocation 0x%x is not
> > > implemented yet"),
> > > +				       N_("relocation
> > > 0x%"PRIxGRUB_UINT64_T
> >
> > Next time please add space between '"' and PRIxGRUB_UINT64_T.
> > I will fix this before committing.
>
> Okay, hadn't noticed that rule. I'll see about adding a patch that
> cleans that up in a few other places in the code too.

That would be nice...

> > > diff --git a/grub-core/kern/riscv/dl.c b/grub-core/kern/riscv/dl.c
> > > index 6fb8385ef..77f5b7648 100644
> > > --- a/grub-core/kern/riscv/dl.c
> > > +++ b/grub-core/kern/riscv/dl.c
> > > @@ -331,8 +331,9 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod,
> > > void *ehdr, break;
> > >  	default:
> > >  	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > > -			     N_("relocation 0x%x is not
> > > implemented yet"),
> > > -			     ELF_R_TYPE (rel->r_info));
> > > +			     N_("relocation 0x%"PRIxGRUB_UINT64_T
> > > +				" is not implemented yet"),
> > > +			     (grub_uint64_t)ELF_R_TYPE
> > > (rel->r_info));
> >
> > Missing space between "(grub_uint64_t)" and ELF_R_TYPE().
>
> Hadn't noticed this rule. Grepping the source shows that it is super
> common to not have a space after cast. Perhaps this would be good to
> add to the developer documentation.

Yeah, I think we should add more than that... And we have to have a tool
which checks formatting in the patches too.

Daniel

[PATCH v4 11/13] error: Use format code PRIuGRUB_UINT64_T for 64-bit typed fileblock in grub_error

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/fs/hfsplus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 9c4e4c88c..b538f52e0 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -188,7 +188,8 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
 	  || !nnode)
 	{
 	  grub_error (GRUB_ERR_READ_ERROR,
-		      "no block found for the file id 0x%x and the block offset 0x%x",
+		      "no block found for the file id 0x%x and the block"
+		      " offset 0x%"PRIuGRUB_UINT64_T,
 		      node->fileid, fileblock);
 	  break;
 	}
-- 
2.27.0

[PATCH v4 12/13] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Glenn Washburn]

For some reason PRIuGRUB_UINT64_T is not expanding to llu, but to lu, which
causes the format string check to fail. Use literal and force cast until
this is debugged.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/fs/zfs/zfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index b6e1e178d..a3691d220 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -1869,8 +1869,8 @@ zio_read (blkptr_t *bp, grub_zfs_endian_t endian, void **buf,
     {
       if (BPE_GET_ETYPE(bp) != BP_EMBEDDED_TYPE_DATA)
 	return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			   "unsupported embedded BP (type=%u)\n",
-			   BPE_GET_ETYPE(bp));
+			   "unsupported embedded BP (type=%llu)\n",
+			   (long long unsigned int)BPE_GET_ETYPE(bp));
       lsize = BPE_GET_LSIZE(bp);
       psize = BF64_GET_SB(grub_zfs_to_cpu64 ((bp)->blk_prop, endian), 25, 7, 0, 1);
     }
-- 
2.27.0

Re: [PATCH v4 12/13] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Daniel Kiper]

On Thu, Feb 18, 2021 at 08:47:13PM -0600, Glenn Washburn wrote:
> For some reason PRIuGRUB_UINT64_T is not expanding to llu, but to lu, which
> causes the format string check to fail. Use literal and force cast until
> this is debugged.

I think the problem is that currently BF64_DECODE() uses "1ULL". I think it should
look like this

  #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low), ((grub_uint64_t) 1) << (len))

instead of

  #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low), 1ULL << (len))

Same or similar for other macros there.

I would prefer if you fix macros in include/grub/zfs/spa.h first and
then do proper fix here.

Daniel

Re: [PATCH v4 12/13] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Glenn Washburn]

On Sat, 27 Feb 2021 13:05:09 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Feb 18, 2021 at 08:47:13PM -0600, Glenn Washburn wrote:
> > For some reason PRIuGRUB_UINT64_T is not expanding to llu, but to
> > lu, which causes the format string check to fail. Use literal and
> > force cast until this is debugged.
> 
> I think the problem is that currently BF64_DECODE() uses "1ULL". I
> think it should look like this
> 
>   #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low),
> ((grub_uint64_t) 1) << (len))
> 
> instead of
> 
>   #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low), 1ULL <<
> (len))
> 
> Same or similar for other macros there.
> 
> I would prefer if you fix macros in include/grub/zfs/spa.h first and
> then do proper fix here.

Yep, completely agree, just hadn't figured out a proper solution. And
not being familiar with the code and not doing the zfs functional tests
(GitLab CI shared runners do not have zfs kernel support), I'm hesitant
to make such intrusive changes. However, your suggestion is the better
way to do it and I've confirmed that it work on i386 and x86_64. I'll
update the patch in the next patch series.

Glenn

Re: [PATCH v4 12/13] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Daniel Kiper]

On Sun, Feb 28, 2021 at 03:10:52PM -0600, Glenn Washburn wrote:
> On Sat, 27 Feb 2021 13:05:09 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Thu, Feb 18, 2021 at 08:47:13PM -0600, Glenn Washburn wrote:
> > > For some reason PRIuGRUB_UINT64_T is not expanding to llu, but to
> > > lu, which causes the format string check to fail. Use literal and
> > > force cast until this is debugged.
> >
> > I think the problem is that currently BF64_DECODE() uses "1ULL". I
> > think it should look like this
> >
> >   #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low),
> > ((grub_uint64_t) 1) << (len))
> >
> > instead of
> >
> >   #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low), 1ULL <<
> > (len))
> >
> > Same or similar for other macros there.
> >
> > I would prefer if you fix macros in include/grub/zfs/spa.h first and
> > then do proper fix here.
>
> Yep, completely agree, just hadn't figured out a proper solution. And
> not being familiar with the code and not doing the zfs functional tests
> (GitLab CI shared runners do not have zfs kernel support), I'm hesitant
> to make such intrusive changes. However, your suggestion is the better
> way to do it and I've confirmed that it work on i386 and x86_64. I'll
> update the patch in the next patch series.

I am still afraid doing that before release until we are able to confirm
that everything works for 32-bit and 64-bit architectures. If not I would
not merge this patch and next one at this point.

Daniel

Re: [PATCH v4 12/13] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Glenn Washburn]

On Wed, 3 Mar 2021 19:17:31 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Sun, Feb 28, 2021 at 03:10:52PM -0600, Glenn Washburn wrote:
> > On Sat, 27 Feb 2021 13:05:09 +0100
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> >
> > > On Thu, Feb 18, 2021 at 08:47:13PM -0600, Glenn Washburn wrote:
> > > > For some reason PRIuGRUB_UINT64_T is not expanding to llu, but
> > > > to lu, which causes the format string check to fail. Use
> > > > literal and force cast until this is debugged.
> > >
> > > I think the problem is that currently BF64_DECODE() uses "1ULL". I
> > > think it should look like this
> > >
> > >   #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low),
> > > ((grub_uint64_t) 1) << (len))
> > >
> > > instead of
> > >
> > >   #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low), 1ULL <<
> > > (len))
> > >
> > > Same or similar for other macros there.
> > >
> > > I would prefer if you fix macros in include/grub/zfs/spa.h first
> > > and then do proper fix here.
> >
> > Yep, completely agree, just hadn't figured out a proper solution.
> > And not being familiar with the code and not doing the zfs
> > functional tests (GitLab CI shared runners do not have zfs kernel
> > support), I'm hesitant to make such intrusive changes. However,
> > your suggestion is the better way to do it and I've confirmed that
> > it work on i386 and x86_64. I'll update the patch in the next patch
> > series.
> 
> I am still afraid doing that before release until we are able to
> confirm that everything works for 32-bit and 64-bit architectures. If
> not I would not merge this patch and next one at this point.

Is there anyone here that can do this testing?

I does seem a little ridiculous that patch #13 would be blocked by zfs,
since its generally useful for the whole code base. My presumption
based on experience here is that if I'm not here to keep pushing for
#13, it will get forgotten about. This is why I'd like to get it in
now. Patch #12 may not be ideal, but its certainly an improvement. I
think it makes sense to include it for now, so that #13 can be
merged. Then at a later date when zfs testing can be done, the better
fix can be done.

Glenn

[PATCH v4 13/13] error: Do compile-time format string checking on grub_error

[Glenn Washburn]

This should help prevent format string errorsand thus improve the quality
of error reporting.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 include/grub/err.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/grub/err.h b/include/grub/err.h
index 24ba9f5f5..b08d5d0de 100644
--- a/include/grub/err.h
+++ b/include/grub/err.h
@@ -85,7 +85,8 @@ struct grub_error_saved
 extern grub_err_t EXPORT_VAR(grub_errno);
 extern char EXPORT_VAR(grub_errmsg)[GRUB_MAX_ERRMSG];
 
-grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...);
+grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...)
+    __attribute__ ((format (GNU_PRINTF, 2, 3)));
 void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
 void EXPORT_FUNC(grub_error_push) (void);
 int EXPORT_FUNC(grub_error_pop) (void);
-- 
2.27.0

Re: [PATCH v4 00/13] error: Do compile-time format string checking on grub_error

[Daniel Kiper]

On Thu, Feb 18, 2021 at 08:47:01PM -0600, Glenn Washburn wrote:
> This patch series fixes all compile errors due to format string issues on
> grub_error. This was tested against nearly all supported platforms
> successfully. This is important because earlier versions of these changes
> compiled successfully on x86 platforms, but had issues on other ones not
> tested.

Could you give examples what kind of errors you get when you build
non-x86 platforms?

> All patches except the last fix actual format string issues. The last patch
> turns format string issues into errors. This is a good idea because it will
> help to prevent introducing new format string issues into the code. Since, I
> presume, Daniel does not do not do a build test for all architectures before
> committing to master, this will not ensure that no format string issues get
> introduced into the code. However, it should flush out any format string

I am confused by these sentence. Anyway, I do build tests of all patches
before committing for all architectures which I am aware of. I think
difference between our results come from difference in build
environments, flags and options to do tests.

> issues when the CI build is done.
>
> Many of these changes are fairly obvious. I tried to use the PRI*GRUB_*_T
> macros as much as I could and to not cast arguments. There are some notable
> exceptions. There is some code that uses the same grub_error code in both
> 32 and 64 bit architectures (riscv), so casting was needed to force the larger
> storage type. The second to last patch for zfs is still confounding to me
> as to why the macro PRIuGRUB_UINT64_T was not expanding to llu, and yet the
> compiler was saying the argument was a long long unsigned.

For all the patches except #12 Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Though I will probably do not get #13 until #12 is updated and merged.

Daniel

Re: [PATCH v4 00/13] error: Do compile-time format string checking on grub_error

[Glenn Washburn]

On Sat, 27 Feb 2021 13:19:17 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Feb 18, 2021 at 08:47:01PM -0600, Glenn Washburn wrote:
> > This patch series fixes all compile errors due to format string
> > issues on grub_error. This was tested against nearly all supported
> > platforms successfully. This is important because earlier versions
> > of these changes compiled successfully on x86 platforms, but had
> > issues on other ones not tested.
> 
> Could you give examples what kind of errors you get when you build
> non-x86 platforms?

More format string errors showed up when building for non-x86 platforms
because code which contained a format string issue, but is not compiled
for x86 was then compiled. For example, the diff for
grub-core/kern/arm64/dl.c in patch #10. That bug did not show up in my
original patch series because I was only building for x86 targets.

> > All patches except the last fix actual format string issues. The
> > last patch turns format string issues into errors. This is a good
> > idea because it will help to prevent introducing new format string
> > issues into the code. Since, I presume, Daniel does not do not do a
> > build test for all architectures before committing to master, this
> > will not ensure that no format string issues get introduced into
> > the code. However, it should flush out any format string
> 
> I am confused by these sentence. Anyway, I do build tests of all
> patches before committing for all architectures which I am aware of.
> I think difference between our results come from difference in build
> environments, flags and options to do tests.

I hope this did not sound like a criticism or that you missed something
due to the way you are build testing. You would have never gotten a
build failure for any of these bugs even if you were building for
all architectures (which it sounds like you were). I was merely unsure
if you did a build test for all known architectures for every update of
master. With patch #13 it can be more of an issue if you're not building
testing all architectures because of the situation I outlined above in
the first paragraph. In the worst case though, GRUB would fail to build
for certain non-tested architectures where patches with format string
bugs were introduced. Presumably, this would be discovered quickly by
the people here who use those architectures.

> > issues when the CI build is done.
> >
> > Many of these changes are fairly obvious. I tried to use the
> > PRI*GRUB_*_T macros as much as I could and to not cast arguments.
> > There are some notable exceptions. There is some code that uses the
> > same grub_error code in both 32 and 64 bit architectures (riscv),
> > so casting was needed to force the larger storage type. The second
> > to last patch for zfs is still confounding to me as to why the
> > macro PRIuGRUB_UINT64_T was not expanding to llu, and yet the
> > compiler was saying the argument was a long long unsigned.
> 
> For all the patches except #12 Reviewed-by: Daniel Kiper
> <daniel.kiper@oracle.com>
> 
> Though I will probably do not get #13 until #12 is updated and merged.

Yes definitely, #13 requires the issue in #12 be addressed in someway.

Glenn

[PATCH v5 00/13] error: Do compile-time format string checking on grub_error

[Glenn Washburn]

This updates patches #8 and #10 per the review in v4. Patch #12 has not been
updated because the changes, though better, would disqualify it from merging
before the release due to it being more invasive. The goal is to get patch
#13, which requires patch #12, in before the release so that patches based
on the release will have to have passed format string checking for
grub_error.

Glenn

Glenn Washburn (13):
  misc: Format string for grub_error should be a literal
  error: grub_error missing format string argument
  error: grub_error format string add missing format code
  dmraid_nvidia: Format string error in grub_error
  grub_error: Use format code PRIuGRUB_SIZE for variables of type
    grub_size_t
  pgp: Format code for grub_error is incorrect
  efi: Format string error in grub_error
  error: Use PRI* macros to get correct format string code across
    architectures
  error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in
    grub_error
  error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error
  error: Use format code PRIuGRUB_UINT64_T for 64-bit typed fileblock in
    grub_error
  error: Use format code llu for 64-bit uint bp->blk_prop in grub_error
  error: Do compile-time format string checking on grub_error

 grub-core/commands/pgp.c           |  2 +-
 grub-core/disk/ata.c               |  4 ++--
 grub-core/disk/cryptodisk.c        | 12 ++++++++----
 grub-core/disk/dmraid_nvidia.c     |  2 +-
 grub-core/efiemu/i386/loadcore64.c |  3 ++-
 grub-core/fs/hfsplus.c             |  3 ++-
 grub-core/fs/zfs/zfs.c             |  4 ++--
 grub-core/kern/arm64/dl.c          |  3 ++-
 grub-core/kern/efi/efi.c           |  2 +-
 grub-core/kern/efi/mm.c            |  5 +++--
 grub-core/kern/ia64/dl.c           |  3 ++-
 grub-core/kern/riscv/dl.c          |  5 +++--
 grub-core/kern/sparc64/dl.c        |  3 ++-
 grub-core/kern/x86_64/dl.c         |  3 ++-
 grub-core/loader/efi/chainloader.c |  4 ++--
 grub-core/loader/i386/bsd.c        |  3 ++-
 grub-core/loader/i386/pc/linux.c   |  7 ++++---
 grub-core/net/tftp.c               |  2 +-
 grub-core/parttool/msdospart.c     |  4 ++--
 grub-core/script/lexer.c           |  2 +-
 grub-core/video/bochs.c            |  4 ++--
 include/grub/err.h                 |  3 ++-
 22 files changed, 49 insertions(+), 34 deletions(-)

-- 
2.27.0

[PATCH v5 01/13] misc: Format string for grub_error should be a literal

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/loader/efi/chainloader.c | 2 +-
 grub-core/net/tftp.c               | 2 +-
 grub-core/script/lexer.c           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index 7b31c3fb9..2e98947a0 100644
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -90,7 +90,7 @@ grub_chainloader_boot (void)
 	      *grub_utf16_to_utf8 ((grub_uint8_t *) buf,
 				   exit_data, exit_data_size) = 0;
 
-	      grub_error (GRUB_ERR_BAD_OS, buf);
+	      grub_error (GRUB_ERR_BAD_OS, "%s", buf);
 	      grub_free (buf);
 	    }
 	}
diff --git a/grub-core/net/tftp.c b/grub-core/net/tftp.c
index 7e659a78b..592bfee34 100644
--- a/grub-core/net/tftp.c
+++ b/grub-core/net/tftp.c
@@ -252,7 +252,7 @@ tftp_receive (grub_net_udp_socket_t sock __attribute__ ((unused)),
     case TFTP_ERROR:
       data->have_oack = 1;
       grub_netbuff_free (nb);
-      grub_error (GRUB_ERR_IO, (char *) tftph->u.err.errmsg);
+      grub_error (GRUB_ERR_IO, "%s", tftph->u.err.errmsg);
       grub_error_save (&data->save_err);
       return GRUB_ERR_NONE;
     default:
diff --git a/grub-core/script/lexer.c b/grub-core/script/lexer.c
index 57778f881..52004d059 100644
--- a/grub-core/script/lexer.c
+++ b/grub-core/script/lexer.c
@@ -349,7 +349,7 @@ void
 grub_script_yyerror (struct grub_parser_param *state, const char *err)
 {
   if (err)
-    grub_error (GRUB_ERR_INVALID_COMMAND, err);
+    grub_error (GRUB_ERR_INVALID_COMMAND, "%s", err);
 
   grub_print_error ();
   state->err++;
-- 
2.27.0

[PATCH v5 02/13] error: grub_error missing format string argument

[Glenn Washburn]

Its obvious from the error message that the variable named "type" was
accidentally omitted.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/parttool/msdospart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/parttool/msdospart.c b/grub-core/parttool/msdospart.c
index dcbf74e3b..2e2e9d527 100644
--- a/grub-core/parttool/msdospart.c
+++ b/grub-core/parttool/msdospart.c
@@ -127,8 +127,8 @@ static grub_err_t grub_pcpart_type (const grub_device_t dev,
     {
       dev->disk->partition = part;
       return grub_error (GRUB_ERR_BAD_ARGUMENT,
-			 N_("the partition type 0x%x isn't "
-			    "valid"));
+			 N_("the partition type 0x%x isn't valid"),
+			    type);
     }
 
   mbr.entries[index].type = type;
-- 
2.27.0

[PATCH v5 03/13] error: grub_error format string add missing format code

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/video/bochs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/video/bochs.c b/grub-core/video/bochs.c
index 7a249eb21..30ea1bd82 100644
--- a/grub-core/video/bochs.c
+++ b/grub-core/video/bochs.c
@@ -249,11 +249,11 @@ grub_video_bochs_setup (unsigned int width, unsigned int height,
     }
 
   if (width > BOCHS_MAX_WIDTH)
-    return grub_error (GRUB_ERR_IO, "width must be at most",
+    return grub_error (GRUB_ERR_IO, "width must be at most %d",
 		       BOCHS_MAX_WIDTH);
 
   if (height > BOCHS_MAX_HEIGHT)
-    return grub_error (GRUB_ERR_IO, "height must be at most",
+    return grub_error (GRUB_ERR_IO, "height must be at most %d",
 		       BOCHS_MAX_HEIGHT);
 
   if (width & (BOCHS_WIDTH_ALIGN - 1))
-- 
2.27.0

[PATCH v5 04/13] dmraid_nvidia: Format string error in grub_error

[Glenn Washburn]

The grub_error has a format string expecting two arguments, but only one
provided. According to the comments in the struct grub_nv_super definition,
the version field looks like a version number where major.minor is encoded
as each a byte in the two-byte short.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/dmraid_nvidia.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/disk/dmraid_nvidia.c b/grub-core/disk/dmraid_nvidia.c
index 4d2fb04d1..6372663e6 100644
--- a/grub-core/disk/dmraid_nvidia.c
+++ b/grub-core/disk/dmraid_nvidia.c
@@ -122,7 +122,7 @@ grub_dmraid_nv_detect (grub_disk_t disk,
   if (sb.version != NV_VERSION)
     {
       grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-		  "unknown version: %d.%d", sb.version);
+		  "unknown version: %d.%d", sb.version >> 8, sb.version & 0xFF);
       return NULL;
     }
 
-- 
2.27.0

[PATCH v5 05/13] grub_error: Use format code PRIuGRUB_SIZE for variables of type grub_size_t

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c        | 12 ++++++++----
 grub-core/kern/efi/efi.c           |  2 +-
 grub-core/loader/efi/chainloader.c |  2 +-
 grub-core/loader/i386/bsd.c        |  3 ++-
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b62835acc..4ff6654a0 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -484,13 +484,15 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
       }
       if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
 	{
-	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
+	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			    "Unsupported XTS block size: %"PRIuGRUB_SIZE,
 			    cipher->cipher->blocksize);
 	  goto err;
 	}
       if (secondary_cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
 	{
-	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
+	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			    "Unsupported XTS block size: %"PRIuGRUB_SIZE,
 			    secondary_cipher->cipher->blocksize);
 	  goto err;
 	}
@@ -501,7 +503,8 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
       cipheriv = ciphermode + sizeof ("lrw-") - 1;
       if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
 	{
-	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported LRW block size: %d",
+	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			    "Unsupported LRW block size: %"PRIuGRUB_SIZE,
 			    cipher->cipher->blocksize);
 	  goto err;
 	}
@@ -523,7 +526,8 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
     {
       if (cipher->cipher->blocksize & (cipher->cipher->blocksize - 1)
 	  || cipher->cipher->blocksize == 0)
-	grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported benbi blocksize: %d",
+	grub_error (GRUB_ERR_BAD_ARGUMENT,
+		    "Unsupported benbi blocksize: %"PRIuGRUB_SIZE,
 		    cipher->cipher->blocksize);
 	/* FIXME should we return an error here? */
       for (benbi_log = 0;
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 2942b8e35..d45cf67a9 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -503,7 +503,7 @@ grub_efi_duplicate_device_path (const grub_efi_device_path_t *dp)
       if (len < 4)
 	{
 	  grub_error (GRUB_ERR_OUT_OF_RANGE,
-		      "malformed EFI Device Path node has length=%d", len);
+		      "malformed EFI Device Path node has length=%"PRIuGRUB_SIZE, len);
 	  return NULL;
 	}
 
diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index 2e98947a0..b8d35f41e 100644
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -164,7 +164,7 @@ make_file_path (grub_efi_device_path_t *dp, const char *filename)
       if (len < 4)
 	{
 	  grub_error (GRUB_ERR_OUT_OF_RANGE,
-		      "malformed EFI Device Path node has length=%d", len);
+		      "malformed EFI Device Path node has length=%"PRIuGRUB_SIZE, len);
 	  return NULL;
 	}
 
diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index f5bf7f89e..fe20b339b 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -2110,7 +2110,8 @@ grub_cmd_openbsd_ramdisk (grub_command_t cmd __attribute__ ((unused)),
     {
       grub_file_close (file);
       return grub_error (GRUB_ERR_BAD_OS, "your kOpenBSD supports ramdisk only"
-			 " up to %u bytes, however you supplied a %u bytes one",
+			 " up to %"PRIuGRUB_SIZE" bytes, however you supplied"
+			 " a %"PRIuGRUB_SIZE" bytes one",
 			 openbsd_ramdisk.max_size, size);
     }
 
-- 
2.27.0

[PATCH v5 06/13] pgp: Format code for grub_error is incorrect

[Glenn Washburn]

The format code is for a 32-bit int, but the argument, keyid, is declared as
a 64 bit int. The comment above says keyid is 32-bit. I'm not sure if the
comment or declaration is wrong, so force the display of a 64-bit int for
now.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/pgp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
index bbf6871fe..150cb6fe9 100644
--- a/grub-core/commands/pgp.c
+++ b/grub-core/commands/pgp.c
@@ -633,7 +633,7 @@ grub_verify_signature_real (struct grub_pubkey_context *ctxt,
   if (!sk)
     {
       /* TRANSLATORS: %08x is 32-bit key id.  */
-      grub_error (GRUB_ERR_BAD_SIGNATURE, N_("public key %08x not found"),
+      grub_error (GRUB_ERR_BAD_SIGNATURE, N_("public key %08"PRIxGRUB_UINT64_T" not found"),
 		  keyid);
       goto fail;
     }
-- 
2.27.0

[PATCH v5 07/13] efi: Format string error in grub_error

[Glenn Washburn]

The second format string argument, GRUB_EFI_MAX_USABLE_ADDRESS, is a macro
to a number literal. However depending on what the target architecture, the
type can be 32 or 64 bits. Cast to a 64-bit integer.  Also, change the
format string literals %llx to use PRIxGRUB_UINT64_T.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/efi/mm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 457772d57..9d3d279fd 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -127,8 +127,9 @@ grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
   if (address > GRUB_EFI_MAX_USABLE_ADDRESS)
     {
       grub_error (GRUB_ERR_BAD_ARGUMENT,
-		  N_("invalid memory address (0x%llx > 0x%llx)"),
-		  address, GRUB_EFI_MAX_USABLE_ADDRESS);
+		  N_("invalid memory address (0x%"PRIxGRUB_UINT64_T
+                     " > 0x%"PRIxGRUB_UINT64_T")"),
+		  address, (grub_efi_uint64_t) GRUB_EFI_MAX_USABLE_ADDRESS);
       return NULL;
     }
 
-- 
2.27.0

[PATCH v5 08/13] error: Use PRI* macros to get correct format string code across architectures

[Glenn Washburn]

Also remove casting of format string args so that the architecture dependent
type is preserved.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/loader/i386/pc/linux.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/i386/pc/linux.c b/grub-core/loader/i386/pc/linux.c
index 814988ab9..2a2995201 100644
--- a/grub-core/loader/i386/pc/linux.c
+++ b/grub-core/loader/i386/pc/linux.c
@@ -230,9 +230,10 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
       && GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size
       > grub_linux_real_target)
     {
-      grub_error (GRUB_ERR_BAD_OS, "too big zImage (0x%x > 0x%x), use bzImage instead",
-		  (char *) GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size,
-		  (grub_size_t) grub_linux_real_target);
+      grub_error (GRUB_ERR_BAD_OS, "too big zImage (0x%" PRIxGRUB_SIZE
+		  " > 0x%" PRIxGRUB_ADDR "), use bzImage instead",
+		  GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size,
+		  grub_linux_real_target);
       goto fail;
     }
 
-- 
2.27.0

[PATCH v5 09/13] error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in grub_error

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/ata.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/ata.c b/grub-core/disk/ata.c
index 685f33a19..c8f350ed3 100644
--- a/grub-core/disk/ata.c
+++ b/grub-core/disk/ata.c
@@ -219,7 +219,7 @@ grub_ata_setaddress (struct grub_ata *dev,
 	if (dev->sectors_per_track == 0
 	    || dev->heads == 0)
 	  return grub_error (GRUB_ERR_OUT_OF_RANGE,
-			     "sector %d cannot be addressed "
+			     "sector %"PRIxGRUB_UINT64_T" cannot be addressed "
 			     "using CHS addressing", sector);
 
 	/* Calculate the sector, cylinder and head to use.  */
@@ -232,7 +232,7 @@ grub_ata_setaddress (struct grub_ata *dev,
 	    || cylinder > dev->cylinders
 	    || head > dev->heads)
 	  return grub_error (GRUB_ERR_OUT_OF_RANGE,
-			     "sector %d cannot be addressed "
+			     "sector %"PRIxGRUB_UINT64_T" cannot be addressed "
 			     "using CHS addressing", sector);
 	
 	parms->taskfile.disk = 0xE0 | head;
-- 
2.27.0

[PATCH v5 10/13] error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error

[Glenn Washburn]

The macro ELF_R_TYPE does not change the underlying type. Here its argument
is a 64-bit Elf64_Xword. Make sure the format code matches.

For the riscv architecture, rel->r_info could be either Elf32_Xword or
Elf64_Xword depending on if 32 or 64-bit risc is being built. So cast to
64-bit value regardless.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/efiemu/i386/loadcore64.c | 3 ++-
 grub-core/kern/arm64/dl.c          | 3 ++-
 grub-core/kern/ia64/dl.c           | 3 ++-
 grub-core/kern/riscv/dl.c          | 5 +++--
 grub-core/kern/sparc64/dl.c        | 3 ++-
 grub-core/kern/x86_64/dl.c         | 3 ++-
 6 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/grub-core/efiemu/i386/loadcore64.c b/grub-core/efiemu/i386/loadcore64.c
index 18facf47f..7316efc39 100644
--- a/grub-core/efiemu/i386/loadcore64.c
+++ b/grub-core/efiemu/i386/loadcore64.c
@@ -122,7 +122,8 @@ grub_arch_efiemu_relocate_symbols64 (grub_efiemu_segment_t segs,
                     break;
 		  default:
 		    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-				       N_("relocation 0x%x is not implemented yet"),
+				       N_("relocation 0x%" PRIxGRUB_UINT64_T
+					  " is not implemented yet"),
 				       ELF_R_TYPE (rel->r_info));
 		  }
 	      }
diff --git a/grub-core/kern/arm64/dl.c b/grub-core/kern/arm64/dl.c
index fb0337319..401672374 100644
--- a/grub-core/kern/arm64/dl.c
+++ b/grub-core/kern/arm64/dl.c
@@ -184,7 +184,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%" PRIxGRUB_UINT64_T
+				" is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
diff --git a/grub-core/kern/ia64/dl.c b/grub-core/kern/ia64/dl.c
index ebcf31629..ddf240976 100644
--- a/grub-core/kern/ia64/dl.c
+++ b/grub-core/kern/ia64/dl.c
@@ -137,7 +137,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 	  break;
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
+				" is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
diff --git a/grub-core/kern/riscv/dl.c b/grub-core/kern/riscv/dl.c
index 6fb8385ef..e6c4f89ff 100644
--- a/grub-core/kern/riscv/dl.c
+++ b/grub-core/kern/riscv/dl.c
@@ -331,8 +331,9 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 	  break;
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
-			     ELF_R_TYPE (rel->r_info));
+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
+				" is not implemented yet"),
+			     (grub_uint64_t) ELF_R_TYPE (rel->r_info));
 	}
     }
 
diff --git a/grub-core/kern/sparc64/dl.c b/grub-core/kern/sparc64/dl.c
index 739be4717..60ce183fb 100644
--- a/grub-core/kern/sparc64/dl.c
+++ b/grub-core/kern/sparc64/dl.c
@@ -177,7 +177,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 	  break;
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
+				" is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
diff --git a/grub-core/kern/x86_64/dl.c b/grub-core/kern/x86_64/dl.c
index 3a73e6e6c..8d2800c9e 100644
--- a/grub-core/kern/x86_64/dl.c
+++ b/grub-core/kern/x86_64/dl.c
@@ -107,7 +107,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
+			        " is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
-- 
2.27.0

[PATCH v5 11/13] error: Use format code PRIuGRUB_UINT64_T for 64-bit typed fileblock in grub_error

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/fs/hfsplus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 9c4e4c88c..b538f52e0 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -188,7 +188,8 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
 	  || !nnode)
 	{
 	  grub_error (GRUB_ERR_READ_ERROR,
-		      "no block found for the file id 0x%x and the block offset 0x%x",
+		      "no block found for the file id 0x%x and the block"
+		      " offset 0x%"PRIuGRUB_UINT64_T,
 		      node->fileid, fileblock);
 	  break;
 	}
-- 
2.27.0

[PATCH v5 12/13] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Glenn Washburn]

For some reason PRIuGRUB_UINT64_T is not expanding to llu, but to lu, which
causes the format string check to fail. Use literal and force cast until
this is debugged.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/fs/zfs/zfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index b6e1e178d..a3691d220 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -1869,8 +1869,8 @@ zio_read (blkptr_t *bp, grub_zfs_endian_t endian, void **buf,
     {
       if (BPE_GET_ETYPE(bp) != BP_EMBEDDED_TYPE_DATA)
 	return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			   "unsupported embedded BP (type=%u)\n",
-			   BPE_GET_ETYPE(bp));
+			   "unsupported embedded BP (type=%llu)\n",
+			   (long long unsigned int)BPE_GET_ETYPE(bp));
       lsize = BPE_GET_LSIZE(bp);
       psize = BF64_GET_SB(grub_zfs_to_cpu64 ((bp)->blk_prop, endian), 25, 7, 0, 1);
     }
-- 
2.27.0

Re: [PATCH v5 12/13] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Daniel Kiper]

On Wed, Mar 03, 2021 at 07:29:17PM -0600, Glenn Washburn wrote:
> For some reason PRIuGRUB_UINT64_T is not expanding to llu, but to lu, which
> causes the format string check to fail. Use literal and force cast until
> this is debugged.

We know the problem and how to fix it at this point. Though we do not
want to fix it because... So, please fix the commit message and explain
everything in it.

Additionally, I will take this and #13 patch if you provide now another
patchset on top of this one which properly fixes the issue as we
discussed earlier. This way I will be sure that we have a proper fix
for the issue ready for apply after 2.06 release.

By the way, if you rework this patchset please fix PRIx* and cast
formatting in all patches as I asked for in earlier comments.

...and I am OK with lines a bit longer than 80 chars if this increases
readability. So, do not hesitate to use that...

Daniel

> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/fs/zfs/zfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> index b6e1e178d..a3691d220 100644
> --- a/grub-core/fs/zfs/zfs.c
> +++ b/grub-core/fs/zfs/zfs.c
> @@ -1869,8 +1869,8 @@ zio_read (blkptr_t *bp, grub_zfs_endian_t endian, void **buf,
>      {
>        if (BPE_GET_ETYPE(bp) != BP_EMBEDDED_TYPE_DATA)
>  	return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> -			   "unsupported embedded BP (type=%u)\n",
> -			   BPE_GET_ETYPE(bp));
> +			   "unsupported embedded BP (type=%llu)\n",
> +			   (long long unsigned int)BPE_GET_ETYPE(bp));
>        lsize = BPE_GET_LSIZE(bp);
>        psize = BF64_GET_SB(grub_zfs_to_cpu64 ((bp)->blk_prop, endian), 25, 7, 0, 1);
>      }

Re: [PATCH v5 12/13] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Glenn Washburn]

On Thu, 4 Mar 2021 18:59:53 +0100
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Wed, Mar 03, 2021 at 07:29:17PM -0600, Glenn Washburn wrote:
> > For some reason PRIuGRUB_UINT64_T is not expanding to llu, but to
> > lu, which causes the format string check to fail. Use literal and
> > force cast until this is debugged.
> 
> We know the problem and how to fix it at this point. Though we do not
> want to fix it because... So, please fix the commit message and
> explain everything in it.
>
> Additionally, I will take this and #13 patch if you provide now
> another patchset on top of this one which properly fixes the issue as
> we discussed earlier. This way I will be sure that we have a proper
> fix for the issue ready for apply after 2.06 release.

Will do.

> By the way, if you rework this patchset please fix PRIx* and cast
> formatting in all patches as I asked for in earlier comments.

Ok, I didn't check all the patches but did fix the ones you explicitly
mentioned. I'll review the rest.

> ...and I am OK with lines a bit longer than 80 chars if this increases
> readability. So, do not hesitate to use that...

Thank you for the guidance.

Glenn

> 
> Daniel
> 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/fs/zfs/zfs.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> > index b6e1e178d..a3691d220 100644
> > --- a/grub-core/fs/zfs/zfs.c
> > +++ b/grub-core/fs/zfs/zfs.c
> > @@ -1869,8 +1869,8 @@ zio_read (blkptr_t *bp, grub_zfs_endian_t
> > endian, void **buf, {
> >        if (BPE_GET_ETYPE(bp) != BP_EMBEDDED_TYPE_DATA)
> >  	return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > -			   "unsupported embedded BP (type=%u)\n",
> > -			   BPE_GET_ETYPE(bp));
> > +			   "unsupported embedded BP (type=%llu)\n",
> > +			   (long long unsigned
> > int)BPE_GET_ETYPE(bp)); lsize = BPE_GET_LSIZE(bp);
> >        psize = BF64_GET_SB(grub_zfs_to_cpu64 ((bp)->blk_prop,
> > endian), 25, 7, 0, 1); }

[PATCH v5 13/13] error: Do compile-time format string checking on grub_error

[Glenn Washburn]

This should help prevent format string errors and thus improve the quality
of error reporting.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 include/grub/err.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/grub/err.h b/include/grub/err.h
index 24ba9f5f5..b08d5d0de 100644
--- a/include/grub/err.h
+++ b/include/grub/err.h
@@ -85,7 +85,8 @@ struct grub_error_saved
 extern grub_err_t EXPORT_VAR(grub_errno);
 extern char EXPORT_VAR(grub_errmsg)[GRUB_MAX_ERRMSG];
 
-grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...);
+grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...)
+    __attribute__ ((format (GNU_PRINTF, 2, 3)));
 void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
 void EXPORT_FUNC(grub_error_push) (void);
 int EXPORT_FUNC(grub_error_pop) (void);
-- 
2.27.0

[PATCH v6 00/14] error: Do compile-time format string checking on grub>

[Glenn Washburn]

Daniel, you mentioned wanting a separate patch series which is the real fix
for patch #12. I've added it to this patch series, since they go together.
I can send the single patch as a separate thread if that still desirable.

Changes since v5
 * Fix formatting issues with spaces around format string macros and casts
 * Add extra patch #14 which is the better fix for #12, but needs more testing

Glenn

Glenn Washburn (14):
  misc: Format string for grub_error should be a literal
  error: grub_error missing format string argument
  error: grub_error format string add missing format code
  dmraid_nvidia: Format string error in grub_error
  grub_error: Use format code PRIuGRUB_SIZE for variables of type
    grub_size_t
  pgp: Format code for grub_error is incorrect
  efi: Format string error in grub_error
  error: Use PRI* macros to get correct format string code across
    architectures
  error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in
    grub_error
  error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error
  error: Use format code PRIuGRUB_UINT64_T for 64-bit typed fileblock in
    grub_error
  error: Use format code llu for 64-bit uint bp->blk_prop in grub_error
  error: Do compile-time format string checking on grub_error
  zfs: Use grub_uint64_t instead of 1ULL in BF64_*CODE macros

 grub-core/commands/pgp.c           |  4 ++--
 grub-core/disk/ata.c               | 10 ++++++----
 grub-core/disk/cryptodisk.c        | 12 ++++++++----
 grub-core/disk/dmraid_nvidia.c     |  2 +-
 grub-core/efiemu/i386/loadcore64.c |  3 ++-
 grub-core/fs/hfsplus.c             |  3 ++-
 grub-core/fs/zfs/zfs.c             |  3 ++-
 grub-core/kern/arm64/dl.c          |  3 ++-
 grub-core/kern/efi/efi.c           |  2 +-
 grub-core/kern/efi/mm.c            |  5 +++--
 grub-core/kern/ia64/dl.c           |  3 ++-
 grub-core/kern/riscv/dl.c          |  5 +++--
 grub-core/kern/sparc64/dl.c        |  3 ++-
 grub-core/kern/x86_64/dl.c         |  3 ++-
 grub-core/loader/efi/chainloader.c |  4 ++--
 grub-core/loader/i386/bsd.c        |  3 ++-
 grub-core/loader/i386/pc/linux.c   |  7 ++++---
 grub-core/net/tftp.c               |  2 +-
 grub-core/parttool/msdospart.c     |  4 ++--
 grub-core/script/lexer.c           |  2 +-
 grub-core/video/bochs.c            |  4 ++--
 include/grub/err.h                 |  3 ++-
 include/grub/zfs/spa.h             |  4 ++--
 23 files changed, 56 insertions(+), 38 deletions(-)

Range-diff against v5:
 1:  f34399c63 !  1:  0f35c04e3 grub_error: Use format code PRIuGRUB_SIZE for variables of type grub_size_t
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_setcipher (grub_cryptodisk_t crypt,
      	{
     -	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
     +	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
    -+			    "Unsupported XTS block size: %"PRIuGRUB_SIZE,
    ++			    "Unsupported XTS block size: %" PRIuGRUB_SIZE,
      			    cipher->cipher->blocksize);
      	  goto err;
      	}
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_setcipher (grub_cryptodisk_t crypt,
      	{
     -	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
     +	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
    -+			    "Unsupported XTS block size: %"PRIuGRUB_SIZE,
    ++			    "Unsupported XTS block size: %" PRIuGRUB_SIZE,
      			    secondary_cipher->cipher->blocksize);
      	  goto err;
      	}
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_setcipher (grub_cryptodisk_t crypt,
      	{
     -	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported LRW block size: %d",
     +	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
    -+			    "Unsupported LRW block size: %"PRIuGRUB_SIZE,
    ++			    "Unsupported LRW block size: %" PRIuGRUB_SIZE,
      			    cipher->cipher->blocksize);
      	  goto err;
      	}
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_setcipher (grub_cryptodisk_t crypt,
      	  || cipher->cipher->blocksize == 0)
     -	grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported benbi blocksize: %d",
     +	grub_error (GRUB_ERR_BAD_ARGUMENT,
    -+		    "Unsupported benbi blocksize: %"PRIuGRUB_SIZE,
    ++		    "Unsupported benbi blocksize: %" PRIuGRUB_SIZE,
      		    cipher->cipher->blocksize);
      	/* FIXME should we return an error here? */
            for (benbi_log = 0;
    @@ grub-core/kern/efi/efi.c: grub_efi_duplicate_device_path (const grub_efi_device_
      	{
      	  grub_error (GRUB_ERR_OUT_OF_RANGE,
     -		      "malformed EFI Device Path node has length=%d", len);
    -+		      "malformed EFI Device Path node has length=%"PRIuGRUB_SIZE, len);
    ++		      "malformed EFI Device Path node has length=%" PRIuGRUB_SIZE, len);
      	  return NULL;
      	}
      
    @@ grub-core/loader/efi/chainloader.c: make_file_path (grub_efi_device_path_t *dp,
      	{
      	  grub_error (GRUB_ERR_OUT_OF_RANGE,
     -		      "malformed EFI Device Path node has length=%d", len);
    -+		      "malformed EFI Device Path node has length=%"PRIuGRUB_SIZE, len);
    ++		      "malformed EFI Device Path node has length=%" PRIuGRUB_SIZE, len);
      	  return NULL;
      	}
      
    @@ grub-core/loader/i386/bsd.c: grub_cmd_openbsd_ramdisk (grub_command_t cmd __attr
            grub_file_close (file);
            return grub_error (GRUB_ERR_BAD_OS, "your kOpenBSD supports ramdisk only"
     -			 " up to %u bytes, however you supplied a %u bytes one",
    -+			 " up to %"PRIuGRUB_SIZE" bytes, however you supplied"
    -+			 " a %"PRIuGRUB_SIZE" bytes one",
    ++			 " up to %" PRIuGRUB_SIZE " bytes, however you supplied"
    ++			 " a %" PRIuGRUB_SIZE " bytes one",
      			 openbsd_ramdisk.max_size, size);
          }
      
 2:  30c3c79fe !  2:  bfff8c10f pgp: Format code for grub_error is incorrect
    @@ grub-core/commands/pgp.c: grub_verify_signature_real (struct grub_pubkey_context
          {
            /* TRANSLATORS: %08x is 32-bit key id.  */
     -      grub_error (GRUB_ERR_BAD_SIGNATURE, N_("public key %08x not found"),
    -+      grub_error (GRUB_ERR_BAD_SIGNATURE, N_("public key %08"PRIxGRUB_UINT64_T" not found"),
    - 		  keyid);
    +-		  keyid);
    ++      grub_error (GRUB_ERR_BAD_SIGNATURE,
    ++		  N_("public key %08" PRIxGRUB_UINT64_T " not found"), keyid);
            goto fail;
          }
    + 
 3:  ed754692d !  3:  df4e82eac efi: Format string error in grub_error
    @@ grub-core/kern/efi/mm.c: grub_efi_allocate_pages_real (grub_efi_physical_address
            grub_error (GRUB_ERR_BAD_ARGUMENT,
     -		  N_("invalid memory address (0x%llx > 0x%llx)"),
     -		  address, GRUB_EFI_MAX_USABLE_ADDRESS);
    -+		  N_("invalid memory address (0x%"PRIxGRUB_UINT64_T
    -+                     " > 0x%"PRIxGRUB_UINT64_T")"),
    ++		  N_("invalid memory address (0x%" PRIxGRUB_UINT64_T
    ++                     " > 0x%" PRIxGRUB_UINT64_T ")"),
     +		  address, (grub_efi_uint64_t) GRUB_EFI_MAX_USABLE_ADDRESS);
            return NULL;
          }
 4:  6a418777d =  4:  6846574fb error: Use PRI* macros to get correct format string code across architectures
 5:  ea743a1d4 <  -:  --------- error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in grub_error
 -:  --------- >  5:  d059639a1 error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in grub_error
 6:  40c210e62 !  6:  1bfdfa47e error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error
    @@ grub-core/kern/ia64/dl.c: grub_arch_dl_relocate_symbols (grub_dl_t mod, void *eh
      	default:
      	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
     -			     N_("relocation 0x%x is not implemented yet"),
    -+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
    ++			     N_("relocation 0x%" PRIxGRUB_UINT64_T
     +				" is not implemented yet"),
      			     ELF_R_TYPE (rel->r_info));
      	}
    @@ grub-core/kern/riscv/dl.c: grub_arch_dl_relocate_symbols (grub_dl_t mod, void *e
      	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
     -			     N_("relocation 0x%x is not implemented yet"),
     -			     ELF_R_TYPE (rel->r_info));
    -+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
    ++			     N_("relocation 0x%" PRIxGRUB_UINT64_T
     +				" is not implemented yet"),
     +			     (grub_uint64_t) ELF_R_TYPE (rel->r_info));
      	}
    @@ grub-core/kern/sparc64/dl.c: grub_arch_dl_relocate_symbols (grub_dl_t mod, void
      	default:
      	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
     -			     N_("relocation 0x%x is not implemented yet"),
    -+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
    ++			     N_("relocation 0x%" PRIxGRUB_UINT64_T
     +				" is not implemented yet"),
      			     ELF_R_TYPE (rel->r_info));
      	}
    @@ grub-core/kern/x86_64/dl.c: grub_arch_dl_relocate_symbols (grub_dl_t mod, void *
      	default:
      	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
     -			     N_("relocation 0x%x is not implemented yet"),
    -+			     N_("relocation 0x%"PRIxGRUB_UINT64_T
    ++			     N_("relocation 0x%" PRIxGRUB_UINT64_T
     +			        " is not implemented yet"),
      			     ELF_R_TYPE (rel->r_info));
      	}
 7:  f9b7933f9 !  7:  bacdaf296 error: Use format code PRIuGRUB_UINT64_T for 64-bit typed fileblock in grub_error
    @@ grub-core/fs/hfsplus.c: grub_hfsplus_read_block (grub_fshelp_node_t node, grub_d
      	  grub_error (GRUB_ERR_READ_ERROR,
     -		      "no block found for the file id 0x%x and the block offset 0x%x",
     +		      "no block found for the file id 0x%x and the block"
    -+		      " offset 0x%"PRIuGRUB_UINT64_T,
    ++		      " offset 0x%" PRIuGRUB_UINT64_T,
      		      node->fileid, fileblock);
      	  break;
      	}
 8:  de6688623 !  8:  88a885a40 error: Use format code llu for 64-bit uint bp->blk_prop in grub_error
    @@ Metadata
      ## Commit message ##
         error: Use format code llu for 64-bit uint bp->blk_prop in grub_error
     
    -    For some reason PRIuGRUB_UINT64_T is not expanding to llu, but to lu, which
    -    causes the format string check to fail. Use literal and force cast until
    -    this is debugged.
    +    This is a temporary, less-intrusive change to get the build to success with
    +    compiler format string checking turned on. There is a better fix which
    +    addresses this issue, but it needs more testing. Use this change so that
    +    format string checking on grub_error can be turned on until the better
    +    change is fully tested.
     
      ## grub-core/fs/zfs/zfs.c ##
     @@ grub-core/fs/zfs/zfs.c: zio_read (blkptr_t *bp, grub_zfs_endian_t endian, void **buf,
    @@ grub-core/fs/zfs/zfs.c: zio_read (blkptr_t *bp, grub_zfs_endian_t endian, void *
     -			   "unsupported embedded BP (type=%u)\n",
     -			   BPE_GET_ETYPE(bp));
     +			   "unsupported embedded BP (type=%llu)\n",
    -+			   (long long unsigned int)BPE_GET_ETYPE(bp));
    ++			   (long long unsigned int) BPE_GET_ETYPE(bp));
            lsize = BPE_GET_LSIZE(bp);
            psize = BF64_GET_SB(grub_zfs_to_cpu64 ((bp)->blk_prop, endian), 25, 7, 0, 1);
          }
 9:  d071036a3 =  9:  d4095f724 error: Do compile-time format string checking on grub_error
10:  bba547214 ! 10:  004d029b5 fixup: error: Use format code llu for 64-bit uint bp->blk_prop in grub_error
    @@ Metadata
     Author: Glenn Washburn <development@efficientek.com>
     
      ## Commit message ##
    -    fixup: error: Use format code llu for 64-bit uint bp->blk_prop in grub_error
    +    zfs: Use grub_uint64_t instead of 1ULL in BF64_*CODE macros
    +
    +    The underlying type of grub_uint64_t changes across architectures, but 1ULL
    +    does not. This allows using these macros as arguments to format string
    +    functions that use the PRI* format string macros that also vary with
    +    architecture.
    +
    +    Change the grub_error call, where this was previously an issue and
    +    temporarily fixed by casting and using a format string literal code, to now
    +    use PRI* macros and remove casting.
     
      ## grub-core/fs/zfs/zfs.c ##
     @@ grub-core/fs/zfs/zfs.c: zio_read (blkptr_t *bp, grub_zfs_endian_t endian, void **buf,
    @@ grub-core/fs/zfs/zfs.c: zio_read (blkptr_t *bp, grub_zfs_endian_t endian, void *
            if (BPE_GET_ETYPE(bp) != BP_EMBEDDED_TYPE_DATA)
      	return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
     -			   "unsupported embedded BP (type=%llu)\n",
    --			   (long long unsigned int)BPE_GET_ETYPE(bp));
    +-			   (long long unsigned int) BPE_GET_ETYPE(bp));
     +			   "unsupported embedded BP (type=%"
     +			   PRIuGRUB_UINT64_T ")\n",
     +			   BPE_GET_ETYPE(bp));
-- 
2.27.0

[PATCH v6 01/14] misc: Format string for grub_error should be a literal

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/loader/efi/chainloader.c | 2 +-
 grub-core/net/tftp.c               | 2 +-
 grub-core/script/lexer.c           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index 7b31c3fb9..2e98947a0 100644
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -90,7 +90,7 @@ grub_chainloader_boot (void)
 	      *grub_utf16_to_utf8 ((grub_uint8_t *) buf,
 				   exit_data, exit_data_size) = 0;
 
-	      grub_error (GRUB_ERR_BAD_OS, buf);
+	      grub_error (GRUB_ERR_BAD_OS, "%s", buf);
 	      grub_free (buf);
 	    }
 	}
diff --git a/grub-core/net/tftp.c b/grub-core/net/tftp.c
index 1f3793ed8..7f44b30f5 100644
--- a/grub-core/net/tftp.c
+++ b/grub-core/net/tftp.c
@@ -252,7 +252,7 @@ tftp_receive (grub_net_udp_socket_t sock __attribute__ ((unused)),
     case TFTP_ERROR:
       data->have_oack = 1;
       grub_netbuff_free (nb);
-      grub_error (GRUB_ERR_IO, (char *) tftph->u.err.errmsg);
+      grub_error (GRUB_ERR_IO, "%s", tftph->u.err.errmsg);
       grub_error_save (&data->save_err);
       return GRUB_ERR_NONE;
     default:
diff --git a/grub-core/script/lexer.c b/grub-core/script/lexer.c
index 57778f881..52004d059 100644
--- a/grub-core/script/lexer.c
+++ b/grub-core/script/lexer.c
@@ -349,7 +349,7 @@ void
 grub_script_yyerror (struct grub_parser_param *state, const char *err)
 {
   if (err)
-    grub_error (GRUB_ERR_INVALID_COMMAND, err);
+    grub_error (GRUB_ERR_INVALID_COMMAND, "%s", err);
 
   grub_print_error ();
   state->err++;
-- 
2.27.0

[PATCH v6 02/14] error: grub_error missing format string argument

[Glenn Washburn]

Its obvious from the error message that the variable named "type" was
accidentally omitted.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/parttool/msdospart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/parttool/msdospart.c b/grub-core/parttool/msdospart.c
index dcbf74e3b..2e2e9d527 100644
--- a/grub-core/parttool/msdospart.c
+++ b/grub-core/parttool/msdospart.c
@@ -127,8 +127,8 @@ static grub_err_t grub_pcpart_type (const grub_device_t dev,
     {
       dev->disk->partition = part;
       return grub_error (GRUB_ERR_BAD_ARGUMENT,
-			 N_("the partition type 0x%x isn't "
-			    "valid"));
+			 N_("the partition type 0x%x isn't valid"),
+			    type);
     }
 
   mbr.entries[index].type = type;
-- 
2.27.0

[PATCH v6 03/14] error: grub_error format string add missing format code

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/video/bochs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/video/bochs.c b/grub-core/video/bochs.c
index 7a249eb21..30ea1bd82 100644
--- a/grub-core/video/bochs.c
+++ b/grub-core/video/bochs.c
@@ -249,11 +249,11 @@ grub_video_bochs_setup (unsigned int width, unsigned int height,
     }
 
   if (width > BOCHS_MAX_WIDTH)
-    return grub_error (GRUB_ERR_IO, "width must be at most",
+    return grub_error (GRUB_ERR_IO, "width must be at most %d",
 		       BOCHS_MAX_WIDTH);
 
   if (height > BOCHS_MAX_HEIGHT)
-    return grub_error (GRUB_ERR_IO, "height must be at most",
+    return grub_error (GRUB_ERR_IO, "height must be at most %d",
 		       BOCHS_MAX_HEIGHT);
 
   if (width & (BOCHS_WIDTH_ALIGN - 1))
-- 
2.27.0

[PATCH v6 04/14] dmraid_nvidia: Format string error in grub_error

[Glenn Washburn]

The grub_error has a format string expecting two arguments, but only one
provided. According to the comments in the struct grub_nv_super definition,
the version field looks like a version number where major.minor is encoded
as each a byte in the two-byte short.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/dmraid_nvidia.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/disk/dmraid_nvidia.c b/grub-core/disk/dmraid_nvidia.c
index 4d2fb04d1..6372663e6 100644
--- a/grub-core/disk/dmraid_nvidia.c
+++ b/grub-core/disk/dmraid_nvidia.c
@@ -122,7 +122,7 @@ grub_dmraid_nv_detect (grub_disk_t disk,
   if (sb.version != NV_VERSION)
     {
       grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-		  "unknown version: %d.%d", sb.version);
+		  "unknown version: %d.%d", sb.version >> 8, sb.version & 0xFF);
       return NULL;
     }
 
-- 
2.27.0

[PATCH v6 05/14] grub_error: Use format code PRIuGRUB_SIZE for variables of type grub_size_t

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c        | 12 ++++++++----
 grub-core/kern/efi/efi.c           |  2 +-
 grub-core/loader/efi/chainloader.c |  2 +-
 grub-core/loader/i386/bsd.c        |  3 ++-
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 41866c62d..90f82b2d3 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -484,13 +484,15 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
       }
       if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
 	{
-	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
+	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			    "Unsupported XTS block size: %" PRIuGRUB_SIZE,
 			    cipher->cipher->blocksize);
 	  goto err;
 	}
       if (secondary_cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
 	{
-	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
+	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			    "Unsupported XTS block size: %" PRIuGRUB_SIZE,
 			    secondary_cipher->cipher->blocksize);
 	  goto err;
 	}
@@ -501,7 +503,8 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
       cipheriv = ciphermode + sizeof ("lrw-") - 1;
       if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
 	{
-	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported LRW block size: %d",
+	  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+			    "Unsupported LRW block size: %" PRIuGRUB_SIZE,
 			    cipher->cipher->blocksize);
 	  goto err;
 	}
@@ -523,7 +526,8 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
     {
       if (cipher->cipher->blocksize & (cipher->cipher->blocksize - 1)
 	  || cipher->cipher->blocksize == 0)
-	grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported benbi blocksize: %d",
+	grub_error (GRUB_ERR_BAD_ARGUMENT,
+		    "Unsupported benbi blocksize: %" PRIuGRUB_SIZE,
 		    cipher->cipher->blocksize);
 	/* FIXME should we return an error here? */
       for (benbi_log = 0;
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 67394bb6f..8cff7be02 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -504,7 +504,7 @@ grub_efi_duplicate_device_path (const grub_efi_device_path_t *dp)
       if (len < 4)
 	{
 	  grub_error (GRUB_ERR_OUT_OF_RANGE,
-		      "malformed EFI Device Path node has length=%d", len);
+		      "malformed EFI Device Path node has length=%" PRIuGRUB_SIZE, len);
 	  return NULL;
 	}
 
diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index 2e98947a0..2bd80f4db 100644
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -164,7 +164,7 @@ make_file_path (grub_efi_device_path_t *dp, const char *filename)
       if (len < 4)
 	{
 	  grub_error (GRUB_ERR_OUT_OF_RANGE,
-		      "malformed EFI Device Path node has length=%d", len);
+		      "malformed EFI Device Path node has length=%" PRIuGRUB_SIZE, len);
 	  return NULL;
 	}
 
diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index d89ff0a7a..5f3290ce1 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -2110,7 +2110,8 @@ grub_cmd_openbsd_ramdisk (grub_command_t cmd __attribute__ ((unused)),
     {
       grub_file_close (file);
       return grub_error (GRUB_ERR_BAD_OS, "your kOpenBSD supports ramdisk only"
-			 " up to %u bytes, however you supplied a %u bytes one",
+			 " up to %" PRIuGRUB_SIZE " bytes, however you supplied"
+			 " a %" PRIuGRUB_SIZE " bytes one",
 			 openbsd_ramdisk.max_size, size);
     }
 
-- 
2.27.0

[PATCH v6 06/14] pgp: Format code for grub_error is incorrect

[Glenn Washburn]

The format code is for a 32-bit int, but the argument, keyid, is declared as
a 64 bit int. The comment above says keyid is 32-bit. I'm not sure if the
comment or declaration is wrong, so force the display of a 64-bit int for
now.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/pgp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
index bbf6871fe..5daa1e9d0 100644
--- a/grub-core/commands/pgp.c
+++ b/grub-core/commands/pgp.c
@@ -633,8 +633,8 @@ grub_verify_signature_real (struct grub_pubkey_context *ctxt,
   if (!sk)
     {
       /* TRANSLATORS: %08x is 32-bit key id.  */
-      grub_error (GRUB_ERR_BAD_SIGNATURE, N_("public key %08x not found"),
-		  keyid);
+      grub_error (GRUB_ERR_BAD_SIGNATURE,
+		  N_("public key %08" PRIxGRUB_UINT64_T " not found"), keyid);
       goto fail;
     }
 
-- 
2.27.0

[PATCH v6 07/14] efi: Format string error in grub_error

[Glenn Washburn]

The second format string argument, GRUB_EFI_MAX_USABLE_ADDRESS, is a macro
to a number literal. However depending on what the target architecture, the
type can be 32 or 64 bits. Cast to a 64-bit integer.  Also, change the
format string literals %llx to use PRIxGRUB_UINT64_T.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/efi/mm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 8d380c054..0cdb063bb 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -127,8 +127,9 @@ grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
   if (address > GRUB_EFI_MAX_USABLE_ADDRESS)
     {
       grub_error (GRUB_ERR_BAD_ARGUMENT,
-		  N_("invalid memory address (0x%llx > 0x%llx)"),
-		  address, GRUB_EFI_MAX_USABLE_ADDRESS);
+		  N_("invalid memory address (0x%" PRIxGRUB_UINT64_T
+                     " > 0x%" PRIxGRUB_UINT64_T ")"),
+		  address, (grub_efi_uint64_t) GRUB_EFI_MAX_USABLE_ADDRESS);
       return NULL;
     }
 
-- 
2.27.0

[PATCH v6 08/14] error: Use PRI* macros to get correct format string code across architectures

[Glenn Washburn]

Also remove casting of format string args so that the architecture dependent
type is preserved.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/loader/i386/pc/linux.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/i386/pc/linux.c b/grub-core/loader/i386/pc/linux.c
index 814988ab9..2a2995201 100644
--- a/grub-core/loader/i386/pc/linux.c
+++ b/grub-core/loader/i386/pc/linux.c
@@ -230,9 +230,10 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
       && GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size
       > grub_linux_real_target)
     {
-      grub_error (GRUB_ERR_BAD_OS, "too big zImage (0x%x > 0x%x), use bzImage instead",
-		  (char *) GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size,
-		  (grub_size_t) grub_linux_real_target);
+      grub_error (GRUB_ERR_BAD_OS, "too big zImage (0x%" PRIxGRUB_SIZE
+		  " > 0x%" PRIxGRUB_ADDR "), use bzImage instead",
+		  GRUB_LINUX_ZIMAGE_ADDR + grub_linux16_prot_size,
+		  grub_linux_real_target);
       goto fail;
     }
 
-- 
2.27.0

[PATCH v6 09/14] error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in grub_error

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/ata.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/grub-core/disk/ata.c b/grub-core/disk/ata.c
index 685f33a19..3620a282e 100644
--- a/grub-core/disk/ata.c
+++ b/grub-core/disk/ata.c
@@ -219,8 +219,9 @@ grub_ata_setaddress (struct grub_ata *dev,
 	if (dev->sectors_per_track == 0
 	    || dev->heads == 0)
 	  return grub_error (GRUB_ERR_OUT_OF_RANGE,
-			     "sector %d cannot be addressed "
-			     "using CHS addressing", sector);
+			     "sector %" PRIxGRUB_UINT64_T " cannot be "
+			     "addressed using CHS addressing",
+			     sector);
 
 	/* Calculate the sector, cylinder and head to use.  */
 	sect = ((grub_uint32_t) sector % dev->sectors_per_track) + 1;
@@ -232,8 +233,9 @@ grub_ata_setaddress (struct grub_ata *dev,
 	    || cylinder > dev->cylinders
 	    || head > dev->heads)
 	  return grub_error (GRUB_ERR_OUT_OF_RANGE,
-			     "sector %d cannot be addressed "
-			     "using CHS addressing", sector);
+			     "sector %" PRIxGRUB_UINT64_T " cannot be "
+			     "addressed using CHS addressing",
+			     sector);
 	
 	parms->taskfile.disk = 0xE0 | head;
 	parms->taskfile.sectnum = sect;
-- 
2.27.0

[PATCH v6 10/14] error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error

[Glenn Washburn]

The macro ELF_R_TYPE does not change the underlying type. Here its argument
is a 64-bit Elf64_Xword. Make sure the format code matches.

For the riscv architecture, rel->r_info could be either Elf32_Xword or
Elf64_Xword depending on if 32 or 64-bit risc is being built. So cast to
64-bit value regardless.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/efiemu/i386/loadcore64.c | 3 ++-
 grub-core/kern/arm64/dl.c          | 3 ++-
 grub-core/kern/ia64/dl.c           | 3 ++-
 grub-core/kern/riscv/dl.c          | 5 +++--
 grub-core/kern/sparc64/dl.c        | 3 ++-
 grub-core/kern/x86_64/dl.c         | 3 ++-
 6 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/grub-core/efiemu/i386/loadcore64.c b/grub-core/efiemu/i386/loadcore64.c
index 18facf47f..7316efc39 100644
--- a/grub-core/efiemu/i386/loadcore64.c
+++ b/grub-core/efiemu/i386/loadcore64.c
@@ -122,7 +122,8 @@ grub_arch_efiemu_relocate_symbols64 (grub_efiemu_segment_t segs,
                     break;
 		  default:
 		    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-				       N_("relocation 0x%x is not implemented yet"),
+				       N_("relocation 0x%" PRIxGRUB_UINT64_T
+					  " is not implemented yet"),
 				       ELF_R_TYPE (rel->r_info));
 		  }
 	      }
diff --git a/grub-core/kern/arm64/dl.c b/grub-core/kern/arm64/dl.c
index fb0337319..401672374 100644
--- a/grub-core/kern/arm64/dl.c
+++ b/grub-core/kern/arm64/dl.c
@@ -184,7 +184,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%" PRIxGRUB_UINT64_T
+				" is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
diff --git a/grub-core/kern/ia64/dl.c b/grub-core/kern/ia64/dl.c
index ebcf31629..b19706c50 100644
--- a/grub-core/kern/ia64/dl.c
+++ b/grub-core/kern/ia64/dl.c
@@ -137,7 +137,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 	  break;
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%" PRIxGRUB_UINT64_T
+				" is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
diff --git a/grub-core/kern/riscv/dl.c b/grub-core/kern/riscv/dl.c
index 6fb8385ef..d78297eee 100644
--- a/grub-core/kern/riscv/dl.c
+++ b/grub-core/kern/riscv/dl.c
@@ -331,8 +331,9 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 	  break;
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
-			     ELF_R_TYPE (rel->r_info));
+			     N_("relocation 0x%" PRIxGRUB_UINT64_T
+				" is not implemented yet"),
+			     (grub_uint64_t) ELF_R_TYPE (rel->r_info));
 	}
     }
 
diff --git a/grub-core/kern/sparc64/dl.c b/grub-core/kern/sparc64/dl.c
index 739be4717..bbcce8ed5 100644
--- a/grub-core/kern/sparc64/dl.c
+++ b/grub-core/kern/sparc64/dl.c
@@ -177,7 +177,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 	  break;
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%" PRIxGRUB_UINT64_T
+				" is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
diff --git a/grub-core/kern/x86_64/dl.c b/grub-core/kern/x86_64/dl.c
index 3a73e6e6c..1af5a0eeb 100644
--- a/grub-core/kern/x86_64/dl.c
+++ b/grub-core/kern/x86_64/dl.c
@@ -107,7 +107,8 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
 
 	default:
 	  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			     N_("relocation 0x%x is not implemented yet"),
+			     N_("relocation 0x%" PRIxGRUB_UINT64_T
+			        " is not implemented yet"),
 			     ELF_R_TYPE (rel->r_info));
 	}
     }
-- 
2.27.0

[PATCH v6 11/14] error: Use format code PRIuGRUB_UINT64_T for 64-bit typed fileblock in grub_error

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/fs/hfsplus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 361e5be49..2a69055c7 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -199,7 +199,8 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
 	  || !nnode)
 	{
 	  grub_error (GRUB_ERR_READ_ERROR,
-		      "no block found for the file id 0x%x and the block offset 0x%x",
+		      "no block found for the file id 0x%x and the block"
+		      " offset 0x%" PRIuGRUB_UINT64_T,
 		      node->fileid, fileblock);
 	  break;
 	}
-- 
2.27.0

[PATCH v6 12/14] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Glenn Washburn]

This is a temporary, less-intrusive change to get the build to success with
compiler format string checking turned on. There is a better fix which
addresses this issue, but it needs more testing. Use this change so that
format string checking on grub_error can be turned on until the better
change is fully tested.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/fs/zfs/zfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index 9ef2fd61a..f9e755197 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -1869,8 +1869,8 @@ zio_read (blkptr_t *bp, grub_zfs_endian_t endian, void **buf,
     {
       if (BPE_GET_ETYPE(bp) != BP_EMBEDDED_TYPE_DATA)
 	return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			   "unsupported embedded BP (type=%u)\n",
-			   BPE_GET_ETYPE(bp));
+			   "unsupported embedded BP (type=%llu)\n",
+			   (long long unsigned int) BPE_GET_ETYPE(bp));
       lsize = BPE_GET_LSIZE(bp);
       psize = BF64_GET_SB(grub_zfs_to_cpu64 ((bp)->blk_prop, endian), 25, 7, 0, 1);
     }
-- 
2.27.0

[PATCH v6 13/14] error: Do compile-time format string checking on grub_error

[Glenn Washburn]

This should help prevent format string errors and thus improve the quality
of error reporting.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 include/grub/err.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/grub/err.h b/include/grub/err.h
index 24ba9f5f5..b08d5d0de 100644
--- a/include/grub/err.h
+++ b/include/grub/err.h
@@ -85,7 +85,8 @@ struct grub_error_saved
 extern grub_err_t EXPORT_VAR(grub_errno);
 extern char EXPORT_VAR(grub_errmsg)[GRUB_MAX_ERRMSG];
 
-grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...);
+grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...)
+    __attribute__ ((format (GNU_PRINTF, 2, 3)));
 void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
 void EXPORT_FUNC(grub_error_push) (void);
 int EXPORT_FUNC(grub_error_pop) (void);
-- 
2.27.0

[PATCH v6 14/14] zfs: Use grub_uint64_t instead of 1ULL in BF64_*CODE macros

[Glenn Washburn]

The underlying type of grub_uint64_t changes across architectures, but 1ULL
does not. This allows using these macros as arguments to format string
functions that use the PRI* format string macros that also vary with
architecture.

Change the grub_error call, where this was previously an issue and
temporarily fixed by casting and using a format string literal code, to now
use PRI* macros and remove casting.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/fs/zfs/zfs.c | 5 +++--
 include/grub/zfs/spa.h | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index f9e755197..a266a936e 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -1869,8 +1869,9 @@ zio_read (blkptr_t *bp, grub_zfs_endian_t endian, void **buf,
     {
       if (BPE_GET_ETYPE(bp) != BP_EMBEDDED_TYPE_DATA)
 	return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-			   "unsupported embedded BP (type=%llu)\n",
-			   (long long unsigned int) BPE_GET_ETYPE(bp));
+			   "unsupported embedded BP (type=%"
+			   PRIuGRUB_UINT64_T ")\n",
+			   BPE_GET_ETYPE(bp));
       lsize = BPE_GET_LSIZE(bp);
       psize = BF64_GET_SB(grub_zfs_to_cpu64 ((bp)->blk_prop, endian), 25, 7, 0, 1);
     }
diff --git a/include/grub/zfs/spa.h b/include/grub/zfs/spa.h
index 8dd1fa8e3..5afbe4ecd 100644
--- a/include/grub/zfs/spa.h
+++ b/include/grub/zfs/spa.h
@@ -44,9 +44,9 @@
  * General-purpose 32-bit and 64-bit bitfield encodings.
  */
 #define	BF32_DECODE(x, low, len)	P2PHASE((x) >> (low), 1U << (len))
-#define	BF64_DECODE(x, low, len)	P2PHASE((x) >> (low), 1ULL << (len))
+#define	BF64_DECODE(x, low, len)	P2PHASE((x) >> (low), ((grub_uint64_t) 1) << (len))
 #define	BF32_ENCODE(x, low, len)	(P2PHASE((x), 1U << (len)) << (low))
-#define	BF64_ENCODE(x, low, len)	(P2PHASE((x), 1ULL << (len)) << (low))
+#define	BF64_ENCODE(x, low, len)	(P2PHASE((x), ((grub_uint64_t) 1) << (len)) << (low))
 
 #define	BF32_GET(x, low, len)		BF32_DECODE(x, low, len)
 #define	BF64_GET(x, low, len)		BF64_DECODE(x, low, len)
-- 
2.27.0

Re: [PATCH v6 00/14] error: Do compile-time format string checking on grub>

[Daniel Kiper]

On Thu, Mar 04, 2021 at 06:22:31PM -0600, Glenn Washburn wrote:
> Daniel, you mentioned wanting a separate patch series which is the real fix
> for patch #12. I've added it to this patch series, since they go together.
> I can send the single patch as a separate thread if that still desirable.

For all Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

I will take all stuff up to #13. The #14 will be applied after 2.06 release.

Thank you for fixing these issues.

By the way, my I ask you once again to send each patch series as
separate thread. Now you are attaching all patch sets to one cover letter
which is confusing. Please stop doing that.

Daniel

> Changes since v5
>  * Fix formatting issues with spaces around format string macros and casts
>  * Add extra patch #14 which is the better fix for #12, but needs more testing
>
> Glenn
>
> Glenn Washburn (14):
>   misc: Format string for grub_error should be a literal
>   error: grub_error missing format string argument
>   error: grub_error format string add missing format code
>   dmraid_nvidia: Format string error in grub_error
>   grub_error: Use format code PRIuGRUB_SIZE for variables of type
>     grub_size_t
>   pgp: Format code for grub_error is incorrect
>   efi: Format string error in grub_error
>   error: Use PRI* macros to get correct format string code across
>     architectures
>   error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in
>     grub_error
>   error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error
>   error: Use format code PRIuGRUB_UINT64_T for 64-bit typed fileblock in
>     grub_error
>   error: Use format code llu for 64-bit uint bp->blk_prop in grub_error
>   error: Do compile-time format string checking on grub_error
>   zfs: Use grub_uint64_t instead of 1ULL in BF64_*CODE macros

Re: [PATCH v6 00/14] error: Do compile-time format string checking on grub>

[Glenn Washburn]

On Fri, 5 Mar 2021 17:27:01 +0100
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Thu, Mar 04, 2021 at 06:22:31PM -0600, Glenn Washburn wrote:
> > Daniel, you mentioned wanting a separate patch series which is the
> > real fix for patch #12. I've added it to this patch series, since
> > they go together. I can send the single patch as a separate thread
> > if that still desirable.
> 
> For all Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> I will take all stuff up to #13. The #14 will be applied after 2.06
> release.

Great!

> Thank you for fixing these issues.
>
> By the way, my I ask you once again to send each patch series as
> separate thread. Now you are attaching all patch sets to one cover
> letter which is confusing. Please stop doing that.

How do you pull patch series from the mailing list? I'm curious if this
is messing with that. Also what email client do you use?

You are correct that I'm attaching all new versions of the patch series
to one cover letter, but each patch series also has its own cover
letter. So I don't consider the cover letter that is the root of the
thread to be the cover letter for the new patch series.

I can't find our prior correspondence. I recall saying that the
patchset series in question had been not done in a less than ideal way
because I had start by replying to the cover letter of the prior
patchset and then switched to replying to a particular patchset cover
letter. This was because with experience I determined that attaching to
the thread of the next version of a patchset to the cover letter of the
first version of the patchset looked much nicer in my browser. I
also recall saying that I'd do this in the future to see if it worked
well doing it properly from the start.

My goal is to keep all versions of a patchset together in a client
with tree view of threads (eg. my mail client claws-mail). This makes
it easy to go back to a prior patchset to look at comments. I also
wanted to meet this goal in an aesthetically pleasing way. The first
attempt which attached a new version of a patchset to its priors cover
letter did not meet this because it created a deep tree for patchsets
with lots of revisions. However, attaching each new patchset to the
cover letter of the first patchset, keeps thread tree to a minimum.  It
also makes it to collapse only certain patchset versions (aside from
the first). Also, since I have threads sorted by thread date, I see
patchsets ordered from most recent to least recent, which it how I like
to order all my emails.

The current case does not strictly adhere to these rules because I'm
taking v4 as the initial patchset, which perhaps may be the source of
some confusion. Other than that I think its worked out nicely.

So I'm curious if this is causing some issue with tooling. And I'm
curious what exactly is causing confusion? In my browser its fairly
obvious that the root of the thread is the cover letter for v4 of the
patch series and that the cover letters of attached patch series are
different, noted by a different version number.

Glenn

> Daniel
> 
> > Changes since v5
> >  * Fix formatting issues with spaces around format string macros
> > and casts
> >  * Add extra patch #14 which is the better fix for #12, but needs
> > more testing
> >
> > Glenn
> >
> > Glenn Washburn (14):
> >   misc: Format string for grub_error should be a literal
> >   error: grub_error missing format string argument
> >   error: grub_error format string add missing format code
> >   dmraid_nvidia: Format string error in grub_error
> >   grub_error: Use format code PRIuGRUB_SIZE for variables of type
> >     grub_size_t
> >   pgp: Format code for grub_error is incorrect
> >   efi: Format string error in grub_error
> >   error: Use PRI* macros to get correct format string code across
> >     architectures
> >   error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument
> > in grub_error
> >   error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in
> > grub_error error: Use format code PRIuGRUB_UINT64_T for 64-bit
> > typed fileblock in grub_error
> >   error: Use format code llu for 64-bit uint bp->blk_prop in
> > grub_error error: Do compile-time format string checking on
> > grub_error zfs: Use grub_uint64_t instead of 1ULL in BF64_*CODE
> > macros

Threading of patch series (was: [PATCH v6 00/14] error: Do compile-time format string checking on grub>)

[Paul Menzel]

Dear Glenn,


Am 06.03.21 um 00:15 schrieb Glenn Washburn:
> On Fri, 5 Mar 2021 17:27:01 +0100 Daniel Kiper wrote:

[…]

>> By the way, my I ask you once again to send each patch series as
>> separate thread. Now you are attaching all patch sets to one cover
>> letter which is confusing. Please stop doing that.
> 
> How do you pull patch series from the mailing list? I'm curious if this
> is messing with that. Also what email client do you use?
> 
> You are correct that I'm attaching all new versions of the patch series
> to one cover letter, but each patch series also has its own cover
> letter. So I don't consider the cover letter that is the root of the
> thread to be the cover letter for the new patch series.
> 
> I can't find our prior correspondence. I recall saying that the
> patchset series in question had been not done in a less than ideal way
> because I had start by replying to the cover letter of the prior
> patchset and then switched to replying to a particular patchset cover
> letter. This was because with experience I determined that attaching to
> the thread of the next version of a patchset to the cover letter of the
> first version of the patchset looked much nicer in my browser. I
> also recall saying that I'd do this in the future to see if it worked
> well doing it properly from the start.
> 
> My goal is to keep all versions of a patchset together in a client
> with tree view of threads (eg. my mail client claws-mail). This makes
> it easy to go back to a prior patchset to look at comments. I also
> wanted to meet this goal in an aesthetically pleasing way. The first
> attempt which attached a new version of a patchset to its priors cover
> letter did not meet this because it created a deep tree for patchsets
> with lots of revisions. However, attaching each new patchset to the
> cover letter of the first patchset, keeps thread tree to a minimum.  It
> also makes it to collapse only certain patchset versions (aside from
> the first). Also, since I have threads sorted by thread date, I see
> patchsets ordered from most recent to least recent, which it how I like
> to order all my emails.
> 
> The current case does not strictly adhere to these rules because I'm
> taking v4 as the initial patchset, which perhaps may be the source of
> some confusion. Other than that I think its worked out nicely.
> 
> So I'm curious if this is causing some issue with tooling. And I'm
> curious what exactly is causing confusion? In my browser its fairly
> obvious that the root of the thread is the cover letter for v4 of the
> patch series and that the cover letters of attached patch series are
> different, noted by a different version number.

At least here, Mozilla Thunderbird 78.8.0 is only able to collapse the 
top thread and not sub threads.

The mailing list archive does not seem to care [1], though that might be 
because the v4 patch set cover letter is in a different month.

Anyway, as *displaying* of threading is not defined and different 
between user agents, maybe it’s better to not rely on that.


Kind regards,

Paul


[1]: https://lists.gnu.org/archive/html/grub-devel/2021-03/threads.html

Re: Threading of patch series (was: [PATCH v6 00/14] error: Do compile-time format string checking on grub>)

[Glenn Washburn]

Hi Paul,

On Sat, 6 Mar 2021 07:59:18 +0100
Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> Dear Glenn,
> 
> 
> Am 06.03.21 um 00:15 schrieb Glenn Washburn:
> > On Fri, 5 Mar 2021 17:27:01 +0100 Daniel Kiper wrote:
> 
> […]
> 
> >> By the way, my I ask you once again to send each patch series as
> >> separate thread. Now you are attaching all patch sets to one cover
> >> letter which is confusing. Please stop doing that.
> > 
> > How do you pull patch series from the mailing list? I'm curious if
> > this is messing with that. Also what email client do you use?
> > 
> > You are correct that I'm attaching all new versions of the patch
> > series to one cover letter, but each patch series also has its own
> > cover letter. So I don't consider the cover letter that is the root
> > of the thread to be the cover letter for the new patch series.
> > 
> > I can't find our prior correspondence. I recall saying that the
> > patchset series in question had been not done in a less than ideal
> > way because I had start by replying to the cover letter of the prior
> > patchset and then switched to replying to a particular patchset
> > cover letter. This was because with experience I determined that
> > attaching to the thread of the next version of a patchset to the
> > cover letter of the first version of the patchset looked much nicer
> > in my browser. I also recall saying that I'd do this in the future
> > to see if it worked well doing it properly from the start.
> > 
> > My goal is to keep all versions of a patchset together in a client
> > with tree view of threads (eg. my mail client claws-mail). This
> > makes it easy to go back to a prior patchset to look at comments. I
> > also wanted to meet this goal in an aesthetically pleasing way. The
> > first attempt which attached a new version of a patchset to its
> > priors cover letter did not meet this because it created a deep
> > tree for patchsets with lots of revisions. However, attaching each
> > new patchset to the cover letter of the first patchset, keeps
> > thread tree to a minimum.  It also makes it to collapse only
> > certain patchset versions (aside from the first). Also, since I
> > have threads sorted by thread date, I see patchsets ordered from
> > most recent to least recent, which it how I like to order all my
> > emails.
> > 
> > The current case does not strictly adhere to these rules because I'm
> > taking v4 as the initial patchset, which perhaps may be the source
> > of some confusion. Other than that I think its worked out nicely.
> > 
> > So I'm curious if this is causing some issue with tooling. And I'm
> > curious what exactly is causing confusion? In my browser its fairly
> > obvious that the root of the thread is the cover letter for v4 of
> > the patch series and that the cover letters of attached patch
> > series are different, noted by a different version number.
> 
> At least here, Mozilla Thunderbird 78.8.0 is only able to collapse
> the top thread and not sub threads.

Thanks for the feedback. I don't suppose that's a big loss of
functionality with respect the way I'm sending new patchset versions.
If the thread is super long, just collapse the whole thread. Perhaps
I'm failing to see the issue.

What could be annoying is if you want to go back several patch
revisions and compare with the current one. But that would still be
easier this way than the current practice because the previous version
is likely to be between a lot of other threads.

Now it definitely would be more problematic for clients that can not
collapse threads (perhaps text-based ones?). Does anyone have this
issue?

> The mailing list archive does not seem to care [1], though that might
> be because the v4 patch set cover letter is in a different month.

Yes, I believe that the mailing list only threads on a per month basis.
I've actually been annoyed by this in the past with other list using
the same setup when trying to read multi-month threads.

> Anyway, as *displaying* of threading is not defined and different 
> between user agents, maybe it’s better to not rely on that.

I'm confused by this. I accept the premise, but I don't understand
"rely" in this context, which I interpret as "to assume". I _am_
relying on the behavior of my email client for my personal usage. I
understand your comment to be analogous to saying that I am relying on
the display capabilities of others email clients when I send a patch
with --thread in git send-email because there may be someone on the
list who chooses to use an email client that does not support a
threaded view.

I'm less concerned with the capabilities of other clients than I am for
how this negatively impacts the current workflow of people on this
list, which is what I'm trying to figure out. How does doing this
making things more difficult for people here? And specifically Daniel.

If this causes tools to break, then that's certainly a good reason to
avoid changing things. If it makes Daniel's workflow more burdensome
because he has a client that can not collapse threads at all, then
that's a valid concern too. I'm currently failing to see as valid
concerns: "because traditionally we've never done it that way" or "I
don't like seeing patchset revisions together".

From a structuring data point of view, it makes more sense (to me) to
link patchset revisions together. Now it might also be true that
looking at previous patchsets is very uncommon and so its not a huge
benefit to link them together. But that's not a reason why it shouldn't
be done.

Perhaps with a better understanding of the harm, I could come to a
different opinion. And sincerely, thank you for the feedback.

Glenn

> Kind regards,
> 
> Paul
> 
> 
> [1]:
> https://lists.gnu.org/archive/html/grub-devel/2021-03/threads.html

Re: Threading of patch series (was: [PATCH v6 00/14] error: Do compile-time format string checking on grub>)

[Konrad Rzeszutek Wilk]

..snip..
> I'm less concerned with the capabilities of other clients than I am for
> how this negatively impacts the current workflow of people on this
> list, which is what I'm trying to figure out. How does doing this
> making things more difficult for people here? And specifically Daniel.

Don't know what flow Daniel is using, but my email reader is mutt.

Once I am comfortable with the patches I usually save them in a mailbox
(.mbox) and do 'git am -s' on them and then kickoff a workflow.

Having multiple threads within threads makes my life as maintainer
more difficult as I have to manually split out the right version and
save it.

I suppose I can use the 'tag-save' option and save the ones
with a certain version to a mailbox which can add another step in this.
And then I am worried I missed something :-(

> 
> If this causes tools to break, then that's certainly a good reason to
> avoid changing things. If it makes Daniel's workflow more burdensome
> because he has a client that can not collapse threads at all, then
> that's a valid concern too. I'm currently failing to see as valid
> concerns: "because traditionally we've never done it that way" or "I
> don't like seeing patchset revisions together".
> 
> From a structuring data point of view, it makes more sense (to me) to
> link patchset revisions together. Now it might also be true that
> looking at previous patchsets is very uncommon and so its not a huge

Maintainers trust that folks add right after the ---

v6: Fixed up spaces
    Fixed up XYZ

and there is no need to go back to v5 or earlier.

> benefit to link them together. But that's not a reason why it shouldn't
> be done.

So ... making the life of maintainer and as simple as possible makes
it easier and faster to get patches upstream.

That is what LKML has taught a lot of us and that is probably why
many of us are used to this workflow.

> 
> Perhaps with a better understanding of the harm, I could come to a
> different opinion. And sincerely, thank you for the feedback.
> 
> Glenn
> 
> > Kind regards,
> > 
> > Paul
> > 
> > 
> > [1]:
> > https://lists.gnu.org/archive/html/grub-devel/2021-03/threads.html
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel