[PATCH] improve error messages in grub-setup

[PATCH] improve error messages in grub-setup

[Robert Millan]

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


This patch improves error messages in grub-setup, and adds a few
warnings when requested to install in odd layouts.

Since there was no facility to emmit a warning that is always
visible (regardless of verbosity), but doesn't abort execution,
I added one (grub_util_warn ()).  Is everyone fine with using
this interface?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: embedding_warnings.diff --]
[-- Type: text/x-diff, Size: 3778 bytes --]

2009-05-02  Robert Millan  <rmh.grub@aybabtu.com>

	* util/misc.c (grub_util_warn): New function.  Emmits a warning
	unconditionally.
	* include/grub/util/misc.h (grub_util_warn): New declaration.

	* util/i386/pc/grub-setup.c (setup): Improve error messages and
	add warnings when requested to install in odd layouts.

Index: include/grub/util/misc.h
===================================================================
--- include/grub/util/misc.h	(revision 2154)
+++ include/grub/util/misc.h	(working copy)
@@ -40,6 +40,7 @@ extern char *progname;
 extern int verbosity;
 extern jmp_buf main_env;
 
+void grub_util_warn (const char *fmt, ...);
 void grub_util_info (const char *fmt, ...);
 void grub_util_error (const char *fmt, ...) __attribute__ ((noreturn));
 
Index: util/i386/pc/grub-setup.c
===================================================================
--- util/i386/pc/grub-setup.c	(revision 2154)
+++ util/i386/pc/grub-setup.c	(working copy)
@@ -109,7 +109,7 @@ setup (const char *dir,
   FILE *fp;
   struct { grub_uint64_t start; grub_uint64_t end; } embed_region;
   embed_region.start = embed_region.end = ~0UL;
-  int able_to_embed = 1;
+  int embedding_area_exists = 0, able_to_embed = 0;
   
   auto void NESTED_FUNC_ATTR save_first_sector (grub_disk_addr_t sector, unsigned offset,
 			       unsigned length);
@@ -304,6 +304,12 @@ setup (const char *dir,
   
   grub_util_info ("dos partition is %d, bsd partition is %d",
 		  dos_part, bsd_part);
+
+  if (! dest_dev->disk->has_partitions)
+    grub_util_warn ("Attempting to install GRUB to a partitionless disk.  This is a BAD idea.");
+
+  if (dest_dev->disk->partition)
+    grub_util_warn ("Attempting to install GRUB to a partition instead of the MBR.  This is a BAD idea.");
   
   /* If the destination device can have partitions and it is the MBR,
      try to embed the core image into after the MBR.  */
@@ -311,11 +317,16 @@ setup (const char *dir,
     {
       grub_partition_iterate (dest_dev->disk, find_usable_region);
 
+      if (embed_region.end != 0)
+	embedding_area_exists = 1;
+
       /* If there is enough space...  */
       if ((unsigned long) core_sectors <= embed_region.end - embed_region.start)
 	{
 	  grub_util_info ("will embed the core image at sector 0x%llx", embed_region.start);
 
+	  able_to_embed = 1;
+
 	  *install_dos_part = grub_cpu_to_le32 (dos_part);
 	  *install_bsd_part = grub_cpu_to_le32 (bsd_part);
 
@@ -349,16 +360,20 @@ setup (const char *dir,
 
 	  goto finish;
 	}
+    }
+
+  if (! able_to_embed)
+    {
+      if (embedding_area_exists)
+	grub_util_warn ("Embedding area is too small for core.img.");
       else
-        able_to_embed = 0;
+	grub_util_warn ("Embedding area not present at all!");
+
+      if (must_embed)
+	grub_util_error ("Embedding is not possible, but this is required when "
+			 "the root device is on a RAID array or LVM volume.");
     }
-  else
-    able_to_embed = 0;
 
-  if (must_embed && ! able_to_embed)
-    grub_util_error ("Core image is too big for embedding, but this is required when\n"
-		     "the root device is on a RAID array or LVM volume.");
-  
   /* The core image must be put on a filesystem unfortunately.  */
   grub_util_info ("will leave the core image on the filesystem");
   
Index: util/misc.c
===================================================================
--- util/misc.c	(revision 2154)
+++ util/misc.c	(working copy)
@@ -52,6 +52,19 @@ char *progname = 0;
 int verbosity = 0;
 
 void
+grub_util_warn (const char *fmt, ...)
+{
+  va_list ap;
+  
+  fprintf (stderr, "%s: warn: ", progname);
+  va_start (ap, fmt);
+  vfprintf (stderr, fmt, ap);
+  va_end (ap);
+  fputc ('\n', stderr);
+  fflush (stderr);
+}
+
+void
 grub_util_info (const char *fmt, ...)
 {
   if (verbosity > 0)

Re: [PATCH] improve error messages in grub-setup

[Pavel Roskin]

On Sat, 2009-05-02 at 13:15 +0200, Robert Millan wrote:
> This patch improves error messages in grub-setup, and adds a few
> warnings when requested to install in odd layouts.
> 
> Since there was no facility to emmit a warning that is always
> visible (regardless of verbosity), but doesn't abort execution,
> I added one (grub_util_warn ()).  Is everyone fine with using
> this interface?

I'm fine with the code, but the warnings are quite serious.  I think
grub-install should not proceed if there are any warnings unless there
is an option to ignore warnings (e.g. --ignore-warnings or --force).

-- 
Regards,
Pavel Roskin

Re: [PATCH] improve error messages in grub-setup

[Robert Millan]

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

On Sun, May 03, 2009 at 12:35:08AM -0400, Pavel Roskin wrote:
> On Sat, 2009-05-02 at 13:15 +0200, Robert Millan wrote:
> > This patch improves error messages in grub-setup, and adds a few
> > warnings when requested to install in odd layouts.
> > 
> > Since there was no facility to emmit a warning that is always
> > visible (regardless of verbosity), but doesn't abort execution,
> > I added one (grub_util_warn ()).  Is everyone fine with using
> > this interface?
> 
> I'm fine with the code, but the warnings are quite serious.  I think
> grub-install should not proceed if there are any warnings unless there
> is an option to ignore warnings (e.g. --ignore-warnings or --force).

Agreed.  It's no harm to add --force for users who are stuck in such layouts.

Here's a new patch.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: embedding_warnings.diff --]
[-- Type: text/x-diff, Size: 7916 bytes --]

2009-05-03  Robert Millan  <rmh.grub@aybabtu.com>

	* util/misc.c (grub_util_warn): New function.  Emmits a warning
	unconditionally.
	* include/grub/util/misc.h (grub_util_warn): New declaration.

	* util/i386/pc/grub-install.in: Understand --force and pass it down
	to grub-setup.

	* util/i386/pc/grub-setup.c (main): Understand --force and pass it
	down to setup().
	(setup): Improve error messages and add warnings when requested to
	install in odd layouts.  Refuse to install using blocklists unless
	--force was set.

Index: include/grub/util/misc.h
===================================================================
--- include/grub/util/misc.h	(revision 2155)
+++ include/grub/util/misc.h	(working copy)
@@ -40,6 +40,7 @@ extern char *progname;
 extern int verbosity;
 extern jmp_buf main_env;
 
+void grub_util_warn (const char *fmt, ...);
 void grub_util_info (const char *fmt, ...);
 void grub_util_error (const char *fmt, ...) __attribute__ ((noreturn));
 
Index: util/i386/pc/grub-install.in
===================================================================
--- util/i386/pc/grub-install.in	(revision 2155)
+++ util/i386/pc/grub-install.in	(working copy)
@@ -67,6 +67,7 @@ Install GRUB on your drive.
   --grub-probe=FILE       use FILE as grub-probe
   --no-floppy             do not probe any floppy drive
   --recheck               probe a device map even if it already exists
+  --force                 install even if problems are detected
 
 INSTALL_DEVICE can be a GRUB device name or a system device filename.
 
@@ -106,6 +107,8 @@ for option in "$@"; do
     # This is an undocumented feature...
     --debug)
 	debug=yes ;;
+    -f | --force)
+        setup_force="--force" ;;
     -*)
 	echo "Unrecognized option \`$option'" 1>&2
 	usage
@@ -295,7 +298,7 @@ if [ "${target_cpu}-${platform}" = "i386
     $grub_mkimage --output=${grubdir}/core.img --prefix=${prefix_drive}${relative_grubdir} $modules || exit 1
 
     # Now perform the installation.
-    $grub_setup ${setup_verbose} --directory=${grubdir} --device-map=${device_map} \
+    $grub_setup ${setup_verbose} ${setup_force} --directory=${grubdir} --device-map=${device_map} \
         ${install_device} || exit 1
 else
     $grub_mkimage -d ${pkglibdir} --output=/boot/multiboot.img --prefix=${prefix_drive}${relative_grubdir} $modules || exit 1
Index: util/i386/pc/grub-setup.c
===================================================================
--- util/i386/pc/grub-setup.c	(revision 2155)
+++ util/i386/pc/grub-setup.c	(working copy)
@@ -86,7 +86,7 @@ grub_refresh (void)
 static void
 setup (const char *dir,
        const char *boot_file, const char *core_file,
-       const char *root, const char *dest, int must_embed)
+       const char *root, const char *dest, int must_embed, int force)
 {
   char *boot_path, *core_path, *core_path_dev;
   char *boot_img, *core_img;
@@ -109,7 +109,7 @@ setup (const char *dir,
   FILE *fp;
   struct { grub_uint64_t start; grub_uint64_t end; } embed_region;
   embed_region.start = embed_region.end = ~0UL;
-  int able_to_embed = 1;
+  int embedding_area_exists = 0;
   
   auto void NESTED_FUNC_ATTR save_first_sector (grub_disk_addr_t sector, unsigned offset,
 			       unsigned length);
@@ -304,6 +304,12 @@ setup (const char *dir,
   
   grub_util_info ("dos partition is %d, bsd partition is %d",
 		  dos_part, bsd_part);
+
+  if (! dest_dev->disk->has_partitions)
+    grub_util_warn ("Attempting to install GRUB to a partitionless disk.  This is a BAD idea.");
+
+  if (dest_dev->disk->partition)
+    grub_util_warn ("Attempting to install GRUB to a partition instead of the MBR.  This is a BAD idea.");
   
   /* If the destination device can have partitions and it is the MBR,
      try to embed the core image into after the MBR.  */
@@ -311,6 +317,9 @@ setup (const char *dir,
     {
       grub_partition_iterate (dest_dev->disk, find_usable_region);
 
+      if (embed_region.end != 0)
+	embedding_area_exists = 1;
+
       /* If there is enough space...  */
       if ((unsigned long) core_sectors <= embed_region.end - embed_region.start)
 	{
@@ -349,18 +358,24 @@ setup (const char *dir,
 
 	  goto finish;
 	}
-      else
-        able_to_embed = 0;
     }
-  else
-    able_to_embed = 0;
 
-  if (must_embed && ! able_to_embed)
-    grub_util_error ("Core image is too big for embedding, but this is required when\n"
+  /* If we reached this point, it means we were unable to embed.  */
+
+  if (embedding_area_exists)
+    grub_util_warn ("Embedding area is too small for core.img.");
+  else
+    grub_util_warn ("Embedding area is not present at all!");
+  
+  if (must_embed)
+    grub_util_error ("Embedding is not possible, but this is required when "
 		     "the root device is on a RAID array or LVM volume.");
   
-  /* The core image must be put on a filesystem unfortunately.  */
-  grub_util_info ("will leave the core image on the filesystem");
+  grub_util_warn ("Embedding is not possible.  GRUB can only be installed in this "
+		  "setup by using blocklists.  However, blocklists are UNRELIABLE and "
+		  "its use is discouraged.");
+  if (! force)
+    grub_util_error ("If you really want blocklists, use --force.");
   
   /* Make sure that GRUB reads the identical image as the OS.  */
   tmp_img = xmalloc (core_size);
@@ -510,6 +525,7 @@ static struct option options[] =
     {"directory", required_argument, 0, 'd'},
     {"device-map", required_argument, 0, 'm'},
     {"root-device", required_argument, 0, 'r'},
+    {"force", no_argument, 0, 'f'},
     {"help", no_argument, 0, 'h'},
     {"version", no_argument, 0, 'V'},
     {"verbose", no_argument, 0, 'v'},
@@ -533,6 +549,7 @@ DEVICE must be a GRUB device (e.g. ``(hd
   -d, --directory=DIR     use GRUB files in the directory DIR [default=%s]\n\
   -m, --device-map=FILE   use FILE as the device map [default=%s]\n\
   -r, --root-device=DEV   use DEV as the root device [default=guessed]\n\
+  -f, --force             install even if problems are detected\n\
   -h, --help              display this message and exit\n\
   -V, --version           print version information and exit\n\
   -v, --verbose           print verbose messages\n\
@@ -566,14 +583,14 @@ main (int argc, char *argv[])
   char *dev_map = 0;
   char *root_dev = 0;
   char *dest_dev;
-  int must_embed = 0;
+  int must_embed = 0, force = 0;
   
   progname = "grub-setup";
 
   /* Check for options.  */
   while (1)
     {
-      int c = getopt_long (argc, argv, "b:c:d:m:r:hVv", options, 0);
+      int c = getopt_long (argc, argv, "b:c:d:m:r:hVvf", options, 0);
 
       if (c == -1)
 	break;
@@ -615,6 +632,10 @@ main (int argc, char *argv[])
 	    root_dev = xstrdup (optarg);
 	    break;
 	    
+	  case 'f':
+	    force = 1;
+	    break;
+
 	  case 'h':
 	    usage (0);
 	    break;
@@ -716,7 +737,7 @@ main (int argc, char *argv[])
 	  setup (dir ? : DEFAULT_DIRECTORY,
 		 boot_file ? : DEFAULT_BOOT_FILE,
 		 core_file ? : DEFAULT_CORE_FILE,
-		 root_dev, grub_util_get_grub_dev (devicelist[i]), 1);
+		 root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force);
 	}
     }
   else
@@ -725,7 +746,7 @@ main (int argc, char *argv[])
     setup (dir ? : DEFAULT_DIRECTORY,
 	   boot_file ? : DEFAULT_BOOT_FILE,
 	   core_file ? : DEFAULT_CORE_FILE,
-	   root_dev, dest_dev, must_embed);
+	   root_dev, dest_dev, must_embed, force);
 
   /* Free resources.  */
   grub_fini_all ();
Index: util/misc.c
===================================================================
--- util/misc.c	(revision 2155)
+++ util/misc.c	(working copy)
@@ -52,6 +52,19 @@ char *progname = 0;
 int verbosity = 0;
 
 void
+grub_util_warn (const char *fmt, ...)
+{
+  va_list ap;
+  
+  fprintf (stderr, "%s: warn: ", progname);
+  va_start (ap, fmt);
+  vfprintf (stderr, fmt, ap);
+  va_end (ap);
+  fputc ('\n', stderr);
+  fflush (stderr);
+}
+
+void
 grub_util_info (const char *fmt, ...)
 {
   if (verbosity > 0)

Re: [PATCH] improve error messages in grub-setup

[Pavel Roskin]

On Sun, 2009-05-03 at 18:55 +0200, Robert Millan wrote:

> Agreed.  It's no harm to add --force for users who are stuck in such layouts.
> 
> Here's a new patch.

I don't see the "force" variable affecting anything in setup() other
than output.  grub_util_warn() doesn't set any flag that there have been
warnings.

-- 
Regards,
Pavel Roskin

Re: [PATCH] improve error messages in grub-setup

[Robert Millan]

On Sun, May 03, 2009 at 01:04:33PM -0400, Pavel Roskin wrote:
> On Sun, 2009-05-03 at 18:55 +0200, Robert Millan wrote:
> 
> > Agreed.  It's no harm to add --force for users who are stuck in such layouts.
> > 
> > Here's a new patch.
> 
> I don't see the "force" variable affecting anything in setup() other
> than output.

There is, see:

+  grub_util_warn ("Embedding is not possible.  GRUB can only be installed in this "
+                 "setup by using blocklists.  However, blocklists are UNRELIABLE and "
+                 "its use is discouraged.");
+  if (! force)
+    grub_util_error ("If you really want blocklists, use --force.");

The first is always displayed, the latter only when we don't have --force

> grub_util_warn() doesn't set any flag that there have been
> warnings.

That's an option, too.  But it would preclude the possibility of issuing
warnings for which we don't want to require --force (i.e. lesser evils).
I wonder if that would be a problem?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] improve error messages in grub-setup

[Pavel Roskin]

On Sun, 2009-05-03 at 22:54 +0200, Robert Millan wrote:
> On Sun, May 03, 2009 at 01:04:33PM -0400, Pavel Roskin wrote:
> > On Sun, 2009-05-03 at 18:55 +0200, Robert Millan wrote:
> > 
> > > Agreed.  It's no harm to add --force for users who are stuck in such layouts.
> > > 
> > > Here's a new patch.
> > 
> > I don't see the "force" variable affecting anything in setup() other
> > than output.
> 
> There is, see:
> 
> +  grub_util_warn ("Embedding is not possible.  GRUB can only be installed in this "
> +                 "setup by using blocklists.  However, blocklists are UNRELIABLE and "
> +                 "its use is discouraged.");
> +  if (! force)
> +    grub_util_error ("If you really want blocklists, use --force.");
> 
> The first is always displayed, the latter only when we don't have --force

Sorry, I missed the side effect of grub_util_error().  Never mind.

> > grub_util_warn() doesn't set any flag that there have been
> > warnings.
> 
> That's an option, too.  But it would preclude the possibility of issuing
> warnings for which we don't want to require --force (i.e. lesser evils).
> I wonder if that would be a problem?

We can decide it when we know what the lesser evils are.  I'm fine with
your patch.

-- 
Regards,
Pavel Roskin

Re: [PATCH] improve error messages in grub-setup

[Robert Millan]

On Sun, May 03, 2009 at 05:02:33PM -0400, Pavel Roskin wrote:
> > > grub_util_warn() doesn't set any flag that there have been
> > > warnings.
> > 
> > That's an option, too.  But it would preclude the possibility of issuing
> > warnings for which we don't want to require --force (i.e. lesser evils).
> > I wonder if that would be a problem?
> 
> We can decide it when we know what the lesser evils are.  I'm fine with
> your patch.

Alright then, I checked it in.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."