[PATCH] fix usability problem in FreeBSD loader

[PATCH] fix usability problem in FreeBSD loader

[Robert Millan]

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


There's a minor usability problem with FreeBSD loader.  E.g. if user runs
freebsd_module first, the error message is confusing.

-- 
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: freebsd.diff --]
[-- Type: text/x-diff, Size: 1805 bytes --]

2009-08-01  Robert Millan  <rmh.grub@aybabtu.com>

	* loader/i386/bsd.c (grub_cmd_freebsd_loadenv, grub_cmd_freebsd_module)
	(grub_cmd_freebsd_module_elf): Abort with "You need to load the
	kernel first." when `kernel_type' is set to KERNEL_TYPE_NONE.

Index: loader/i386/bsd.c
===================================================================
--- loader/i386/bsd.c	(revision 2461)
+++ loader/i386/bsd.c	(working copy)
@@ -47,7 +47,7 @@
 
 #define MOD_BUF_ALLOC_UNIT	4096
 
-static int kernel_type;
+static int kernel_type; /* 0 == KERNEL_TYPE_NONE */
 static grub_dl_t my_mod;
 static grub_addr_t entry, entry_hi, kern_start, kern_end;
 static grub_uint32_t bootflags;
@@ -914,6 +914,10 @@ grub_cmd_freebsd_loadenv (grub_command_t
   char *buf = 0, *curr, *next;
   int len;
 
+  if (kernel_type == KERNEL_TYPE_NONE)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       "You need to load the kernel first.");
+
   if (kernel_type != KERNEL_TYPE_FREEBSD)
     return grub_error (GRUB_ERR_BAD_ARGUMENT,
 		       "only freebsd support environment");
@@ -1004,6 +1008,10 @@ grub_cmd_freebsd_module (grub_command_t 
   char **modargv;
   char *type;
 
+  if (kernel_type == KERNEL_TYPE_NONE)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       "You need to load the kernel first.");
+
   if (kernel_type != KERNEL_TYPE_FREEBSD)
     return grub_error (GRUB_ERR_BAD_ARGUMENT,
 		       "only freebsd support module");
@@ -1066,6 +1074,10 @@ grub_cmd_freebsd_module_elf (grub_comman
   grub_file_t file = 0;
   grub_err_t err;
 
+  if (kernel_type == KERNEL_TYPE_NONE)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       "You need to load the kernel first.");
+
   if (kernel_type != KERNEL_TYPE_FREEBSD)
     return grub_error (GRUB_ERR_BAD_ARGUMENT,
 		       "only freebsd support module");

Re: [PATCH] fix usability problem in FreeBSD loader

[Vladimir 'phcoder' Serbinenko]

On Sat, Aug 1, 2009 at 3:40 PM, Robert Millan<rmh@aybabtu.com> wrote:
>
> There's a minor usability problem with FreeBSD loader.  E.g. if user runs
> freebsd_module first, the error message is confusing.
>
-static int kernel_type;
+static int kernel_type; /* 0 == KERNEL_TYPE_NONE */
I would prefer either
static int kernel_type = KERNEL_TYPE_NONE;
And it would be better to use an enum here rather than defines.

+                      "You need to load the kernel first.");
+
   if (kernel_type != KERNEL_TYPE_FREEBSD)
     return grub_error (GRUB_ERR_BAD_ARGUMENT,
                       "only freebsd support module");
It seems that we have a bitrot in error message styles. We should
agree on one style and use it everywhere
Other than these two remarks patch is good.
> --
> 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."
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] fix usability problem in FreeBSD loader

[Robert Millan]

On Sat, Aug 01, 2009 at 03:49:15PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Sat, Aug 1, 2009 at 3:40 PM, Robert Millan<rmh@aybabtu.com> wrote:
> >
> > There's a minor usability problem with FreeBSD loader.  E.g. if user runs
> > freebsd_module first, the error message is confusing.
> >
> -static int kernel_type;
> +static int kernel_type; /* 0 == KERNEL_TYPE_NONE */
> I would prefer either
> static int kernel_type = KERNEL_TYPE_NONE;

Ok.

> And it would be better to use an enum here rather than defines.

I'll fix that too.

> +                      "You need to load the kernel first.");
> +
>    if (kernel_type != KERNEL_TYPE_FREEBSD)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT,
>                        "only freebsd support module");
> It seems that we have a bitrot in error message styles. We should
> agree on one style and use it everywhere

This error message, in particular, is copied from linux.c.  I think we
should capitalize the first letter of a phrase every time.  I'll fix
the other messages in this file too.

-- 
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] fix usability problem in FreeBSD loader

[Robert Millan]

Committed.

On Sat, Aug 01, 2009 at 04:28:52PM +0200, Robert Millan wrote:
> On Sat, Aug 01, 2009 at 03:49:15PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > On Sat, Aug 1, 2009 at 3:40 PM, Robert Millan<rmh@aybabtu.com> wrote:
> > >
> > > There's a minor usability problem with FreeBSD loader.  E.g. if user runs
> > > freebsd_module first, the error message is confusing.
> > >
> > -static int kernel_type;
> > +static int kernel_type; /* 0 == KERNEL_TYPE_NONE */
> > I would prefer either
> > static int kernel_type = KERNEL_TYPE_NONE;
> 
> Ok.
> 
> > And it would be better to use an enum here rather than defines.
> 
> I'll fix that too.
> 
> > +                      "You need to load the kernel first.");
> > +
> >    if (kernel_type != KERNEL_TYPE_FREEBSD)
> >      return grub_error (GRUB_ERR_BAD_ARGUMENT,
> >                        "only freebsd support module");
> > It seems that we have a bitrot in error message styles. We should
> > agree on one style and use it everywhere
> 
> This error message, in particular, is copied from linux.c.  I think we
> should capitalize the first letter of a phrase every time.  I'll fix
> the other messages in this file too.
> 
> -- 
> 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."
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
> 

-- 
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."