On Wed, Mar 11, 2020, 18:08 Didier Spaier <did...@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, §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
> >
> > _______________________________________________
> > 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
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
