On Wed, Mar 11, 2020 at 2:34 PM Daniel Kiper <dki...@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, §ors); > > else if (ctx.dest_partmap) > > err = ctx.dest_partmap->embed (dest_dev->disk, &nsec, maxsec, > > - GRUB_EMBED_PCBIOS, §ors); > > + GRUB_EMBED_PCBIOS, §ors, warn_small); > > else > > err = fs->fs_embed (dest_dev, &nsec, maxsec, > > GRUB_EMBED_PCBIOS, §ors); > > ...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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel