All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Warn if MBR gap is small and user uses advanced modules
@ 2020-03-10 12:23 Vladimir 'phcoder' Serbinenko
  2020-03-10 12:30 ` Mihai Moldovan
  2020-03-11 12:43 ` Daniel Kiper
  0 siblings, 2 replies; 8+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2020-03-10 12:23 UTC (permalink / raw)
  To: The development of GRUB 2

Daniel, do you want to adjust the whitelist?

We don't want to support small MBR gap in pair with anything but
the simplest config of biosdisk+part_msdos+simple filesystem. In this
path "simple filesystems" are all current filesystems except zfs and
btrfs
---
 grub-core/partmap/gpt.c     |  9 ++++++++-
 grub-core/partmap/msdos.c   |  7 ++++++-
 include/grub/partition.h    |  3 ++-
 include/grub/util/install.h |  7 +++++--
 util/grub-install-common.c  | 24 ++++++++++++++++++++++++
 util/grub-install.c         | 10 +++++++---
 util/grub-setup.c           |  2 +-
 util/setup.c                |  5 +++--
 8 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
index 103f6796f..25a5a1934 100644
--- a/grub-core/partmap/gpt.c
+++ b/grub-core/partmap/gpt.c
@@ -25,6 +25,9 @@
 #include <grub/msdos_partition.h>
 #include <grub/gpt_partition.h>
 #include <grub/i18n.h>
+#ifdef GRUB_UTIL
+#include <grub/emu/misc.h>
+#endif

 GRUB_MOD_LICENSE ("GPLv3+");

@@ -169,7 +172,8 @@ static grub_err_t
 gpt_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
  unsigned int max_nsectors,
  grub_embed_type_t embed_type,
- grub_disk_addr_t **sectors)
+ grub_disk_addr_t **sectors,
+ int warn_short)
 {
   struct gpt_partition_map_embed_ctx ctx = {
     .start = 0,
@@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
unsigned int *nsectors,
         N_("this GPT partition label contains no BIOS Boot Partition;"
    " embedding won't be possible"));

+  if (ctx.len < 450) {
+    grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
increase its size.");
+  }
   if (ctx.len < *nsectors)
     return grub_error (GRUB_ERR_OUT_OF_RANGE,
         N_("your BIOS Boot Partition is too small;"
diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
index 7b8e45076..402bce050 100644
--- a/grub-core/partmap/msdos.c
+++ b/grub-core/partmap/msdos.c
@@ -236,7 +236,8 @@ static grub_err_t
 pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
  unsigned int max_nsectors,
  grub_embed_type_t embed_type,
- grub_disk_addr_t **sectors)
+ grub_disk_addr_t **sectors,
+ int warn_short)
 {
   grub_disk_addr_t end = ~0ULL;
   struct grub_msdos_partition_mbr mbr;
@@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
unsigned int *nsectors,
       return GRUB_ERR_NONE;
     }

+  if (end < 450 && warn_short) {
+    grub_util_warn("You have a short MBR gap and use advanced config.
Please increase post-MBR gap");
+  }
+
   if (end <= 1)
     return grub_error (GRUB_ERR_FILE_NOT_FOUND,
         N_("this msdos-style partition label has no "
diff --git a/include/grub/partition.h b/include/grub/partition.h
index 7adb7ec6e..5697e28d5 100644
--- a/include/grub/partition.h
+++ b/include/grub/partition.h
@@ -55,7 +55,8 @@ struct grub_partition_map
   grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
         unsigned int max_nsectors,
         grub_embed_type_t embed_type,
-        grub_disk_addr_t **sectors);
+        grub_disk_addr_t **sectors,
+        int warn_short);
 #endif
 };
 typedef struct grub_partition_map *grub_partition_map_t;
diff --git a/include/grub/util/install.h b/include/grub/util/install.h
index 2631b1074..982115f57 100644
--- a/include/grub/util/install.h
+++ b/include/grub/util/install.h
@@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
        const char *boot_file, const char *core_file,
        const char *dest, int force,
        int fs_probe, int allow_floppy,
-       int add_rs_codes);
+       int add_rs_codes, int warn_short_mbr_gap);
 void
 grub_util_sparc_setup (const char *dir,
         const char *boot_file, const char *core_file,
         const char *dest, int force,
         int fs_probe, int allow_floppy,
-        int add_rs_codes);
+        int add_rs_codes, int warn_short_mbr_gap);

 char *
 grub_install_get_image_targets_string (void);
@@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
grub_install_image_target_desc *t);
 extern char *grub_install_copy_buffer;
 #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576

+int
+grub_install_is_short_mgrgap_supported(void);
+
 #endif
diff --git a/util/grub-install-common.c b/util/grub-install-common.c
index ca0ac612a..e4cff2871 100644
--- a/util/grub-install-common.c
+++ b/util/grub-install-common.c
@@ -234,6 +234,30 @@ char *grub_install_source_directory = NULL;
 char *grub_install_locale_directory = NULL;
 char *grub_install_themes_directory = NULL;

+int
+grub_install_is_short_mgrgap_supported()
+{
+  int i, j;
+  static const char *whitelist[] =
+    {
+     "part_msdos", "biosdisk", "affs", "afs", "bfs", "archelp",
+     "cpio", "cpio_be", "newc", "odc", "ext2", "fat", "exfat",
+     "f2fs", "fshelp", "hfs", "hfsplus", "hfspluscomp",
+     "iso9660", "jfs", "minix", "minix2", "minix3", "minix_be",
+     "minix2_be", "minix2_be", "nilfs2", "ntfs", "ntfscomp",
+     "reiserfs", "romfs", "sfs", "squash4", "tar", "udf",
+     "ufs1", "ufs1_be", "ufs2", "xfs"
+    };
+  for (i = 0; i < modules.n_entries; i++) {
+    for (j = 0; j < ARRAY_SIZE (whitelist); j++)
+      if (strcmp(modules.entries[i], whitelist[j]) == 0)
+ break;
+    if (j == ARRAY_SIZE (whitelist))
+      return 0;
+  }
+  return 1;
+}
+
 void
 grub_install_push_module (const char *val)
 {
diff --git a/util/grub-install.c b/util/grub-install.c
index 8970b73aa..ba27657d9 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -1718,10 +1718,14 @@ main (int argc, char *argv[])
  install_device);

  /*  Now perform the installation.  */
- if (install_bootsector)
+ if (install_bootsector) {
+   int warn_short_mbr_gap = grub_install_is_short_mgrgap_supported();
+
    grub_util_bios_setup (platdir, "boot.img", "core.img",
  install_drive, force,
- fs_probe, allow_floppy, add_rs_codes);
+ fs_probe, allow_floppy, add_rs_codes,
+ warn_short_mbr_gap);
+ }
  break;
       }
     case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
@@ -1748,7 +1752,7 @@ main (int argc, char *argv[])
    grub_util_sparc_setup (platdir, "boot.img", "core.img",
  install_drive, force,
  fs_probe, allow_floppy,
- 0 /* unused */ );
+ 0 /* unused */, 0 /* unused */ );
  break;
       }

diff --git a/util/grub-setup.c b/util/grub-setup.c
index 42b98ad3c..1783224dd 100644
--- a/util/grub-setup.c
+++ b/util/grub-setup.c
@@ -315,7 +315,7 @@ main (int argc, char *argv[])
     arguments.core_file ? : DEFAULT_CORE_FILE,
     dest_dev, arguments.force,
     arguments.fs_probe, arguments.allow_floppy,
-    arguments.add_rs_codes);
+    arguments.add_rs_codes, 0);

   /* Free resources.  */
   grub_fini_all ();
diff --git a/util/setup.c b/util/setup.c
index 3be88aae1..da5f2c07f 100644
--- a/util/setup.c
+++ b/util/setup.c
@@ -254,7 +254,8 @@ SETUP (const char *dir,
        const char *boot_file, const char *core_file,
        const char *dest, int force,
        int fs_probe, int allow_floppy,
-       int add_rs_codes __attribute__ ((unused))) /* unused on sparc64 */
+       int add_rs_codes __attribute__ ((unused)), /* unused on sparc64 */
+       int warn_small)
 {
   char *core_path;
   char *boot_img, *core_img, *boot_path;
@@ -530,7 +531,7 @@ SETUP (const char *dir,
  GRUB_EMBED_PCBIOS, &sectors);
     else if (ctx.dest_partmap)
       err = ctx.dest_partmap->embed (dest_dev->disk, &nsec, maxsec,
-      GRUB_EMBED_PCBIOS, &sectors);
+      GRUB_EMBED_PCBIOS, &sectors, warn_small);
     else
       err = fs->fs_embed (dest_dev, &nsec, maxsec,
    GRUB_EMBED_PCBIOS, &sectors);
-- 
2.20.1


-- 
Regards
Vladimir 'phcoder' Serbinenko


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] Warn if MBR gap is small and user uses advanced modules
  2020-03-10 12:23 [PATCH] Warn if MBR gap is small and user uses advanced modules Vladimir 'phcoder' Serbinenko
@ 2020-03-10 12:30 ` Mihai Moldovan
  2020-03-11 12:43 ` Daniel Kiper
  1 sibling, 0 replies; 8+ messages in thread
From: Mihai Moldovan @ 2020-03-10 12:30 UTC (permalink / raw)
  To: The development of GNU GRUB, Vladimir 'phcoder' Serbinenko


[-- Attachment #1.1: Type: text/plain, Size: 1268 bytes --]

* On 3/10/20 1:23 PM, Vladimir 'phcoder' Serbinenko wrote:
> [...]
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index 2631b1074..982115f57 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
>         const char *boot_file, const char *core_file,
>         const char *dest, int force,
>         int fs_probe, int allow_floppy,
> -       int add_rs_codes);
> +       int add_rs_codes, int warn_short_mbr_gap);
>  void
>  grub_util_sparc_setup (const char *dir,
>          const char *boot_file, const char *core_file,
>          const char *dest, int force,
>          int fs_probe, int allow_floppy,
> -        int add_rs_codes);
> +        int add_rs_codes, int warn_short_mbr_gap);
> 
>  char *
>  grub_install_get_image_targets_string (void);
> @@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
> grub_install_image_target_desc *t);
>  extern char *grub_install_copy_buffer;
>  #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576
> 
> +int
> +grub_install_is_short_mgrgap_supported(void);

Did you mean to use "grub_install_is_short_mbrgap_supported" instead here and in
all the other places?



Mihai


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Warn if MBR gap is small and user uses advanced modules
  2020-03-10 12:23 [PATCH] Warn if MBR gap is small and user uses advanced modules Vladimir 'phcoder' Serbinenko
  2020-03-10 12:30 ` Mihai Moldovan
@ 2020-03-11 12:43 ` Daniel Kiper
  2020-03-11 14:56   ` Javier Martinez Canillas
                     ` (3 more replies)
  1 sibling, 4 replies; 8+ messages in thread
From: Daniel Kiper @ 2020-03-11 12:43 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: The development of GRUB 2, mchang, ionic, javierm, pjones

Adding Michael, Mihai, Javier and Peter...

Below you can find what more or less Vladimir and I agreed WRT small MBR
gap. In general Vladimir convinced me to phase out small MBR gaps
support gradually. This is first step in this journey. We think that we
have to build some warnings into the code and extend documentation.
Please chime in what you think about that...

On Tue, Mar 10, 2020 at 01:23:10PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Daniel, do you want to adjust the whitelist?
>
> We don't want to support small MBR gap in pair with anything but
> the simplest config of biosdisk+part_msdos+simple filesystem. In this
> path "simple filesystems" are all current filesystems except zfs and
> btrfs

Missing SOB...

> ---
>  grub-core/partmap/gpt.c     |  9 ++++++++-
>  grub-core/partmap/msdos.c   |  7 ++++++-
>  include/grub/partition.h    |  3 ++-
>  include/grub/util/install.h |  7 +++++--
>  util/grub-install-common.c  | 24 ++++++++++++++++++++++++
>  util/grub-install.c         | 10 +++++++---
>  util/grub-setup.c           |  2 +-
>  util/setup.c                |  5 +++--
>  8 files changed, 56 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> index 103f6796f..25a5a1934 100644
> --- a/grub-core/partmap/gpt.c
> +++ b/grub-core/partmap/gpt.c
> @@ -25,6 +25,9 @@
>  #include <grub/msdos_partition.h>
>  #include <grub/gpt_partition.h>
>  #include <grub/i18n.h>
> +#ifdef GRUB_UTIL
> +#include <grub/emu/misc.h>
> +#endif
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -169,7 +172,8 @@ static grub_err_t
>  gpt_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
>   unsigned int max_nsectors,
>   grub_embed_type_t embed_type,
> - grub_disk_addr_t **sectors)
> + grub_disk_addr_t **sectors,
> + int warn_short)
>  {
>    struct gpt_partition_map_embed_ctx ctx = {
>      .start = 0,
> @@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
> unsigned int *nsectors,
>          N_("this GPT partition label contains no BIOS Boot Partition;"
>     " embedding won't be possible"));
>
> +  if (ctx.len < 450) {

Could you define constant somewhere?

Is it in sectors? Why 450? Should not it be 2048 if 1 MiB below?

...and missing "&& warn_short" check...

> +    grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
> increase its size.");
> +  }
>    if (ctx.len < *nsectors)

Could not we use this check somehow instead of adding new one?

>      return grub_error (GRUB_ERR_OUT_OF_RANGE,
>          N_("your BIOS Boot Partition is too small;"
> diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
> index 7b8e45076..402bce050 100644
> --- a/grub-core/partmap/msdos.c
> +++ b/grub-core/partmap/msdos.c
> @@ -236,7 +236,8 @@ static grub_err_t
>  pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
>   unsigned int max_nsectors,
>   grub_embed_type_t embed_type,
> - grub_disk_addr_t **sectors)
> + grub_disk_addr_t **sectors,
> + int warn_short)
>  {
>    grub_disk_addr_t end = ~0ULL;
>    struct grub_msdos_partition_mbr mbr;
> @@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
> unsigned int *nsectors,
>        return GRUB_ERR_NONE;
>      }
>
> +  if (end < 450 && warn_short) {
> +    grub_util_warn("You have a short MBR gap and use advanced config.
> Please increase post-MBR gap");

Ditto?

> +  }
> +
>    if (end <= 1)
>      return grub_error (GRUB_ERR_FILE_NOT_FOUND,
>          N_("this msdos-style partition label has no "
> diff --git a/include/grub/partition.h b/include/grub/partition.h
> index 7adb7ec6e..5697e28d5 100644
> --- a/include/grub/partition.h
> +++ b/include/grub/partition.h
> @@ -55,7 +55,8 @@ struct grub_partition_map
>    grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
>          unsigned int max_nsectors,
>          grub_embed_type_t embed_type,
> -        grub_disk_addr_t **sectors);
> +        grub_disk_addr_t **sectors,
> +        int warn_short);
>  #endif
>  };
>  typedef struct grub_partition_map *grub_partition_map_t;
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index 2631b1074..982115f57 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
>         const char *boot_file, const char *core_file,
>         const char *dest, int force,
>         int fs_probe, int allow_floppy,
> -       int add_rs_codes);
> +       int add_rs_codes, int warn_short_mbr_gap);
>  void
>  grub_util_sparc_setup (const char *dir,
>          const char *boot_file, const char *core_file,
>          const char *dest, int force,
>          int fs_probe, int allow_floppy,
> -        int add_rs_codes);
> +        int add_rs_codes, int warn_short_mbr_gap);
>
>  char *
>  grub_install_get_image_targets_string (void);
> @@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
> grub_install_image_target_desc *t);
>  extern char *grub_install_copy_buffer;
>  #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576
>
> +int

extern int

> +grub_install_is_short_mgrgap_supported(void);
> +
>  #endif
> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> index ca0ac612a..e4cff2871 100644
> --- a/util/grub-install-common.c
> +++ b/util/grub-install-common.c
> @@ -234,6 +234,30 @@ char *grub_install_source_directory = NULL;
>  char *grub_install_locale_directory = NULL;
>  char *grub_install_themes_directory = NULL;
>
> +int
> +grub_install_is_short_mgrgap_supported()
> +{
> +  int i, j;
> +  static const char *whitelist[] =
> +    {
> +     "part_msdos", "biosdisk", "affs", "afs", "bfs", "archelp",
> +     "cpio", "cpio_be", "newc", "odc", "ext2", "fat", "exfat",
> +     "f2fs", "fshelp", "hfs", "hfsplus", "hfspluscomp",
> +     "iso9660", "jfs", "minix", "minix2", "minix3", "minix_be",
> +     "minix2_be", "minix2_be", "nilfs2", "ntfs", "ntfscomp",
> +     "reiserfs", "romfs", "sfs", "squash4", "tar", "udf",
> +     "ufs1", "ufs1_be", "ufs2", "xfs"
> +    };

The list LGTM...

> +  for (i = 0; i < modules.n_entries; i++) {
> +    for (j = 0; j < ARRAY_SIZE (whitelist); j++)
> +      if (strcmp(modules.entries[i], whitelist[j]) == 0)
> + break;
> +    if (j == ARRAY_SIZE (whitelist))
> +      return 0;
> +  }
> +  return 1;
> +}
> +
>  void
>  grub_install_push_module (const char *val)
>  {
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 8970b73aa..ba27657d9 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -1718,10 +1718,14 @@ main (int argc, char *argv[])
>   install_device);
>
>   /*  Now perform the installation.  */
> - if (install_bootsector)
> + if (install_bootsector) {
> +   int warn_short_mbr_gap = grub_install_is_short_mgrgap_supported();
> +
>     grub_util_bios_setup (platdir, "boot.img", "core.img",
>   install_drive, force,
> - fs_probe, allow_floppy, add_rs_codes);
> + fs_probe, allow_floppy, add_rs_codes,
> + warn_short_mbr_gap);
> + }
>   break;
>        }
>      case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
> @@ -1748,7 +1752,7 @@ main (int argc, char *argv[])
>     grub_util_sparc_setup (platdir, "boot.img", "core.img",
>   install_drive, force,
>   fs_probe, allow_floppy,
> - 0 /* unused */ );
> + 0 /* unused */, 0 /* unused */ );
>   break;
>        }
>
> diff --git a/util/grub-setup.c b/util/grub-setup.c
> index 42b98ad3c..1783224dd 100644
> --- a/util/grub-setup.c
> +++ b/util/grub-setup.c
> @@ -315,7 +315,7 @@ main (int argc, char *argv[])
>      arguments.core_file ? : DEFAULT_CORE_FILE,
>      dest_dev, arguments.force,
>      arguments.fs_probe, arguments.allow_floppy,
> -    arguments.add_rs_codes);
> +    arguments.add_rs_codes, 0);
>
>    /* Free resources.  */
>    grub_fini_all ();
> diff --git a/util/setup.c b/util/setup.c
> index 3be88aae1..da5f2c07f 100644
> --- a/util/setup.c
> +++ b/util/setup.c
> @@ -254,7 +254,8 @@ SETUP (const char *dir,
>         const char *boot_file, const char *core_file,
>         const char *dest, int force,
>         int fs_probe, int allow_floppy,
> -       int add_rs_codes __attribute__ ((unused))) /* unused on sparc64 */
> +       int add_rs_codes __attribute__ ((unused)), /* unused on sparc64 */
> +       int warn_small)
>  {
>    char *core_path;
>    char *boot_img, *core_img, *boot_path;
> @@ -530,7 +531,7 @@ SETUP (const char *dir,
>   GRUB_EMBED_PCBIOS, &sectors);
>      else if (ctx.dest_partmap)
>        err = ctx.dest_partmap->embed (dest_dev->disk, &nsec, maxsec,
> -      GRUB_EMBED_PCBIOS, &sectors);
> +      GRUB_EMBED_PCBIOS, &sectors, warn_small);
>      else
>        err = fs->fs_embed (dest_dev, &nsec, maxsec,
>     GRUB_EMBED_PCBIOS, &sectors);

...plus some missing spaces in function names and other places...

Additionally, please extend relevant doc section with explanation why we
are doing that. And maybe with an schedule for phase out...

Daniel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Warn if MBR gap is small and user uses advanced modules
  2020-03-11 12:43 ` Daniel Kiper
@ 2020-03-11 14:56   ` Javier Martinez Canillas
  2020-03-11 15:28   ` Michael Chang
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2020-03-11 14:56 UTC (permalink / raw)
  To: Daniel Kiper, Vladimir 'phcoder' Serbinenko
  Cc: The development of GRUB 2, mchang, ionic, pjones

Hello Daniel,

On 3/11/20 1:43 PM, Daniel Kiper wrote:
> Adding Michael, Mihai, Javier and Peter...
> 
> Below you can find what more or less Vladimir and I agreed WRT small MBR
> gap. In general Vladimir convinced me to phase out small MBR gaps
> support gradually. This is first step in this journey. We think that we
> have to build some warnings into the code and extend documentation.
> Please chime in what you think about that...
>

I agree that is a good approach. And I'm happy to keep carrying the patch
that adds file/line to grub_error() in Fedora until we don't have the gap
size restriction anymore.

For Vladimir's patch, I see that you pointed out things so I'll wait for
v2 to do a review.

Best regards, -- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Warn if MBR gap is small and user uses advanced modules
  2020-03-11 12:43 ` Daniel Kiper
  2020-03-11 14:56   ` Javier Martinez Canillas
@ 2020-03-11 15:28   ` Michael Chang
  2020-03-11 17:07   ` Didier Spaier
  2020-04-27 15:41   ` Vladimir 'phcoder' Serbinenko
  3 siblings, 0 replies; 8+ messages in thread
From: Michael Chang @ 2020-03-11 15:28 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: Vladimir 'phcoder' Serbinenko, The development of GRUB 2,
	ionic, javierm, pjones

On Wed, Mar 11, 2020 at 01:43:14PM +0100, Daniel Kiper wrote:
> Adding Michael, Mihai, Javier and Peter...
> 
> Below you can find what more or less Vladimir and I agreed WRT small MBR
> gap. In general Vladimir convinced me to phase out small MBR gaps
> support gradually. This is first step in this journey. We think that we
> have to build some warnings into the code and extend documentation.
> Please chime in what you think about that...

Usually I am also not in favor of making radical changes, so I could
share the opinions to take gradual change here. Even though I just
composed the patch to stop small mbr gap support, the diversity of
viewing the problem was still expected. In the end if that helps to
foster a better approach to improve the overall situation that's still a
worthwhile effort ...

> 
> On Tue, Mar 10, 2020 at 01:23:10PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > Daniel, do you want to adjust the whitelist?
> >
> > We don't want to support small MBR gap in pair with anything but
> > the simplest config of biosdisk+part_msdos+simple filesystem. In this
> > path "simple filesystems" are all current filesystems except zfs and
> > btrfs

What about lvm, md and cryto disks here ? They could have simple file
system formatted on top, but the size of the core image can go much
bigger.

In addition, a parallel problem I want to address here is that grub
modules get copied to the platform directory way too early regardless of
the image setup result being successful or not. We'd better fix that
problem as well or the "warning" would actually be "critical" in case of
running into 'undefined symbol error' that renders system unbootable. 

Thanks,
Michael

> 
> Missing SOB...
> 
> > ---
> >  grub-core/partmap/gpt.c     |  9 ++++++++-
> >  grub-core/partmap/msdos.c   |  7 ++++++-
> >  include/grub/partition.h    |  3 ++-
> >  include/grub/util/install.h |  7 +++++--
> >  util/grub-install-common.c  | 24 ++++++++++++++++++++++++
> >  util/grub-install.c         | 10 +++++++---
> >  util/grub-setup.c           |  2 +-
> >  util/setup.c                |  5 +++--
> >  8 files changed, 56 insertions(+), 11 deletions(-)
> >
> > diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> > index 103f6796f..25a5a1934 100644
> > --- a/grub-core/partmap/gpt.c
> > +++ b/grub-core/partmap/gpt.c
> > @@ -25,6 +25,9 @@
> >  #include <grub/msdos_partition.h>
> >  #include <grub/gpt_partition.h>
> >  #include <grub/i18n.h>
> > +#ifdef GRUB_UTIL
> > +#include <grub/emu/misc.h>
> > +#endif
> >
> >  GRUB_MOD_LICENSE ("GPLv3+");
> >
> > @@ -169,7 +172,8 @@ static grub_err_t
> >  gpt_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> >   unsigned int max_nsectors,
> >   grub_embed_type_t embed_type,
> > - grub_disk_addr_t **sectors)
> > + grub_disk_addr_t **sectors,
> > + int warn_short)
> >  {
> >    struct gpt_partition_map_embed_ctx ctx = {
> >      .start = 0,
> > @@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
> > unsigned int *nsectors,
> >          N_("this GPT partition label contains no BIOS Boot Partition;"
> >     " embedding won't be possible"));
> >
> > +  if (ctx.len < 450) {
> 
> Could you define constant somewhere?
> 
> Is it in sectors? Why 450? Should not it be 2048 if 1 MiB below?
> 
> ...and missing "&& warn_short" check...
> 
> > +    grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
> > increase its size.");
> > +  }
> >    if (ctx.len < *nsectors)
> 
> Could not we use this check somehow instead of adding new one?
> 
> >      return grub_error (GRUB_ERR_OUT_OF_RANGE,
> >          N_("your BIOS Boot Partition is too small;"
> > diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
> > index 7b8e45076..402bce050 100644
> > --- a/grub-core/partmap/msdos.c
> > +++ b/grub-core/partmap/msdos.c
> > @@ -236,7 +236,8 @@ static grub_err_t
> >  pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> >   unsigned int max_nsectors,
> >   grub_embed_type_t embed_type,
> > - grub_disk_addr_t **sectors)
> > + grub_disk_addr_t **sectors,
> > + int warn_short)
> >  {
> >    grub_disk_addr_t end = ~0ULL;
> >    struct grub_msdos_partition_mbr mbr;
> > @@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
> > unsigned int *nsectors,
> >        return GRUB_ERR_NONE;
> >      }
> >
> > +  if (end < 450 && warn_short) {
> > +    grub_util_warn("You have a short MBR gap and use advanced config.
> > Please increase post-MBR gap");
> 
> Ditto?
> 
> > +  }
> > +
> >    if (end <= 1)
> >      return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> >          N_("this msdos-style partition label has no "
> > diff --git a/include/grub/partition.h b/include/grub/partition.h
> > index 7adb7ec6e..5697e28d5 100644
> > --- a/include/grub/partition.h
> > +++ b/include/grub/partition.h
> > @@ -55,7 +55,8 @@ struct grub_partition_map
> >    grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
> >          unsigned int max_nsectors,
> >          grub_embed_type_t embed_type,
> > -        grub_disk_addr_t **sectors);
> > +        grub_disk_addr_t **sectors,
> > +        int warn_short);
> >  #endif
> >  };
> >  typedef struct grub_partition_map *grub_partition_map_t;
> > diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> > index 2631b1074..982115f57 100644
> > --- a/include/grub/util/install.h
> > +++ b/include/grub/util/install.h
> > @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
> >         const char *boot_file, const char *core_file,
> >         const char *dest, int force,
> >         int fs_probe, int allow_floppy,
> > -       int add_rs_codes);
> > +       int add_rs_codes, int warn_short_mbr_gap);
> >  void
> >  grub_util_sparc_setup (const char *dir,
> >          const char *boot_file, const char *core_file,
> >          const char *dest, int force,
> >          int fs_probe, int allow_floppy,
> > -        int add_rs_codes);
> > +        int add_rs_codes, int warn_short_mbr_gap);
> >
> >  char *
> >  grub_install_get_image_targets_string (void);
> > @@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
> > grub_install_image_target_desc *t);
> >  extern char *grub_install_copy_buffer;
> >  #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576
> >
> > +int
> 
> extern int
> 
> > +grub_install_is_short_mgrgap_supported(void);
> > +
> >  #endif
> > diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> > index ca0ac612a..e4cff2871 100644
> > --- a/util/grub-install-common.c
> > +++ b/util/grub-install-common.c
> > @@ -234,6 +234,30 @@ char *grub_install_source_directory = NULL;
> >  char *grub_install_locale_directory = NULL;
> >  char *grub_install_themes_directory = NULL;
> >
> > +int
> > +grub_install_is_short_mgrgap_supported()
> > +{
> > +  int i, j;
> > +  static const char *whitelist[] =
> > +    {
> > +     "part_msdos", "biosdisk", "affs", "afs", "bfs", "archelp",
> > +     "cpio", "cpio_be", "newc", "odc", "ext2", "fat", "exfat",
> > +     "f2fs", "fshelp", "hfs", "hfsplus", "hfspluscomp",
> > +     "iso9660", "jfs", "minix", "minix2", "minix3", "minix_be",
> > +     "minix2_be", "minix2_be", "nilfs2", "ntfs", "ntfscomp",
> > +     "reiserfs", "romfs", "sfs", "squash4", "tar", "udf",
> > +     "ufs1", "ufs1_be", "ufs2", "xfs"
> > +    };
> 
> The list LGTM...
> 
> > +  for (i = 0; i < modules.n_entries; i++) {
> > +    for (j = 0; j < ARRAY_SIZE (whitelist); j++)
> > +      if (strcmp(modules.entries[i], whitelist[j]) == 0)
> > + break;
> > +    if (j == ARRAY_SIZE (whitelist))
> > +      return 0;
> > +  }
> > +  return 1;
> > +}
> > +
> >  void
> >  grub_install_push_module (const char *val)
> >  {
> > diff --git a/util/grub-install.c b/util/grub-install.c
> > index 8970b73aa..ba27657d9 100644
> > --- a/util/grub-install.c
> > +++ b/util/grub-install.c
> > @@ -1718,10 +1718,14 @@ main (int argc, char *argv[])
> >   install_device);
> >
> >   /*  Now perform the installation.  */
> > - if (install_bootsector)
> > + if (install_bootsector) {
> > +   int warn_short_mbr_gap = grub_install_is_short_mgrgap_supported();
> > +
> >     grub_util_bios_setup (platdir, "boot.img", "core.img",
> >   install_drive, force,
> > - fs_probe, allow_floppy, add_rs_codes);
> > + fs_probe, allow_floppy, add_rs_codes,
> > + warn_short_mbr_gap);
> > + }
> >   break;
> >        }
> >      case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
> > @@ -1748,7 +1752,7 @@ main (int argc, char *argv[])
> >     grub_util_sparc_setup (platdir, "boot.img", "core.img",
> >   install_drive, force,
> >   fs_probe, allow_floppy,
> > - 0 /* unused */ );
> > + 0 /* unused */, 0 /* unused */ );
> >   break;
> >        }
> >
> > diff --git a/util/grub-setup.c b/util/grub-setup.c
> > index 42b98ad3c..1783224dd 100644
> > --- a/util/grub-setup.c
> > +++ b/util/grub-setup.c
> > @@ -315,7 +315,7 @@ main (int argc, char *argv[])
> >      arguments.core_file ? : DEFAULT_CORE_FILE,
> >      dest_dev, arguments.force,
> >      arguments.fs_probe, arguments.allow_floppy,
> > -    arguments.add_rs_codes);
> > +    arguments.add_rs_codes, 0);
> >
> >    /* Free resources.  */
> >    grub_fini_all ();
> > diff --git a/util/setup.c b/util/setup.c
> > index 3be88aae1..da5f2c07f 100644
> > --- a/util/setup.c
> > +++ b/util/setup.c
> > @@ -254,7 +254,8 @@ SETUP (const char *dir,
> >         const char *boot_file, const char *core_file,
> >         const char *dest, int force,
> >         int fs_probe, int allow_floppy,
> > -       int add_rs_codes __attribute__ ((unused))) /* unused on sparc64 */
> > +       int add_rs_codes __attribute__ ((unused)), /* unused on sparc64 */
> > +       int warn_small)
> >  {
> >    char *core_path;
> >    char *boot_img, *core_img, *boot_path;
> > @@ -530,7 +531,7 @@ SETUP (const char *dir,
> >   GRUB_EMBED_PCBIOS, &sectors);
> >      else if (ctx.dest_partmap)
> >        err = ctx.dest_partmap->embed (dest_dev->disk, &nsec, maxsec,
> > -      GRUB_EMBED_PCBIOS, &sectors);
> > +      GRUB_EMBED_PCBIOS, &sectors, warn_small);
> >      else
> >        err = fs->fs_embed (dest_dev, &nsec, maxsec,
> >     GRUB_EMBED_PCBIOS, &sectors);
> 
> ...plus some missing spaces in function names and other places...
> 
> Additionally, please extend relevant doc section with explanation why we
> are doing that. And maybe with an schedule for phase out...
> 
> Daniel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Warn if MBR gap is small and user uses advanced modules
  2020-03-11 12:43 ` Daniel Kiper
  2020-03-11 14:56   ` Javier Martinez Canillas
  2020-03-11 15:28   ` Michael Chang
@ 2020-03-11 17:07   ` Didier Spaier
  2020-03-11 17:42     ` Vladimir 'phcoder' Serbinenko
  2020-04-27 15:41   ` Vladimir 'phcoder' Serbinenko
  3 siblings, 1 reply; 8+ messages in thread
From: Didier Spaier @ 2020-03-11 17:07 UTC (permalink / raw)
  To: grub-devel



Le 11/03/2020 à 13:43, Daniel Kiper a écrit :
> Adding Michael, Mihai, Javier and Peter...
> 
> Below you can find what more or less Vladimir and I agreed WRT small MBR
> gap. In general Vladimir convinced me to phase out small MBR gaps
> support gradually. This is first step in this journey. We think that we
> have to build some warnings into the code and extend documentation.
> Please chime in what you think about that...
> 
> On Tue, Mar 10, 2020 at 01:23:10PM +0100, Vladimir 'phcoder' Serbinenko wrote:
>> Daniel, do you want to adjust the whitelist?
>>
>> We don't want to support small MBR gap in pair with anything but
>> the simplest config of biosdisk+part_msdos+simple filesystem. In this
>> path "simple filesystems" are all current filesystems except zfs and
>> btrfs
> 
> Missing SOB...
> 
>> ---
>>  grub-core/partmap/gpt.c     |  9 ++++++++-
>>  grub-core/partmap/msdos.c   |  7 ++++++-
>>  include/grub/partition.h    |  3 ++-
>>  include/grub/util/install.h |  7 +++++--
>>  util/grub-install-common.c  | 24 ++++++++++++++++++++++++
>>  util/grub-install.c         | 10 +++++++---
>>  util/grub-setup.c           |  2 +-
>>  util/setup.c                |  5 +++--
>>  8 files changed, 56 insertions(+), 11 deletions(-)
>>
>> diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
>> index 103f6796f..25a5a1934 100644
>> --- a/grub-core/partmap/gpt.c
>> +++ b/grub-core/partmap/gpt.c
>> @@ -25,6 +25,9 @@
>>  #include <grub/msdos_partition.h>
>>  #include <grub/gpt_partition.h>
>>  #include <grub/i18n.h>
>> +#ifdef GRUB_UTIL
>> +#include <grub/emu/misc.h>
>> +#endif
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -169,7 +172,8 @@ static grub_err_t
>>  gpt_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
>>   unsigned int max_nsectors,
>>   grub_embed_type_t embed_type,
>> - grub_disk_addr_t **sectors)
>> + grub_disk_addr_t **sectors,
>> + int warn_short)
>>  {
>>    struct gpt_partition_map_embed_ctx ctx = {
>>      .start = 0,
>> @@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
>> unsigned int *nsectors,
>>          N_("this GPT partition label contains no BIOS Boot Partition;"
>>     " embedding won't be possible"));
>>
>> +  if (ctx.len < 450) {
> 
> Could you define constant somewhere?
> 
> Is it in sectors? Why 450? Should not it be 2048 if 1 MiB below?

Whichs lead to a question: would the slot between 24K (or maybe
32K or 64K if it is wise to round it?) and 1M still a good fit for
a Bios boot partition in case of a gpt? if not in which cases?

> ...and missing "&& warn_short" check...
> 
>> +    grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
>> increase its size.");
>> +  }
>>    if (ctx.len < *nsectors)
> 
> Could not we use this check somehow instead of adding new one?
> 
>>      return grub_error (GRUB_ERR_OUT_OF_RANGE,
>>          N_("your BIOS Boot Partition is too small;"
>> diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
>> index 7b8e45076..402bce050 100644
>> --- a/grub-core/partmap/msdos.c
>> +++ b/grub-core/partmap/msdos.c
>> @@ -236,7 +236,8 @@ static grub_err_t
>>  pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
>>   unsigned int max_nsectors,
>>   grub_embed_type_t embed_type,
>> - grub_disk_addr_t **sectors)
>> + grub_disk_addr_t **sectors,
>> + int warn_short)
>>  {
>>    grub_disk_addr_t end = ~0ULL;
>>    struct grub_msdos_partition_mbr mbr;
>> @@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
>> unsigned int *nsectors,
>>        return GRUB_ERR_NONE;
>>      }
>>
>> +  if (end < 450 && warn_short) {
>> +    grub_util_warn("You have a short MBR gap and use advanced config.
>> Please increase post-MBR gap");
> 
> Ditto?
> 
>> +  }
>> +
>>    if (end <= 1)
>>      return grub_error (GRUB_ERR_FILE_NOT_FOUND,
>>          N_("this msdos-style partition label has no "
>> diff --git a/include/grub/partition.h b/include/grub/partition.h
>> index 7adb7ec6e..5697e28d5 100644
>> --- a/include/grub/partition.h
>> +++ b/include/grub/partition.h
>> @@ -55,7 +55,8 @@ struct grub_partition_map
>>    grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
>>          unsigned int max_nsectors,
>>          grub_embed_type_t embed_type,
>> -        grub_disk_addr_t **sectors);
>> +        grub_disk_addr_t **sectors,
>> +        int warn_short);
>>  #endif
>>  };
>>  typedef struct grub_partition_map *grub_partition_map_t;
>> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
>> index 2631b1074..982115f57 100644
>> --- a/include/grub/util/install.h
>> +++ b/include/grub/util/install.h
>> @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
>>         const char *boot_file, const char *core_file,
>>         const char *dest, int force,
>>         int fs_probe, int allow_floppy,
>> -       int add_rs_codes);
>> +       int add_rs_codes, int warn_short_mbr_gap);
>>  void
>>  grub_util_sparc_setup (const char *dir,
>>          const char *boot_file, const char *core_file,
>>          const char *dest, int force,
>>          int fs_probe, int allow_floppy,
>> -        int add_rs_codes);
>> +        int add_rs_codes, int warn_short_mbr_gap);
>>
>>  char *
>>  grub_install_get_image_targets_string (void);
>> @@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
>> grub_install_image_target_desc *t);
>>  extern char *grub_install_copy_buffer;
>>  #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576
>>
>> +int
> 
> extern int
> 
>> +grub_install_is_short_mgrgap_supported(void);
>> +
>>  #endif
>> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
>> index ca0ac612a..e4cff2871 100644
>> --- a/util/grub-install-common.c
>> +++ b/util/grub-install-common.c
>> @@ -234,6 +234,30 @@ char *grub_install_source_directory = NULL;
>>  char *grub_install_locale_directory = NULL;
>>  char *grub_install_themes_directory = NULL;
>>
>> +int
>> +grub_install_is_short_mgrgap_supported()
>> +{
>> +  int i, j;
>> +  static const char *whitelist[] =
>> +    {
>> +     "part_msdos", "biosdisk", "affs", "afs", "bfs", "archelp",
>> +     "cpio", "cpio_be", "newc", "odc", "ext2", "fat", "exfat",
>> +     "f2fs", "fshelp", "hfs", "hfsplus", "hfspluscomp",
>> +     "iso9660", "jfs", "minix", "minix2", "minix3", "minix_be",
>> +     "minix2_be", "minix2_be", "nilfs2", "ntfs", "ntfscomp",
>> +     "reiserfs", "romfs", "sfs", "squash4", "tar", "udf",
>> +     "ufs1", "ufs1_be", "ufs2", "xfs"
>> +    };
> 
> The list LGTM...
> 
>> +  for (i = 0; i < modules.n_entries; i++) {
>> +    for (j = 0; j < ARRAY_SIZE (whitelist); j++)
>> +      if (strcmp(modules.entries[i], whitelist[j]) == 0)
>> + break;
>> +    if (j == ARRAY_SIZE (whitelist))
>> +      return 0;
>> +  }
>> +  return 1;
>> +}
>> +
>>  void
>>  grub_install_push_module (const char *val)
>>  {
>> diff --git a/util/grub-install.c b/util/grub-install.c
>> index 8970b73aa..ba27657d9 100644
>> --- a/util/grub-install.c
>> +++ b/util/grub-install.c
>> @@ -1718,10 +1718,14 @@ main (int argc, char *argv[])
>>   install_device);
>>
>>   /*  Now perform the installation.  */
>> - if (install_bootsector)
>> + if (install_bootsector) {
>> +   int warn_short_mbr_gap = grub_install_is_short_mgrgap_supported();
>> +
>>     grub_util_bios_setup (platdir, "boot.img", "core.img",
>>   install_drive, force,
>> - fs_probe, allow_floppy, add_rs_codes);
>> + fs_probe, allow_floppy, add_rs_codes,
>> + warn_short_mbr_gap);
>> + }
>>   break;
>>        }
>>      case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
>> @@ -1748,7 +1752,7 @@ main (int argc, char *argv[])
>>     grub_util_sparc_setup (platdir, "boot.img", "core.img",
>>   install_drive, force,
>>   fs_probe, allow_floppy,
>> - 0 /* unused */ );
>> + 0 /* unused */, 0 /* unused */ );
>>   break;
>>        }
>>
>> diff --git a/util/grub-setup.c b/util/grub-setup.c
>> index 42b98ad3c..1783224dd 100644
>> --- a/util/grub-setup.c
>> +++ b/util/grub-setup.c
>> @@ -315,7 +315,7 @@ main (int argc, char *argv[])
>>      arguments.core_file ? : DEFAULT_CORE_FILE,
>>      dest_dev, arguments.force,
>>      arguments.fs_probe, arguments.allow_floppy,
>> -    arguments.add_rs_codes);
>> +    arguments.add_rs_codes, 0);
>>
>>    /* Free resources.  */
>>    grub_fini_all ();
>> diff --git a/util/setup.c b/util/setup.c
>> index 3be88aae1..da5f2c07f 100644
>> --- a/util/setup.c
>> +++ b/util/setup.c
>> @@ -254,7 +254,8 @@ SETUP (const char *dir,
>>         const char *boot_file, const char *core_file,
>>         const char *dest, int force,
>>         int fs_probe, int allow_floppy,
>> -       int add_rs_codes __attribute__ ((unused))) /* unused on sparc64 */
>> +       int add_rs_codes __attribute__ ((unused)), /* unused on sparc64 */
>> +       int warn_small)
>>  {
>>    char *core_path;
>>    char *boot_img, *core_img, *boot_path;
>> @@ -530,7 +531,7 @@ SETUP (const char *dir,
>>   GRUB_EMBED_PCBIOS, &sectors);
>>      else if (ctx.dest_partmap)
>>        err = ctx.dest_partmap->embed (dest_dev->disk, &nsec, maxsec,
>> -      GRUB_EMBED_PCBIOS, &sectors);
>> +      GRUB_EMBED_PCBIOS, &sectors, warn_small);
>>      else
>>        err = fs->fs_embed (dest_dev, &nsec, maxsec,
>>     GRUB_EMBED_PCBIOS, &sectors);
> 
> ...plus some missing spaces in function names and other places...
> 
> Additionally, please extend relevant doc section with explanation why we
> are doing that. And maybe with an schedule for phase out...
> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Warn if MBR gap is small and user uses advanced modules
  2020-03-11 17:07   ` Didier Spaier
@ 2020-03-11 17:42     ` Vladimir 'phcoder' Serbinenko
  0 siblings, 0 replies; 8+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2020-03-11 17:42 UTC (permalink / raw)
  To: The development of GNU GRUB

[-- Attachment #1: Type: text/plain, Size: 10729 bytes --]

On Wed, Mar 11, 2020, 18:08 Didier Spaier <didier@slint.fr> wrote:

>
>
> Le 11/03/2020 à 13:43, Daniel Kiper a écrit :
> > Adding Michael, Mihai, Javier and Peter...
> >
> > Below you can find what more or less Vladimir and I agreed WRT small MBR
> > gap. In general Vladimir convinced me to phase out small MBR gaps
> > support gradually. This is first step in this journey. We think that we
> > have to build some warnings into the code and extend documentation.
> > Please chime in what you think about that...
> >
> > On Tue, Mar 10, 2020 at 01:23:10PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> >> Daniel, do you want to adjust the whitelist?
> >>
> >> We don't want to support small MBR gap in pair with anything but
> >> the simplest config of biosdisk+part_msdos+simple filesystem. In this
> >> path "simple filesystems" are all current filesystems except zfs and
> >> btrfs
> >
> > Missing SOB...
> >
> >> ---
> >>  grub-core/partmap/gpt.c     |  9 ++++++++-
> >>  grub-core/partmap/msdos.c   |  7 ++++++-
> >>  include/grub/partition.h    |  3 ++-
> >>  include/grub/util/install.h |  7 +++++--
> >>  util/grub-install-common.c  | 24 ++++++++++++++++++++++++
> >>  util/grub-install.c         | 10 +++++++---
> >>  util/grub-setup.c           |  2 +-
> >>  util/setup.c                |  5 +++--
> >>  8 files changed, 56 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> >> index 103f6796f..25a5a1934 100644
> >> --- a/grub-core/partmap/gpt.c
> >> +++ b/grub-core/partmap/gpt.c
> >> @@ -25,6 +25,9 @@
> >>  #include <grub/msdos_partition.h>
> >>  #include <grub/gpt_partition.h>
> >>  #include <grub/i18n.h>
> >> +#ifdef GRUB_UTIL
> >> +#include <grub/emu/misc.h>
> >> +#endif
> >>
> >>  GRUB_MOD_LICENSE ("GPLv3+");
> >>
> >> @@ -169,7 +172,8 @@ static grub_err_t
> >>  gpt_partition_map_embed (struct grub_disk *disk, unsigned int
> *nsectors,
> >>   unsigned int max_nsectors,
> >>   grub_embed_type_t embed_type,
> >> - grub_disk_addr_t **sectors)
> >> + grub_disk_addr_t **sectors,
> >> + int warn_short)
> >>  {
> >>    struct gpt_partition_map_embed_ctx ctx = {
> >>      .start = 0,
> >> @@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
> >> unsigned int *nsectors,
> >>          N_("this GPT partition label contains no BIOS Boot Partition;"
> >>     " embedding won't be possible"));
> >>
> >> +  if (ctx.len < 450) {
> >
> > Could you define constant somewhere?
> >
> > Is it in sectors? Why 450? Should not it be 2048 if 1 MiB below?
>
> Whichs lead to a question: would the slot between 24K (or maybe
> 32K or 64K if it is wise to round it?) and 1M still a good fit for
> a Bios boot partition in case of a gpt? if not in which cases?
>

BIOS boot partition should never be under 960K. Just it was never a problem
to begin with.

>
> > ...and missing "&& warn_short" check...
> >
> >> +    grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
> >> increase its size.");
> >> +  }
> >>    if (ctx.len < *nsectors)
> >
> > Could not we use this check somehow instead of adding new one?
> >
> >>      return grub_error (GRUB_ERR_OUT_OF_RANGE,
> >>          N_("your BIOS Boot Partition is too small;"
> >> diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
> >> index 7b8e45076..402bce050 100644
> >> --- a/grub-core/partmap/msdos.c
> >> +++ b/grub-core/partmap/msdos.c
> >> @@ -236,7 +236,8 @@ static grub_err_t
> >>  pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> >>   unsigned int max_nsectors,
> >>   grub_embed_type_t embed_type,
> >> - grub_disk_addr_t **sectors)
> >> + grub_disk_addr_t **sectors,
> >> + int warn_short)
> >>  {
> >>    grub_disk_addr_t end = ~0ULL;
> >>    struct grub_msdos_partition_mbr mbr;
> >> @@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
> >> unsigned int *nsectors,
> >>        return GRUB_ERR_NONE;
> >>      }
> >>
> >> +  if (end < 450 && warn_short) {
> >> +    grub_util_warn("You have a short MBR gap and use advanced config.
> >> Please increase post-MBR gap");
> >
> > Ditto?
> >
> >> +  }
> >> +
> >>    if (end <= 1)
> >>      return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> >>          N_("this msdos-style partition label has no "
> >> diff --git a/include/grub/partition.h b/include/grub/partition.h
> >> index 7adb7ec6e..5697e28d5 100644
> >> --- a/include/grub/partition.h
> >> +++ b/include/grub/partition.h
> >> @@ -55,7 +55,8 @@ struct grub_partition_map
> >>    grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
> >>          unsigned int max_nsectors,
> >>          grub_embed_type_t embed_type,
> >> -        grub_disk_addr_t **sectors);
> >> +        grub_disk_addr_t **sectors,
> >> +        int warn_short);
> >>  #endif
> >>  };
> >>  typedef struct grub_partition_map *grub_partition_map_t;
> >> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> >> index 2631b1074..982115f57 100644
> >> --- a/include/grub/util/install.h
> >> +++ b/include/grub/util/install.h
> >> @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
> >>         const char *boot_file, const char *core_file,
> >>         const char *dest, int force,
> >>         int fs_probe, int allow_floppy,
> >> -       int add_rs_codes);
> >> +       int add_rs_codes, int warn_short_mbr_gap);
> >>  void
> >>  grub_util_sparc_setup (const char *dir,
> >>          const char *boot_file, const char *core_file,
> >>          const char *dest, int force,
> >>          int fs_probe, int allow_floppy,
> >> -        int add_rs_codes);
> >> +        int add_rs_codes, int warn_short_mbr_gap);
> >>
> >>  char *
> >>  grub_install_get_image_targets_string (void);
> >> @@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
> >> grub_install_image_target_desc *t);
> >>  extern char *grub_install_copy_buffer;
> >>  #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576
> >>
> >> +int
> >
> > extern int
> >
> >> +grub_install_is_short_mgrgap_supported(void);
> >> +
> >>  #endif
> >> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> >> index ca0ac612a..e4cff2871 100644
> >> --- a/util/grub-install-common.c
> >> +++ b/util/grub-install-common.c
> >> @@ -234,6 +234,30 @@ char *grub_install_source_directory = NULL;
> >>  char *grub_install_locale_directory = NULL;
> >>  char *grub_install_themes_directory = NULL;
> >>
> >> +int
> >> +grub_install_is_short_mgrgap_supported()
> >> +{
> >> +  int i, j;
> >> +  static const char *whitelist[] =
> >> +    {
> >> +     "part_msdos", "biosdisk", "affs", "afs", "bfs", "archelp",
> >> +     "cpio", "cpio_be", "newc", "odc", "ext2", "fat", "exfat",
> >> +     "f2fs", "fshelp", "hfs", "hfsplus", "hfspluscomp",
> >> +     "iso9660", "jfs", "minix", "minix2", "minix3", "minix_be",
> >> +     "minix2_be", "minix2_be", "nilfs2", "ntfs", "ntfscomp",
> >> +     "reiserfs", "romfs", "sfs", "squash4", "tar", "udf",
> >> +     "ufs1", "ufs1_be", "ufs2", "xfs"
> >> +    };
> >
> > The list LGTM...
> >
> >> +  for (i = 0; i < modules.n_entries; i++) {
> >> +    for (j = 0; j < ARRAY_SIZE (whitelist); j++)
> >> +      if (strcmp(modules.entries[i], whitelist[j]) == 0)
> >> + break;
> >> +    if (j == ARRAY_SIZE (whitelist))
> >> +      return 0;
> >> +  }
> >> +  return 1;
> >> +}
> >> +
> >>  void
> >>  grub_install_push_module (const char *val)
> >>  {
> >> diff --git a/util/grub-install.c b/util/grub-install.c
> >> index 8970b73aa..ba27657d9 100644
> >> --- a/util/grub-install.c
> >> +++ b/util/grub-install.c
> >> @@ -1718,10 +1718,14 @@ main (int argc, char *argv[])
> >>   install_device);
> >>
> >>   /*  Now perform the installation.  */
> >> - if (install_bootsector)
> >> + if (install_bootsector) {
> >> +   int warn_short_mbr_gap = grub_install_is_short_mgrgap_supported();
> >> +
> >>     grub_util_bios_setup (platdir, "boot.img", "core.img",
> >>   install_drive, force,
> >> - fs_probe, allow_floppy, add_rs_codes);
> >> + fs_probe, allow_floppy, add_rs_codes,
> >> + warn_short_mbr_gap);
> >> + }
> >>   break;
> >>        }
> >>      case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
> >> @@ -1748,7 +1752,7 @@ main (int argc, char *argv[])
> >>     grub_util_sparc_setup (platdir, "boot.img", "core.img",
> >>   install_drive, force,
> >>   fs_probe, allow_floppy,
> >> - 0 /* unused */ );
> >> + 0 /* unused */, 0 /* unused */ );
> >>   break;
> >>        }
> >>
> >> diff --git a/util/grub-setup.c b/util/grub-setup.c
> >> index 42b98ad3c..1783224dd 100644
> >> --- a/util/grub-setup.c
> >> +++ b/util/grub-setup.c
> >> @@ -315,7 +315,7 @@ main (int argc, char *argv[])
> >>      arguments.core_file ? : DEFAULT_CORE_FILE,
> >>      dest_dev, arguments.force,
> >>      arguments.fs_probe, arguments.allow_floppy,
> >> -    arguments.add_rs_codes);
> >> +    arguments.add_rs_codes, 0);
> >>
> >>    /* Free resources.  */
> >>    grub_fini_all ();
> >> diff --git a/util/setup.c b/util/setup.c
> >> index 3be88aae1..da5f2c07f 100644
> >> --- a/util/setup.c
> >> +++ b/util/setup.c
> >> @@ -254,7 +254,8 @@ SETUP (const char *dir,
> >>         const char *boot_file, const char *core_file,
> >>         const char *dest, int force,
> >>         int fs_probe, int allow_floppy,
> >> -       int add_rs_codes __attribute__ ((unused))) /* unused on sparc64
> */
> >> +       int add_rs_codes __attribute__ ((unused)), /* unused on sparc64
> */
> >> +       int warn_small)
> >>  {
> >>    char *core_path;
> >>    char *boot_img, *core_img, *boot_path;
> >> @@ -530,7 +531,7 @@ SETUP (const char *dir,
> >>   GRUB_EMBED_PCBIOS, &sectors);
> >>      else if (ctx.dest_partmap)
> >>        err = ctx.dest_partmap->embed (dest_dev->disk, &nsec, maxsec,
> >> -      GRUB_EMBED_PCBIOS, &sectors);
> >> +      GRUB_EMBED_PCBIOS, &sectors, warn_small);
> >>      else
> >>        err = fs->fs_embed (dest_dev, &nsec, maxsec,
> >>     GRUB_EMBED_PCBIOS, &sectors);
> >
> > ...plus some missing spaces in function names and other places...
> >
> > Additionally, please extend relevant doc section with explanation why we
> > are doing that. And maybe with an schedule for phase out...
> >
> > Daniel
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 14585 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Warn if MBR gap is small and user uses advanced modules
  2020-03-11 12:43 ` Daniel Kiper
                     ` (2 preceding siblings ...)
  2020-03-11 17:07   ` Didier Spaier
@ 2020-04-27 15:41   ` Vladimir 'phcoder' Serbinenko
  3 siblings, 0 replies; 8+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2020-04-27 15:41 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: The development of GRUB 2, Michael Chang, Mihai Moldovan,
	Javier Martinez Canillas, Peter Jones

On Wed, Mar 11, 2020 at 2:34 PM Daniel Kiper <dkiper@net-space.pl> wrote:
>
> Adding Michael, Mihai, Javier and Peter...
>
> Below you can find what more or less Vladimir and I agreed WRT small MBR
> gap. In general Vladimir convinced me to phase out small MBR gaps
> support gradually. This is first step in this journey. We think that we
> have to build some warnings into the code and extend documentation.
> Please chime in what you think about that...
>
> On Tue, Mar 10, 2020 at 01:23:10PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > Daniel, do you want to adjust the whitelist?
> >
> > We don't want to support small MBR gap in pair with anything but
> > the simplest config of biosdisk+part_msdos+simple filesystem. In this
> > path "simple filesystems" are all current filesystems except zfs and
> > btrfs
>
> Missing SOB...
>
> > ---
> >  grub-core/partmap/gpt.c     |  9 ++++++++-
> >  grub-core/partmap/msdos.c   |  7 ++++++-
> >  include/grub/partition.h    |  3 ++-
> >  include/grub/util/install.h |  7 +++++--
> >  util/grub-install-common.c  | 24 ++++++++++++++++++++++++
> >  util/grub-install.c         | 10 +++++++---
> >  util/grub-setup.c           |  2 +-
> >  util/setup.c                |  5 +++--
> >  8 files changed, 56 insertions(+), 11 deletions(-)
> >
> > diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> > index 103f6796f..25a5a1934 100644
> > --- a/grub-core/partmap/gpt.c
> > +++ b/grub-core/partmap/gpt.c
> > @@ -25,6 +25,9 @@
> >  #include <grub/msdos_partition.h>
> >  #include <grub/gpt_partition.h>
> >  #include <grub/i18n.h>
> > +#ifdef GRUB_UTIL
> > +#include <grub/emu/misc.h>
> > +#endif
> >
> >  GRUB_MOD_LICENSE ("GPLv3+");
> >
> > @@ -169,7 +172,8 @@ static grub_err_t
> >  gpt_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> >   unsigned int max_nsectors,
> >   grub_embed_type_t embed_type,
> > - grub_disk_addr_t **sectors)
> > + grub_disk_addr_t **sectors,
> > + int warn_short)
> >  {
> >    struct gpt_partition_map_embed_ctx ctx = {
> >      .start = 0,
> > @@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
> > unsigned int *nsectors,
> >          N_("this GPT partition label contains no BIOS Boot Partition;"
> >     " embedding won't be possible"));
> >
> > +  if (ctx.len < 450) {
>
> Could you define constant somewhere?
>
> Is it in sectors? Why 450? Should not it be 2048 if 1 MiB below?
Increased to 1900. So slighlty under 1MiB is accepted but not much
smaller. Made it a const
>
> ...and missing "&& warn_short" check...
It's intentional. On GPT we don't want BBP under 1MiB even if setup is simple.
>
> > +    grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
> > increase its size.");
> > +  }
> >    if (ctx.len < *nsectors)
>
> Could not we use this check somehow instead of adding new one?
No, it checks for different thing.
>
> >      return grub_error (GRUB_ERR_OUT_OF_RANGE,
> >          N_("your BIOS Boot Partition is too small;"
> > diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
> > index 7b8e45076..402bce050 100644
> > --- a/grub-core/partmap/msdos.c
> > +++ b/grub-core/partmap/msdos.c
> > @@ -236,7 +236,8 @@ static grub_err_t
> >  pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> >   unsigned int max_nsectors,
> >   grub_embed_type_t embed_type,
> > - grub_disk_addr_t **sectors)
> > + grub_disk_addr_t **sectors,
> > + int warn_short)
> >  {
> >    grub_disk_addr_t end = ~0ULL;
> >    struct grub_msdos_partition_mbr mbr;
> > @@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
> > unsigned int *nsectors,
> >        return GRUB_ERR_NONE;
> >      }
> >
> > +  if (end < 450 && warn_short) {
> > +    grub_util_warn("You have a short MBR gap and use advanced config.
> > Please increase post-MBR gap");
>
> Ditto?
Changed
>
> > +  }
> > +
> >    if (end <= 1)
> >      return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> >          N_("this msdos-style partition label has no "
> > diff --git a/include/grub/partition.h b/include/grub/partition.h
> > index 7adb7ec6e..5697e28d5 100644
> > --- a/include/grub/partition.h
> > +++ b/include/grub/partition.h
> > @@ -55,7 +55,8 @@ struct grub_partition_map
> >    grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
> >          unsigned int max_nsectors,
> >          grub_embed_type_t embed_type,
> > -        grub_disk_addr_t **sectors);
> > +        grub_disk_addr_t **sectors,
> > +        int warn_short);
> >  #endif
> >  };
> >  typedef struct grub_partition_map *grub_partition_map_t;
> > diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> > index 2631b1074..982115f57 100644
> > --- a/include/grub/util/install.h
> > +++ b/include/grub/util/install.h
> > @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
> >         const char *boot_file, const char *core_file,
> >         const char *dest, int force,
> >         int fs_probe, int allow_floppy,
> > -       int add_rs_codes);
> > +       int add_rs_codes, int warn_short_mbr_gap);
> >  void
> >  grub_util_sparc_setup (const char *dir,
> >          const char *boot_file, const char *core_file,
> >          const char *dest, int force,
> >          int fs_probe, int allow_floppy,
> > -        int add_rs_codes);
> > +        int add_rs_codes, int warn_short_mbr_gap);
> >
> >  char *
> >  grub_install_get_image_targets_string (void);
> > @@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
> > grub_install_image_target_desc *t);
> >  extern char *grub_install_copy_buffer;
> >  #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576
> >
> > +int
>
> extern int
>
> > +grub_install_is_short_mgrgap_supported(void);
> > +
> >  #endif
> > diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> > index ca0ac612a..e4cff2871 100644
> > --- a/util/grub-install-common.c
> > +++ b/util/grub-install-common.c
> > @@ -234,6 +234,30 @@ char *grub_install_source_directory = NULL;
> >  char *grub_install_locale_directory = NULL;
> >  char *grub_install_themes_directory = NULL;
> >
> > +int
> > +grub_install_is_short_mgrgap_supported()
> > +{
> > +  int i, j;
> > +  static const char *whitelist[] =
> > +    {
> > +     "part_msdos", "biosdisk", "affs", "afs", "bfs", "archelp",
> > +     "cpio", "cpio_be", "newc", "odc", "ext2", "fat", "exfat",
> > +     "f2fs", "fshelp", "hfs", "hfsplus", "hfspluscomp",
> > +     "iso9660", "jfs", "minix", "minix2", "minix3", "minix_be",
> > +     "minix2_be", "minix2_be", "nilfs2", "ntfs", "ntfscomp",
> > +     "reiserfs", "romfs", "sfs", "squash4", "tar", "udf",
> > +     "ufs1", "ufs1_be", "ufs2", "xfs"
> > +    };
>
> The list LGTM...
>
> > +  for (i = 0; i < modules.n_entries; i++) {
> > +    for (j = 0; j < ARRAY_SIZE (whitelist); j++)
> > +      if (strcmp(modules.entries[i], whitelist[j]) == 0)
> > + break;
> > +    if (j == ARRAY_SIZE (whitelist))
> > +      return 0;
> > +  }
> > +  return 1;
> > +}
> > +
> >  void
> >  grub_install_push_module (const char *val)
> >  {
> > diff --git a/util/grub-install.c b/util/grub-install.c
> > index 8970b73aa..ba27657d9 100644
> > --- a/util/grub-install.c
> > +++ b/util/grub-install.c
> > @@ -1718,10 +1718,14 @@ main (int argc, char *argv[])
> >   install_device);
> >
> >   /*  Now perform the installation.  */
> > - if (install_bootsector)
> > + if (install_bootsector) {
> > +   int warn_short_mbr_gap = grub_install_is_short_mgrgap_supported();
> > +
> >     grub_util_bios_setup (platdir, "boot.img", "core.img",
> >   install_drive, force,
> > - fs_probe, allow_floppy, add_rs_codes);
> > + fs_probe, allow_floppy, add_rs_codes,
> > + warn_short_mbr_gap);
> > + }
> >   break;
> >        }
> >      case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
> > @@ -1748,7 +1752,7 @@ main (int argc, char *argv[])
> >     grub_util_sparc_setup (platdir, "boot.img", "core.img",
> >   install_drive, force,
> >   fs_probe, allow_floppy,
> > - 0 /* unused */ );
> > + 0 /* unused */, 0 /* unused */ );
> >   break;
> >        }
> >
> > diff --git a/util/grub-setup.c b/util/grub-setup.c
> > index 42b98ad3c..1783224dd 100644
> > --- a/util/grub-setup.c
> > +++ b/util/grub-setup.c
> > @@ -315,7 +315,7 @@ main (int argc, char *argv[])
> >      arguments.core_file ? : DEFAULT_CORE_FILE,
> >      dest_dev, arguments.force,
> >      arguments.fs_probe, arguments.allow_floppy,
> > -    arguments.add_rs_codes);
> > +    arguments.add_rs_codes, 0);
> >
> >    /* Free resources.  */
> >    grub_fini_all ();
> > diff --git a/util/setup.c b/util/setup.c
> > index 3be88aae1..da5f2c07f 100644
> > --- a/util/setup.c
> > +++ b/util/setup.c
> > @@ -254,7 +254,8 @@ SETUP (const char *dir,
> >         const char *boot_file, const char *core_file,
> >         const char *dest, int force,
> >         int fs_probe, int allow_floppy,
> > -       int add_rs_codes __attribute__ ((unused))) /* unused on sparc64 */
> > +       int add_rs_codes __attribute__ ((unused)), /* unused on sparc64 */
> > +       int warn_small)
> >  {
> >    char *core_path;
> >    char *boot_img, *core_img, *boot_path;
> > @@ -530,7 +531,7 @@ SETUP (const char *dir,
> >   GRUB_EMBED_PCBIOS, &sectors);
> >      else if (ctx.dest_partmap)
> >        err = ctx.dest_partmap->embed (dest_dev->disk, &nsec, maxsec,
> > -      GRUB_EMBED_PCBIOS, &sectors);
> > +      GRUB_EMBED_PCBIOS, &sectors, warn_small);
> >      else
> >        err = fs->fs_embed (dest_dev, &nsec, maxsec,
> >     GRUB_EMBED_PCBIOS, &sectors);
>
> ...plus some missing spaces in function names and other places...
>
> Additionally, please extend relevant doc section with explanation why we
> are doing that. And maybe with an schedule for phase out...
>
> Daniel



-- 
Regards
Vladimir 'phcoder' Serbinenko


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-04-27 15:41 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 12:23 [PATCH] Warn if MBR gap is small and user uses advanced modules Vladimir 'phcoder' Serbinenko
2020-03-10 12:30 ` Mihai Moldovan
2020-03-11 12:43 ` Daniel Kiper
2020-03-11 14:56   ` Javier Martinez Canillas
2020-03-11 15:28   ` Michael Chang
2020-03-11 17:07   ` Didier Spaier
2020-03-11 17:42     ` Vladimir 'phcoder' Serbinenko
2020-04-27 15:41   ` Vladimir 'phcoder' Serbinenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.