Re: grub-probe detects ext4 wronly as ext2

[Bean]

Hi,

First of all, grub-setup is conservative enough. It read the data
twice, one using host os, one using grub, and compare result. If the
data is corrupted, the comparison will fail and it will refuse to
install. This is an example of good practice,  which check the driver
by function, not by flags. The  flags are a little vague, for example,
if they change btree structure in the future, this would introduce
another flag, but we don't need to worry about it, because we don't
use it at all.

If we can ensure the boot partition can be accessed, then the problem
is solved. Yes, there may be other partition using ext4 that we can't
access, but remember that linux loader will check for signatures as
well, a corrupted kernel would not be loaded. So the difference is
merely the error message users sees, unknown filesystem or invalid
kernel.

And, grub WILL follow the evolution the extN, because it's the primary
boot loader for linux. The only reason we don't have ext4 support at
present is because it's not stable. If major distro starts to use it
as default, we would have to support it as well.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH] Drivemap module

[Javier Martín]

Just an updated version of the patch that adds support for device-like
names instead of raw BIOS disk numbers, i.e. this is now supported:
grub> drivemap (hd0) (hd1)
In addition to the already supported:
grub> drivemap (hd0) 0x81
The effect is the same: the second BIOS hard drive will map to (hd0)
through the installed int13h routine. The new syntax does not require
the target device name to exist (hd1 need not exist in my example), and
the parsing is very simple: it accepts names like (fdN) and (hdN) with
and without parenthesis, and with N ranging from 0 to 127, thus allowing
the full 0x00-0xFF range even though most BIOS-probing OSes don't bother
going any further than fd7 and hd7 respectively.

For newcomers, full info on the patch is available on the list archives
- it was proposed on June and its discussion deferred for "two or three
weeks" because the developers were busy.
Index: commands/i386/pc/drivemap.c
===
RCS file: commands/i386/pc/drivemap.c
diff -N commands/i386/pc/drivemap.c
--- /dev/null	1 Jan 1970 00:00:00 -
+++ commands/i386/pc/drivemap.c	2 Jul 2008 01:12:36 -
@@ -0,0 +1,391 @@
+/* drivemap.c - command to manage the BIOS drive mappings.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Uncomment the following line to enable debugging output */
+/* #define DRIVEMAP_DEBUG */
+
+#ifdef DRIVEMAP_DEBUG
+# define DBG_PRINTF(...) grub_printf(__VA_ARGS__)
+#else
+# define DBG_PRINTF(...)
+#endif
+
+static const struct grub_arg_option options[] = {
+  {"list", 'l', 0, "show the current mappings", 0, 0},
+  {"reset", 'r', 0, "reset all mappings to the default values", 0, 0},
+  {0, 0, 0, 0, 0, 0}
+};
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - start */
+
+/* realmode far ptr = 2 * 16b */
+extern grub_uint32_t EXPORT_VAR (grub_drivemap_int13_oldhandler);
+/* Size of the section to be copied */
+extern grub_uint16_t EXPORT_VAR (grub_drivemap_int13_size);
+
+/* NOT a typo - just need the symbol's address with &symbol */
+typedef void grub_symbol_t;
+extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_handler_base);
+extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_mapstart);
+
+void EXPORT_FUNC (grub_drivemap_int13_handler) (void);
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - end */
+
+static grub_preboot_hookid insthandler_hook = 0;
+
+typedef struct drivemap_node
+{
+  grub_uint8_t newdrive;
+  grub_uint8_t redirto;
+  struct drivemap_node *next;
+} drivemap_node_t;
+
+static drivemap_node_t *drivemap = 0;
+static grub_err_t install_int13_handler (void);
+
+static grub_err_t
+drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
+  /* Puts the specified mapping into the table, replacing an existing mapping
+   * for newdrive or adding a new one if required. */
+{
+  drivemap_node_t *mapping = 0, *search = drivemap;
+  while (search)
+{
+  if (search->newdrive == newdrive)
+{
+  mapping = search;
+  break;
+}
+  search = search->next;
+}
+
+  if (mapping)  /* There was a mapping already in place, modify it */
+mapping->redirto = redirto;
+  else  /* Create a new mapping and add it to the head of the list */
+{
+  mapping = grub_malloc (sizeof (drivemap_node_t));
+  if (!mapping)
+return grub_error (GRUB_ERR_OUT_OF_MEMORY,
+   "cannot allocate map entry, not enough memory");
+  mapping->newdrive = newdrive;
+  mapping->redirto = redirto;
+  mapping->next = drivemap;
+  drivemap = mapping;
+}
+  return GRUB_ERR_NONE;
+}
+
+static void
+drivemap_remove (grub_uint8_t newdrive)
+  /* Removes the mapping for newdrive from the table. If there is no mapping,
+   * then this function behaves like a no-op on the map. */
+{
+  drivemap_node_t *mapping = 0, *search = drivemap, *previous = 0;
+  while (search)
+{
+  if (search->newdrive == newdrive)
+{
+  mapping = search;
+  break;
+}
+  previous = search;
+  search = search->next;
+}
+  if (mapping) /* Found */
+{
+  if (previous)
+previous->next = mapping->next;
+  else drivemap = mapping->next; /* Entry was

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

And, after writing all that, I forgot to append the patch. Sigh.

Index: fs/ext2.c
===
--- fs/ext2.c	(revisión: 1687)
+++ fs/ext2.c	(copia de trabajo)
@@ -50,6 +50,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Log2 size of ext2 block in 512 blocks.  */
 #define LOG2_EXT2_BLOCK_SIZE(data)			\
@@ -71,8 +72,42 @@
  ? EXT2_GOOD_OLD_INODE_SIZE \
  : grub_le_to_cpu16 (data->sblock.inode_size))
 
-#define EXT3_FEATURE_COMPAT_HAS_JOURNAL	0x0004
+/* Superblock filesystem feature flags (RW compatible) */
+#define EXT2_FEATURE_COMPAT_DIR_PREALLOC	0x0001
+#define EXT2_FEATURE_COMPAT_IMAGIC_INODES	0x0002
+#define EXT3_FEATURE_COMPAT_HAS_JOURNAL		0x0004
+#define EXT2_FEATURE_COMPAT_EXT_ATTR		0x0008
+#define EXT2_FEATURE_COMPAT_RESIZE_INODE	0x0010
+#define EXT2_FEATURE_COMPAT_DIR_INDEX		0x0020
+/* Superblock filesystem feature flags (RO compatible) */
+#define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER	0x0001
+#define EXT2_FEATURE_RO_COMPAT_LARGE_FILE	0x0002
+#define EXT2_FEATURE_RO_COMPAT_BTREE_DIR	0x0004
+#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
+#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
+#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
+/* Superblock filesystem feature flags (back-incompatible) */
+#define EXT2_FEATURE_INCOMPAT_COMPRESSION	0x0001
+#define EXT2_FEATURE_INCOMPAT_FILETYPE		0x0002
+#define EXT3_FEATURE_INCOMPAT_RECOVER		0x0004 /* Needs recovery */
+#define EXT3_FEATURE_INCOMPAT_JOURNAL_DEV	0x0008 /* Journal device */
+#define EXT2_FEATURE_INCOMPAT_META_BG		0x0010
+#define EXT4_FEATURE_INCOMPAT_EXTENTS		0x0040 /* extents support */
+#define EXT4_FEATURE_INCOMPAT_64BIT		0x0080
+#define EXT4_FEATURE_INCOMPAT_FLEX_BG		0x0200
 
+/* The set of back-incompatible features this driver DOES support. Add (OR)
+ * flags here as the related features are implemented into the driver */
+#define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE )
+/* The set of back-incompatible features this driver DOES NOT support but are
+ * ignored for some hackish reason. Flags here should be here _temporarily_!
+ * Remember that INCOMPAT_* features are so for a reason! */
+#define EXT2_DRIVER_IGNORED_INCOMPAT ( EXT3_FEATURE_INCOMPAT_RECOVER )
+
+/* Strings for the driver environment options */
+#define EXT2_DRIVER_ENVOPT_VAR "ext2_options"
+#define EXT2_DRIVER_ENVOPT_IGNOREINCOMPAT "ignore_incompatible"
+
 #define EXT3_JOURNAL_MAGIC_NUMBER	0xc03b3998U
 
 #define EXT3_JOURNAL_DESCRIPTOR_BLOCK	1
@@ -379,6 +414,7 @@
 grub_ext2_mount (grub_disk_t disk)
 {
   struct grub_ext2_data *data;
+  char* env_options = grub_env_get (EXT2_DRIVER_ENVOPT_VAR);
 
   data = grub_malloc (sizeof (struct grub_ext2_data));
   if (!data)
@@ -394,6 +430,14 @@
   if (grub_le_to_cpu16 (data->sblock.magic) != EXT2_MAGIC)
 goto fail;
   
+  /* Check the FS doesn't have feature bits enabled that we don't support.
+   * Ignore this check if the "ignore_incompatible" env. option is set */
+  if (0 != (grub_le_to_cpu32 (data->sblock.feature_incompat)
+& ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
+  && (0 == grub_strstr (env_options, EXT2_DRIVER_ENVOPT_IGNOREINCOMPAT)))
+goto fail;
+
+  
   data->disk = disk;
 
   data->diropen.data = data;
@@ -409,7 +453,8 @@
   return data;
 
  fail:
-  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
+  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem, or incompatible"
+   "features enabled (extents, etc.)");
   grub_free (data);
   return 0;
 }


signature.asc
Description: Esta parte del mensaje está firmada	digitalmente
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

El vie, 04-07-2008 a las 02:08 +0200, Robert Millan escribió:
> On Thu, Jul 03, 2008 at 07:07:55PM +0200, Javier Martín wrote:
> > > The question here is whether we should increase complexity with interfaces
> > > that don't provide any usefulness to the user, just to make it easier to
> > > do potentially dangerous things.  If a user understands the implications
> > > and really doesn't mind making her system bootability unreliable, then she
> > > should have no problem modifiing the source to do it.
> > The system bootability is already unreliable with the current code,
> 
> You mean because grub-probe is not conservative enough when determining 
> whether
> a filesystem can be accessed?  I think we agreed that this needs fixing.
Indeed, grub-probe should adopt the most conservative stance possible so
that real GRUB works on minimum assurances. However, I meant reliability
was compromised because grub-probe, as currently used by grub-install on
i386-pc, only checks the availability of the filesystem GRUB will boot
from. Thus, if GRUB is installed on a normal ext2/fat/whatever partition
and the OS kernel we will boot (or another file we want to use) is
stored in an ext4 partition, the most likely output of our current
scheme is an error from the ext2 driver (ranging from an error message
to a crash), because in its current best-effort policy it ignored the
"forbidden" signs in the superblock and tried to read the ext4 partition
as ext2.

> 
> > and
> > as I already explained, it will be unreliable as long as the filesystem
> > drivers use the "best-effort" politics you support.
> 
> If grub-probe says "you can't rely on this filesystem being readable by
> GRUB", then we're bound to that.  You don't make it any more reliable by
> adding user input into the equation.
As I said, there are setups in which (the current use of) grub-probe
does not add any reliability, because it just checks the GRUB boot
partition, not those with the kernels we might load (and I'm not
proposing instituting such checks).

> 
> When I talk about real GRUB being "best-effort", it's not reliability that
> is at stake here;  we already got that from grub-probe.  This is a minor
> problem IMHO;  it's just about having read access to as many filesystems
> as possible (other than the one we need for booting) vs not risking
> corruption during data read.  I seriously think we're wasting our time
> here.  Really, it's not worth it.  This whole discussion is not about how
> we engineer a new feature or solve a bug, but about how we deal with a
> _temporary_ limitation in our code.
It is both a temporary limitation and a permanent one: the limitation of
not being able to read ext4 extents and such is temporary indeed, but I
already said why we cannot just ignore the "incompat" field. Today is
ext4, tomorrow it will be something else. As I mentioned above, such
flags are huge warning signs telling us not to dare enter the partition
if we don't know what to do with them.

> 
> > I'm just proposing
> > that we change the default politics in the bootloader to "nearly
> > conservative" and then having an user-controllable option to enable
> > "best-effort" behaviour. I've had GRUB since at least 2004, and I've
> > done a few nasty things in its command line from the start; but only now
> > I feel comfortable enough to modify its source, so don't assume that a
> > knowledgeable/advanced user will be brave enough to modify the source of
> > his _bootloader_ just like that. I don't understand why you're so
> > adamant against sensible defaults AND the possibility of a rational,
> > free choice.
> 
> Because it's a gratuitous increase in complexity of the user interface.  It's
> a choice about a bug, which is due to be fixed, and for which an informed
> answer will always be the same.
As I said before, we can fix the "ext4 compatibility not implemented"
bug, we WILL eventually encounter _another_ "incompatible" feature, and
then another... We can't be in par with the evolution of extN
filesystems!

> 
> > > This looks like a more general problem.  I wonder if we should really 
> > > address
> > > it at the filesystem level.  e.g. the filesystem could be perfectly fine 
> > > but
> > > the files in it went corrupt;  if the data you're reading is corrupt, 
> > > does it
> > > matter at which point it was corrupted?
> > It does, if the data on disk is not corrupted and our filesystem driver
> > reads wrong data because in its "best-effort" trials to read the data it
> > forfeits the "specification"-mandated behaviour of aborting on finding
> > incompatible feature flags. We would be INTRODUCING errors in the data,
> > instead of just reading erroneous data because of a crash or something
> > like that.
> 
> When the problem is you've read corrupt data, and you have to handle this
> gracefuly, it doesn't make any difference that it was your fault because
> you violated a spec, it's still the same problem.
Fine, then. Let's _not_ worry about automatical

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

On Thu, Jul 03, 2008 at 07:07:55PM +0200, Javier Martín wrote:
> > The question here is whether we should increase complexity with interfaces
> > that don't provide any usefulness to the user, just to make it easier to
> > do potentially dangerous things.  If a user understands the implications
> > and really doesn't mind making her system bootability unreliable, then she
> > should have no problem modifiing the source to do it.
> The system bootability is already unreliable with the current code,

You mean because grub-probe is not conservative enough when determining whether
a filesystem can be accessed?  I think we agreed that this needs fixing.

> and
> as I already explained, it will be unreliable as long as the filesystem
> drivers use the "best-effort" politics you support.

If grub-probe says "you can't rely on this filesystem being readable by
GRUB", then we're bound to that.  You don't make it any more reliable by
adding user input into the equation.

However, you can avoid the problem by refusing to access that filesystem
(perhaps by disabling font loading, or whatever we needed from it).  It's
at that point you can rely on GRUB being able to boot.

When I talk about real GRUB being "best-effort", it's not reliability that
is at stake here;  we already got that from grub-probe.  This is a minor
problem IMHO;  it's just about having read access to as many filesystems
as possible (other than the one we need for booting) vs not risking
corruption during data read.  I seriously think we're wasting our time
here.  Really, it's not worth it.  This whole discussion is not about how
we engineer a new feature or solve a bug, but about how we deal with a
_temporary_ limitation in our code.

> I'm just proposing
> that we change the default politics in the bootloader to "nearly
> conservative" and then having an user-controllable option to enable
> "best-effort" behaviour. I've had GRUB since at least 2004, and I've
> done a few nasty things in its command line from the start; but only now
> I feel comfortable enough to modify its source, so don't assume that a
> knowledgeable/advanced user will be brave enough to modify the source of
> his _bootloader_ just like that. I don't understand why you're so
> adamant against sensible defaults AND the possibility of a rational,
> free choice.

Because it's a gratuitous increase in complexity of the user interface.  It's
a choice about a bug, which is due to be fixed, and for which an informed
answer will always be the same.

> > This looks like a more general problem.  I wonder if we should really 
> > address
> > it at the filesystem level.  e.g. the filesystem could be perfectly fine but
> > the files in it went corrupt;  if the data you're reading is corrupt, does 
> > it
> > matter at which point it was corrupted?
> It does, if the data on disk is not corrupted and our filesystem driver
> reads wrong data because in its "best-effort" trials to read the data it
> forfeits the "specification"-mandated behaviour of aborting on finding
> incompatible feature flags. We would be INTRODUCING errors in the data,
> instead of just reading erroneous data because of a crash or something
> like that.

When the problem is you've read corrupt data, and you have to handle this
gracefuly, it doesn't make any difference that it was your fault because
you violated a spec, it's still the same problem.

> This is fine for update-grub, but the GRUB2 scripting language is
> complex enough that detecting every file access without missing any can
> get quite complex, and we'd need to embed even more code in the kernel
> (the hash verifier). I've implemented MD5 and SHA1 in various
> programming languages as a simple challenge and I can tell you that,
> while not the toughest thing in the world, it's not simple to get right
> and it means even less space in an already big core.img.

Why in the kernel?  It's essential that during install time you can rely on
/boot/grub being readable (otherwise I don't think we should support
install at all).

> I'm finding this discussion increasingly unproductive

Hey, at least we agree on something :-)

-- 
Robert Millan

 I know my rights; I want my phone call!
 What good is a phone call… if you are unable to speak?
(as seen on /.)


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] disk/fs_uuid.c

[Robert Millan]

On Fri, Jul 04, 2008 at 12:50:21AM +0200, Robert Millan wrote:
> 
> I _did_ extend the search command (before this patch).  The reason for
> providing this is basicaly that it makes grub.cfg more readable (and easier
> to understand), and can't be used outside the context of scripting (for
> example, to set your root/prefix to it, but that would require a patch I
> intend to put together soon).

Basicaly like this;  but it'd have to check that UUID is needed rather than
using it unconditionaly, of course.

-- 
Robert Millan

 I know my rights; I want my phone call!
 What good is a phone call… if you are unable to speak?
(as seen on /.)
Index: include/grub/i386/pc/kernel.h
===
--- include/grub/i386/pc/kernel.h	(revision 1686)
+++ include/grub/i386/pc/kernel.h	(working copy)
@@ -41,7 +41,7 @@
 #define GRUB_KERNEL_MACHINE_PREFIX		0x20
 
 /* End of the data section. */
-#define GRUB_KERNEL_MACHINE_DATA_END		0x50
+#define GRUB_KERNEL_MACHINE_DATA_END		0x60
 
 /* The size of the first region which won't be compressed.  */
 #define GRUB_KERNEL_MACHINE_RAW_SIZE		(GRUB_KERNEL_MACHINE_DATA_END + 0x450)
Index: util/i386/pc/grub-install.in
===
--- util/i386/pc/grub-install.in	(revision 1686)
+++ util/i386/pc/grub-install.in	(working copy)
@@ -232,13 +232,13 @@
 devabstraction_module=`$grub_probe --target=abstraction --device ${grub_device}`
 
 if [ "x${devabstraction_module}" = "x" ] ; then
-prefix_drive=
+prefix_drive=\(UUID=`$grub_probe --target=fs_uuid --device ${grub_device}`\)
 else
 prefix_drive=`$grub_probe --target=drive --device ${grub_device}`
 fi
 
 # The order in this list is critical.  Be careful when modifiing it.
-modules="$modules $fs_module $partmap_module biosdisk $devabstraction_module"
+modules="$modules $fs_module $partmap_module biosdisk $devabstraction_module fs_uuid"
 
 $grub_mkimage --output=${grubdir}/core.img \
 --prefix=${prefix_drive}`make_system_path_relative_to_its_root ${grubdir}`/ \
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] pc & gpt partmap iterators don't abort when their hook requests it

[Robert Millan]

On Thu, Jul 03, 2008 at 08:31:05PM +0200, Marco Gerards wrote:
> Robert Millan <[EMAIL PROTECTED]> writes:
> 
> > On Tue, Jul 01, 2008 at 03:25:32PM +0200, Robert Millan wrote:
> >> 
> >> See ChangeLog for description.  I'd really like to receive some review on
> >> this one, since the code it touches is so fragile (although I tested it on 
> >> a
> >> typical setup and it works).
> >
> > Tough luck.  Inmediately after this I noticed it breaks grub-setup (I 
> > tested it
> > by loading core.img directly).
> >
> > I found a few other callers that relied on the buggy behaviour.  Here's a 
> > new
> > patch.
> 
> Thanks for fixing this.  I had a quick look and it looks sane at first
> sight.

I just committed it (after fixing a pair of mistakes with grub_errno handling).

> Did you have a look at the other modules as well?

Yes.  At first glance, they don't appear to be affected.  However, since I
can't debug them, I wouldn't want to mess with them anyway.

-- 
Robert Millan

 I know my rights; I want my phone call!
 What good is a phone call… if you are unable to speak?
(as seen on /.)


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Save/Load environment variable support

[Robert Millan]

On Fri, Jul 04, 2008 at 04:11:07AM +0800, Bean wrote:
> 
> BTW, I think update-grub should create a new grubenv if it doesn't
> exists, so that user won't see the file not found error.

Would be interesting if update-grub added a "savedefault" blob to each
menuentry, and loaded the default afterwards (unless instructed otherwise
by the user).

-- 
Robert Millan

 I know my rights; I want my phone call!
 What good is a phone call… if you are unable to speak?
(as seen on /.)


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] disk/fs_uuid.c

[Robert Millan]

On Thu, Jul 03, 2008 at 09:05:54PM +0200, Marco Gerards wrote:
> Robert Millan <[EMAIL PROTECTED]> writes:
> 
> > This disk driver allows access to partitions that contain a filesystem with
> > known UUID via "(UUID=xxx)" syntax.
> >
> > The iterator is unimplemented on purpose, because it makes the code smaller
> > and it isn't useful to waste time inspecting the same devices twice while
> > scanning for LVM / whatsoever.
> >
> > Also note that as it currently stands it is highly inefficient.  I can't
> > understand why;  new UUIDs are searched everytime on each open() call, but
> > this shouldn't be much of an issue since we have a disk cache in kernel to
> > make that efficient.  At least, it isn't e.g. for grub_fs_probe() which is
> > called multiple times.
> >
> > Even with this efficiency problem, since it doesn't interfere with anything
> > else I'm inclined to commit it, if nobody objects.
> 
> Why didn't you just extend the search command?  I think that would be
> favorable.

I _did_ extend the search command (before this patch).  The reason for
providing this is basicaly that it makes grub.cfg more readable (and easier
to understand), and can't be used outside the context of scripting (for
example, to set your root/prefix to it, but that would require a patch I
intend to put together soon).

-- 
Robert Millan

 I know my rights; I want my phone call!
 What good is a phone call… if you are unable to speak?
(as seen on /.)


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Save/Load environment variable support

[Bean]

On Fri, Jul 4, 2008 at 4:41 AM, Marco Gerards <[EMAIL PROTECTED]> wrote:
> Pavel Roskin <[EMAIL PROTECTED]> writes:
>
>> On Thu, 2008-07-03 at 20:04 +0200, Marco Gerards wrote:
>>
>>> Great!  Can you explain how it works?
>>
>> Very good question.  It's not "discoverable".  I could not find way to
>> figure out that /boot/grub/grubenv is the default without looking at the
>> code.
>
> Actually, I meant how it works technically.

Hi,

It first reads the first 8192 bytes into memory, then, set read_hook
and do it again. Inside the hook, it records the sector location. It
then uses the location list to read from disk, and compare the result.
If they match, we assume it's the right one.

There are some consideration about the environment file. First, it
must be at least 8192 bytes long, this is used to avoid tail packing
in some fs. Then, we have the signature "GeNv", followed by the length
of the block. The signature doesn't need to the at the beginning, but
it has to been within the first 8192 byte, and dword aligned. This is
used to simplify the search function, also reduce the change of a
mismatch. The environment block consists of name=value strings,
terminated by an empty string.

The environment block can be embedded in other files, like core.img.
Then we can use grub-editenv, or save_env from grub console, to set
variable which can be used to locate root at runtime, like uuid, etc.
But this is the subject of another patch.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Enable writing to ATA devices, fix several bugs

[Pavel Roskin]

On Thu, 2008-07-03 at 20:27 +0200, Marco Gerards wrote:

> The more patches I get, the better ;-)
> 
> > ChangeLog:
> >
> > * disk/ata.c (grub_ata_pio_write): Check status before writing,
> > like we do in grub_ata_pio_read().
> >
> > (grub_ata_readwrite): Always write individual sectors.  Fix the
> > sector count for the remainder.
> 
> Why?

Because we do it elsewhere.  I assume you forgot to convert the code for
writing, but you meant to do it:

r1335 | marco_g | 2007-11-03 08:25:19 -0400 (Sat, 03 Nov 2007) | 6 lines

2007-11-03  Marco Gerards  <[EMAIL PROTECTED]>

* disk/ata.c (grub_ata_readwrite): Call grub_ata_pio_read and
grub_ata_pio_write once for every single sector, instead of for
multiple sectors.

I guess it's safer.  We can explore some optimization, but first we
should make it reliable.

> > (grub_ata_write): Enable writing to ATA devices.  Correctly
> > report error for ATAPI devices.

> Great!  Did you test this?

Yes, I tested this part.  env_save didn't report any error originally,
so I introduced grub_error(), and env_save started reporting the error.
Then I enabled writing and tested it in qemu.

I cannot get the ata module to recognize the hard drive on my test
machine, so more work is needed to test it on the real hardware.

> If you can fix Roberts comment, it would be great!  Can you commit it
> afterwards?

Sure.

If you check Linux include/linux/ata.h, 1 is ATA_ERR, and we really need
ata_ok(), which checks multiple flags.  So it's clearly material for a
separate patch.

-- 
Regards,
Pavel Roskin


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Save/Load environment variable support

[Marco Gerards]

Pavel Roskin <[EMAIL PROTECTED]> writes:

> On Thu, 2008-07-03 at 20:04 +0200, Marco Gerards wrote:
>
>> Great!  Can you explain how it works?
>
> Very good question.  It's not "discoverable".  I could not find way to
> figure out that /boot/grub/grubenv is the default without looking at the
> code.

Actually, I meant how it works technically.

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] LZMA support in i386-pc kernel

[Javier Martín]

El vie, 04-07-2008 a las 03:59 +0800, Bean escribió:
> On Fri, Jul 4, 2008 at 3:37 AM, Isaac Dupree
> <[EMAIL PROTECTED]> wrote:
> > by the way, is LZMA decoding slow enough to be noticable on old machines,
> > every boot time? (say, more than 1 second to decode core.img?)
> 
> The code is highly optimized for size, speed may hurt a little bit.
> But anyway, today's machine should have no problem with it. Besides,
> the time spent to load font file or a true color splash image may well
> exceed it
Besides, on old machines there is also the possibility of using LZO - I
doubt someone would try to put a setup which required embedding
[biosdisk pc raid lvm ext2], which is what pushed core.img over the
limit, on a 80386.


signature.asc
Description: Esta parte del mensaje está firmada	digitalmente
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Save/Load environment variable support

[Bean]

On Fri, Jul 4, 2008 at 2:09 AM, Pavel Roskin <[EMAIL PROTECTED]> wrote:
> On Thu, 2008-07-03 at 20:04 +0200, Marco Gerards wrote:
>
>> Great!  Can you explain how it works?
>
> Very good question.  It's not "discoverable".  I could not find way to
> figure out that /boot/grub/grubenv is the default without looking at the
> code.
>
> load_env without arguments merely prints "error: file not found" without
> telling which file it needs.  Help texts are silent about the defaults.
> Many commands ignore extra arguments silently.  The whole code needs a
> serious cleanup with end users in mind.

load_env would load /boot/grub/grubenv with no input, you can also
overwrite the default with -f parameter.

BTW, I think update-grub should create a new grubenv if it doesn't
exists, so that user won't see the file not found error.

>
> We also have too many commands regarding environment.  There is even
> freebsd_loadenv, which should probably merged with load_env somehow.  Or
> maybe not.  Maybe all native environment files should be handled with
> one command, such as "env".

freebsd_loadenv is used to load the hint file, which is plain text. It
also add the FreeBSD prefix so that they won't conflict with native
variables of grub2.

>
> We also need mechanisms to implement "savedefault" functionality,
> perhaps with easy examples.  Maybe update-grub should use it.
>
> Anyway, to start using it, create the environment file: grub-editenv (by
> the way, grub-env would be a better name):
>
> grub-editenv /boot/grub/grubenv create
> grub-editenv /boot/grub/grubenv set foo=bar
>
> Now you can inspect it with "list_env", load it into environment with
> "load_env" and save variables into it with "save_env".

Yes, this is the basic idea.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] LZMA support in i386-pc kernel

[Bean]

On Fri, Jul 4, 2008 at 3:37 AM, Isaac Dupree
<[EMAIL PROTECTED]> wrote:
> by the way, is LZMA decoding slow enough to be noticable on old machines,
> every boot time? (say, more than 1 second to decode core.img?)

The code is highly optimized for size, speed may hurt a little bit.
But anyway, today's machine should have no problem with it. Besides,
the time spent to load font file or a true color splash image may well
exceed it

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] New x86_64 EFI patch

[Bean]

On Fri, Jul 4, 2008 at 2:11 AM, Marco Gerards <[EMAIL PROTECTED]> wrote:
> Hi,
>
> Bean <[EMAIL PROTECTED]> writes:
>
>> This new patch add the following function to the original x86_64 EFI patch:
>>
>> 1, Fix menu drawing problem
>> It maps the unicode char to EFI char so that the rectangle box is
>> showed properly
>
> Doesn't it break other systems?

It only changes term/efi/console.c, which is efi specific.

>
>> 2, Handle command line option for chainloader command
>>
>> 3, Add new command appleloader
>> The command is used to enable bios boot in apple's bootcamp, for example
>
> I am not sure if appleloader is a good word.  Can you explain how it
> works and what it actually does?  Perhaps something like "legacy"
> would be a better word, especially since it is not apple specific.

The code is apple specific. They use special pathname for the bios
switcher, we just chainloading it. The pathname is not even the same
among different models, we need to go through an array to find a
possible match.

>
> btw, you forgot the changelog entry.
>

Here it is:

2008-07-04  Bean  <[EMAIL PROTECTED]>

* conf/i386/efi.rmk (pkglib_MODULES): add pci.mod and lspci.mod.
(appleldr_mod_SOURCE): New variavle.
(appleldr_mod_CFLAGS): Likewise.
(appleldr_mod_LDFLAGS): Likewise.
(pci_mod_SOURCES): Likewise.
(pci_mod_CFLAGS): Likewise.
(pci_mod_LDFLAGS): Likewise.
(lspci_mod_SOURCES): Likewise.
(lspci_mod_CFLAGS): Likewise.
(lspci_mod_LDFLAGS): Likewise.

* conf/x86_64-efi.rmk: New file.

* disk/efi/efidisk.c (grub_efidisk_read): Wrap efi calls with efi_call_N
macro.
(grub_efidisk_write): Likewise.

* include/efi/api.h (efi_call_0): New macro.
(efi_call_1): Likewise.
(efi_call_2): Likewise.
(efi_call_3): Likewise.
(efi_call_4): Likewise.
(efi_call_5): Likewise.
(efi_call_6): Likewise.

* include/grub/efi/chainloader.h (grub_chainloader_cmd): Rename to
grub_rescue_cmd_chainloader.

* include/grub/efi/pe32.h (GRUB_PE32_MACHINE_X86_64): New macro.
(grub_pe32_optional_header): Change some fields based on i386 or
x86_64 platform.
(GRUB_PE32_PE32_MAGIC): Likewise.

* include/grub/efi/uga_draw.h: New file.

* include/grub/elf.h (STN_ABS): New constant.
(R_X86_64_NONE): Relocation constant for x86_64.
(R_X86_64_64): Likewise.
(R_X86_64_PC32): Likewise.
(R_X86_64_GOT32): Likewise.
(R_X86_64_PLT32): Likewise.
(R_X86_64_COPY): Likewise.
(R_X86_64_GLOB_DAT): Likewise.
(R_X86_64_JUMP_SLOT): Likewise.
(R_X86_64_RELATIVE): Likewise.
(R_X86_64_GOTPCREL): Likewise.
(R_X86_64_32): Likewise.
(R_X86_64_32S): Likewise.
(R_X86_64_16): Likewise.
(R_X86_64_PC16): Likewise.
(R_X86_64_8): Likewise.
(R_X86_64_PC8): Likewise.

* include/grub/i386/efi/pci.h: New file.

* include/grub/i386/linux.h (GRUB_LINUX_EFI_SIGNATURE):
Change it value based on platform.
(GRUB_LINUX_EFI_SIGNATURE_0204): New constant.
(GRUB_E820_RAM): Likewise.
(GRUB_E820_RESERVED): Likewise.
(GRUB_E820_ACPI): Likewise.
(GRUB_E820_NVS): Likewise.
(GRUB_E820_EXEC_CODE): Likewise.
(GRUB_E820_MAX_ENTRY): Likewise.
(grub_e820_mmap): New structure.
(linux_kernel_header): Change the efi field according to different
kernel version, also field from linux_kernel_header.

* include/grub/kernel.h (grub_module_info): Add padding for x86_64.

* include/grub/pci.h (GRUB_PCI_ADDR_SPACE_MASK): New constant.
(GRUB_PCI_ADDR_SPACE_MEMORY): Likewise.
(GRUB_PCI_ADDR_SPACE_IO): Likewise.
(GRUB_PCI_ADDR_MEM_TYPE_MASK): Likewise.
(GRUB_PCI_ADDR_MEM_TYPE_32): Likewise.
(GRUB_PCI_ADDR_MEM_TYPE_1M): Likewise.
(GRUB_PCI_ADDR_MEM_TYPE_64): Likewise.
(GRUB_PCI_ADDR_MEM_PREFETCH): Likewise.
(GRUB_PCI_ADDR_MEM_MASK): Likewise.
(GRUB_PCI_ADDR_IO_MASK): Likewise.

* include/grub/x86_64/efi/kernel.h: New file.

* include/grub/x86_64/efi/loader.h: Likewise.

* include/grub/x86_64/efi/machine.h: Likewise.

* include/grub/x86_64/efi/pci.h: Likewise.

* include/grub/x86_64/efi/time.h: Likewise.

* include/grub/x86_64/linux.h: Likewise.

* include/grub/x86_64/setjmp.h: Likewise.

* include/grub/x86_64/time.h: Likewise.

* include/grub/x86_64/types.h: Likewise.

* kern/dl.c (GRUB_CPU_SIZEOF_VOID_P): Changed to
 GRUB_TARGET_SIZEOF_VOID_P.

* kern/efi/efi.c (grub_efi_locate_protocol): Wrap efi calls.
(grub_efi_locate_handle): Likewise.
(grub_efi_open_protocol): Likewise.
(grub_efi_set_text_mode): Likewise.
(grub_efi_stall): Likewise.
(grub_exit): Like

Re: [PATCH] LZMA support in i386-pc kernel

[Isaac Dupree]

by the way, is LZMA decoding slow enough to be noticable on old 
machines, every boot time? (say, more than 1 second to decode core.img?)


-Isaac



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: Eliminating grub_size_t

[Pavel Roskin]

On Thu, 2008-07-03 at 22:07 +0300, Vesa Jääskeläinen wrote:

> And size_t is kinda connected to memory addresses. Do you agree :) ?

Yes.  However, size_t should hold the maximal structure size, and we can
limit it to 4 (or even 2) gigabytes.  You can think of it as of the size
of a contiguous chunk of memory.  Difference between pointers to
different chunks doesn't have to fit size_t or ptrdiff_t.

Let's see: size_t is used:

in sizeof - OK to limit
in malloc - OK to limit
in strlen - OK to limit
in memcpy - OK to limit
in file I/O - OK to limit

We should not be limiting file and partition sizes and memory addresses,
but it's OK to limit the size of memory that is used at once, including
reading from files.  That's the whole reason why off_t may be longer
than size_t.

-- 
Regards,
Pavel Roskin


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: Eliminating grub_size_t

[Vesa Jääskeläinen]

Pavel Roskin wrote:

On Thu, 2008-07-03 at 21:42 +0300, Vesa Jääskeläinen wrote:


I think I'll try to make grub_size_t 32-bit everywhere and see if it's
going to make any difference or help discover some issues.

Why? I would let it be optimal type for current memory bus width.


int is supposed to be the optimal type, and int is 32-bit on 64-bit
platforms we support.  As far as I know, the x86_64 machine code
actually defaults to 32-bit for all arguments except memory addresses.


And size_t is kinda connected to memory addresses. Do you agree :) ?


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: Eliminating grub_size_t

[Pavel Roskin]

On Thu, 2008-07-03 at 20:56 +0200, Marco Gerards wrote:
> Pavel Roskin <[EMAIL PROTECTED]> writes:

> > I think I'll try to make grub_size_t 32-bit everywhere and see if it's
> > going to make any difference or help discover some issues.
> 
> Please don't.  I'd rather stick to integers, such change will only
> slow down GRUB with no gain.

Integers are 32-bit on all platforms we support.  Moreover, long is
32-bit on all platforms we support except SPARC, which is basically on
life support (I didn't look at the x86_64 EFI changes yet).

Anyway, I'd like to stop this thread until I have specific code.
Otherwise, I have to explain what I mean instead of showing it.

-- 
Regards,
Pavel Roskin


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] disk/fs_uuid.c

[Marco Gerards]

Robert Millan <[EMAIL PROTECTED]> writes:

> This disk driver allows access to partitions that contain a filesystem with
> known UUID via "(UUID=xxx)" syntax.
>
> The iterator is unimplemented on purpose, because it makes the code smaller
> and it isn't useful to waste time inspecting the same devices twice while
> scanning for LVM / whatsoever.
>
> Also note that as it currently stands it is highly inefficient.  I can't
> understand why;  new UUIDs are searched everytime on each open() call, but
> this shouldn't be much of an issue since we have a disk cache in kernel to
> make that efficient.  At least, it isn't e.g. for grub_fs_probe() which is
> called multiple times.
>
> Even with this efficiency problem, since it doesn't interfere with anything
> else I'm inclined to commit it, if nobody objects.

Why didn't you just extend the search command?  I think that would be
favorable.

--
Marco




___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] LZMA support in i386-pc kernel

[Bean]

On Fri, Jul 4, 2008 at 2:24 AM, Marco Gerards <[EMAIL PROTECTED]> wrote:
> Hi,
>
> Bean <[EMAIL PROTECTED]> writes:
>
>> This is the new patch.
>
> Can you please include a changelog entry?

Hi,

Oh, this is the changelog

2008-07-04  Bean  <[EMAIL PROTECTED]>

* Makefile.in (enable_lzo): New rule.

* i386-pc.rmk (grub_mkimage_SOURCES): New test with enable_lzo.

* configure.ac (ENABLE_LZO): New option --enable-lzo.

* include/grub/i386/pc/kernel.h (GRUB_KERNEL_MACHINE_RAW_SIZE): Change
its value accordding to the compression algorithm used, lzo or lzma.

* util/i386/pc/grub-mkimage.c (compress_kernel): Use different 
compression
according to macro ENABLE_LZO.

* kern/i386/pc/startup.S (codestart): Likewise.

* kern/i386/pc/lzma_decode.S: New file.

* include/grub/lib/LzFind.h: Likewise.

* include/grub/lib/LzHash.h: Likewise.

* include/grub/lib/LzmaDec.h: Likewise.

* lib/LzFind.c: Likewise.

* lib/LzmaDec.c: Likewise.

* lib/LzmaEnc.c: Likewise.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: Eliminating grub_size_t

[Pavel Roskin]

On Thu, 2008-07-03 at 21:42 +0300, Vesa Jääskeläinen wrote:

> > I think I'll try to make grub_size_t 32-bit everywhere and see if it's
> > going to make any difference or help discover some issues.
> 
> Why? I would let it be optimal type for current memory bus width.

int is supposed to be the optimal type, and int is 32-bit on 64-bit
platforms we support.  As far as I know, the x86_64 machine code
actually defaults to 32-bit for all arguments except memory addresses.

-- 
Regards,
Pavel Roskin


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Loading windows in macbook

[Marco Gerards]

Bean <[EMAIL PROTECTED]> writes:

> On Fri, Jul 4, 2008 at 2:40 AM, Marco Gerards <[EMAIL PROTECTED]> wrote:
>> Hi,
>>
>> Bean <[EMAIL PROTECTED]> writes:
>>
>>> Oh, actually a20 of macbook can be disabled with fast a20 port 92.
>>> However, the current a20 code do the keyboard controller test before
>>> trying port 92, which cause it to hang. To fix it, I only need to
>>> adjust the order of tests.
>>
>> What is the order you propose?  I wouldn't mind such a fix since the
>> Intel Mac is quite popular, although changing this might break GRUB on
>> other systems...
>
> Hi,
>
> Currently, the order is
>
> bios
> keyboard controller
> fast a20 port
>
> The second test would hang macbook, so I suggest
> bios
> fast a20 port
> keyboard controller
>
> I don't know if it will break other system. The fast a20 port code is
> simple enough, it reads from port 92, modify and write it back, system
> that don't support it shouldn't be affected, unless they use port 92
> for other purpose.

This seems to be a sane order.  I would favor your solution to this
problem.  Please wait a while before you commit this, so Okuji can
comment on this in case he disagrees.

--
Marco




___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: Eliminating grub_size_t

[Marco Gerards]

Pavel Roskin <[EMAIL PROTECTED]> writes:

> On Thu, 2008-07-03 at 20:02 +0200, Marco Gerards wrote:
>> > I know what it is.  I believe int should be as good as size_t for most
>> > purposes is we are not working with very large structures or read
>> > gigabytes of data from files at once.
>> 
>> Perhaps, but it doesn't hurt either.  I think it is a good thing to
>> have a type such that it is clear what kind of variable is used.
>
> I mean, we can have the type, but make it 32-bit on all systems.
> Anyway, the warnings have been fixed.  In some cases, grub_size_t was
> used for offsets, which is wrong because we want to support large files
> on 32-bit systems.
>
> I think I'll try to make grub_size_t 32-bit everywhere and see if it's
> going to make any difference or help discover some issues.

Please don't.  I'd rather stick to integers, such change will only
slow down GRUB with no gain.

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Fix warning in fs/xfs.c

[Pavel Roskin]

On Thu, 2008-07-03 at 20:21 +0200, Marco Gerards wrote:
> Pavel Roskin <[EMAIL PROTECTED]> writes:
> 
> > ChangeLog:
> > * fs/xfs.c (struct grub_xfs_dir_header): Use names similar to
> > those in Linux XFS code.  Provide a way to access 64-bit parent
> > inode.
> > (grub_xfs_iterate_dir): Use the new names.  Avoid reading past
> > the end of struct grub_xfs_dir_header.
> 
> *please* do not look at Linux code or whatever *and* contribute to
>  GRUB.  It might cause copyright troubles I will have to deal with :-/

I just tried to make names similar without copying any code.  But it's a
useful reminder.

> I do not see the advantage of this patch.  Can you please explain why
> we need these name changes?

We were casting a pointer to a 32-bit integer to a pointer to a 64-bit
integer, which is bad, and gcc was emitting a warning about it.

Worse yet, the 64-bit value was "sticking" beyond the end the structure
we were using to describe the header.

i4 and i8 are generally used by Linux XFS code to describe 32-bit and
64-bit values if either can be used.  The "smallino" field was highly
misleading because it had to be negated.  It's the number of "big" (i8
or 64-bit) entries.  If it's 0, then the entries are "small".

So it was natural to call it "i8count".  And once it was "i8count", it
was natural to call the first value "count".

If you prefer another naming convention, let's rename the entries
according to it.  I was thinking having 2 32-bit integers "parent_hi"
and "parent_lo" or something like that.  Anyway, let's not use
"smallino" - "bigentries" would be better.

-- 
Regards,
Pavel Roskin


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC] High resolution time support using x86 TSC

[Marco Gerards]

Hi Colin,

Colin D Bennett <[EMAIL PROTECTED]> writes:

> I have implemented high resolution time support (through the
> new grub_get_time_ms() function) using the RDTSC instruction available
> on Pentium and higher x86 CPUs.  The TSC value is simply a 64-bit block
> cycle counter that is zeroed at bootup, so grub_main() calls
> grub_time_init(), which is defined by each platform.  If a platform
> links to kern/i386/tsc.c, then the grub_time_init() function from tsc.c
> is used, which calibrates the TSC rate and absolute zero reference using
> the RTC.

This is great!

> I also extracted the grub_millisleep_generic() function into
> kern/generic/millisleep.c and renamed it to grub_millisleep() so that
> platforms linking to it don't need to define a grub_millisleep() just
> to call grub_millisleep_generic() anymore.  Simply linking in
> kern/generic/millisleep.c will use the generic implementation.

Ok.

> I have only tested this patch on the i386-pc platform, so if anyone
> tests it on another platform and finds problems, please let me know and
> I can try to fix it.

Did someone else test it?  I can't...

> === modified file 'ChangeLog'
> --- ChangeLog 2008-06-21 14:21:03 +
> +++ ChangeLog 2008-06-23 14:40:22 +
> @@ -1,3 +1,87 @@
> +2008-06-22  Colin D Bennett <[EMAIL PROTECTED]>
> +
> + High resolution timer support.  Implemented for i386 CPU using TSC.
> + Extracted generic grub_millisleep() so it's linked in only as needed.

What if TSC is not available?

> + * conf/i386-efi.rmk: Added TSC high resolution time module, link with
> + generic grub_millisleep() function.
> +
> + * conf/i386-pc.rmk: Likewise.
> +
> + * conf/sparc64-ieee1275.rmk: Add kern/generic/millisleep.c and
> + kern/generic/get_time_ms.c to kernel, to use generic time functions.
> +
> + * conf/powerpc-ieee1275.rmk: Add kern/generic/millisleep.c to kernel,
> + to use generic grub_millisleep() function.
> +
> + * kern/generic/get_time_ms.c (grub_get_time_ms): New file.  Platform
> + independent implementation of grub_get_time_ms() using the RTC that
> + can be linked into a platform's kernel when it does not implement its
> + own specialized grub_get_time_ms() function.
> +
> + * kern/generic/millisleep.c (grub_millisleep): New file.  Extracted
> + from grub_millisleep_generic() in kern/misc.c and renamed.  Now it is
> + not included in the kernel when a platform defines a specialized
> + grub_millisleep() implementation.
> +
> + * include/grub/i386/tsc.h (grub_get_tsc): New file.  Inline function
> + grub_get_tsc() uses x86 RDTSC instruction (available on Pentium+ CPUs)
> + to read the counter value for the TSC.
> +
> + * kern/i386/tsc.c (grub_get_time_ms): x86 TSC support providing a high
> + resolution clock.
> + (grub_tsc_calibrate): Static function to calibrate the TSC using RTC.
> + (grub_time_init): Implemented to call the static grub_tsc_calibrate()
> + function to calibrate the TSC.
> +
> + * include/grub/kernel.h (grub_time_init): Add declaration for the
> + grub_time_init() platform specific time initialization function.  This
> + function should be implemented by all platforms.  If kern/i386/tsc.c
> + is linked in, it will provide grub_time_init().
> +
> + * include/grub/time.h (grub_get_time_ms): Added grub_get_time_ms()
> + function to return the current time in millseconds since the epoch.
> + This supports higher resolution time than grub_get_rtc() on some
> + platforms such as i386-pc, where the RTC has only about 1/18 s
> + precision but a higher precision timer such as the TSC is available.
> +
> + * kern/i386/efi/init.c (grub_millisleep): Don't define
> + grub_millisleep() -- it just called grub_millisleep_generic() but now
> + we just need to link with kern/generic/millisleep.c to use the generic
> + implementation.
> +
> + * kern/i386/pc/init.c (grub_millisleep): Likewise.
> +
> + * kern/ieee1275/init.c (grub_millisleep): Don't define
> + grub_millisleep() -- it just called grub_millisleep_generic() but now
> + we just need to link with kern/generic/millisleep.c to use the generic
> + implementation.

No need to mention how it has to be linked.  I just assume you made
this change already?

> + (grub_get_time_ms): Implement this required function.  Simply uses the
> + prior implementation of grub_get_rtc().
> + (grub_get_rtc): Now calls grub_get_time_ms(), which does the real
> + work.
> + (grub_time_init): Define this empty but required function. No further
> + initialization necessary.
> +
> + * kern/sparc64/ieee1275/init.c (grub_millisleep): Don't define
> + grub_millisleep(), just link with kern/generic/millisleep.c.
> + (grub_time_init): Define this required function. Does nothing since
> + the generic RTC-based functions are used.

No need to mention what a function does.  Only describe the cha

Re: [PATCH] Loading windows in macbook

[Bean]

On Fri, Jul 4, 2008 at 2:40 AM, Marco Gerards <[EMAIL PROTECTED]> wrote:
> Hi,
>
> Bean <[EMAIL PROTECTED]> writes:
>
>> Oh, actually a20 of macbook can be disabled with fast a20 port 92.
>> However, the current a20 code do the keyboard controller test before
>> trying port 92, which cause it to hang. To fix it, I only need to
>> adjust the order of tests.
>
> What is the order you propose?  I wouldn't mind such a fix since the
> Intel Mac is quite popular, although changing this might break GRUB on
> other systems...

Hi,

Currently, the order is

bios
keyboard controller
fast a20 port

The second test would hang macbook, so I suggest
bios
fast a20 port
keyboard controller

I don't know if it will break other system. The fast a20 port code is
simple enough, it reads from port 92, modify and write it back, system
that don't support it shouldn't be affected, unless they use port 92
for other purpose.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: Eliminating grub_size_t

[Vesa Jääskeläinen]

Pavel Roskin wrote:

On Thu, 2008-07-03 at 20:02 +0200, Marco Gerards wrote:

I know what it is.  I believe int should be as good as size_t for most
purposes is we are not working with very large structures or read
gigabytes of data from files at once.

Perhaps, but it doesn't hurt either.  I think it is a good thing to
have a type such that it is clear what kind of variable is used.



I think I'll try to make grub_size_t 32-bit everywhere and see if it's
going to make any difference or help discover some issues.


Why? I would let it be optimal type for current memory bus width.



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Loading windows in macbook

[Marco Gerards]

Hi,

Bean <[EMAIL PROTECTED]> writes:

> Oh, actually a20 of macbook can be disabled with fast a20 port 92.
> However, the current a20 code do the keyboard controller test before
> trying port 92, which cause it to hang. To fix it, I only need to
> adjust the order of tests.

What is the order you propose?  I wouldn't mind such a fix since the
Intel Mac is quite popular, although changing this might break GRUB on
other systems...

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: Eliminating grub_size_t

[Pavel Roskin]

On Thu, 2008-07-03 at 20:02 +0200, Marco Gerards wrote:
> > I know what it is.  I believe int should be as good as size_t for most
> > purposes is we are not working with very large structures or read
> > gigabytes of data from files at once.
> 
> Perhaps, but it doesn't hurt either.  I think it is a good thing to
> have a type such that it is clear what kind of variable is used.

I mean, we can have the type, but make it 32-bit on all systems.
Anyway, the warnings have been fixed.  In some cases, grub_size_t was
used for offsets, which is wrong because we want to support large files
on 32-bit systems.

I think I'll try to make grub_size_t 32-bit everywhere and see if it's
going to make any difference or help discover some issues.

-- 
Regards,
Pavel Roskin


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] pc & gpt partmap iterators don't abort when their hook requests it

[Marco Gerards]

Robert Millan <[EMAIL PROTECTED]> writes:

> On Tue, Jul 01, 2008 at 03:25:32PM +0200, Robert Millan wrote:
>> 
>> See ChangeLog for description.  I'd really like to receive some review on
>> this one, since the code it touches is so fragile (although I tested it on a
>> typical setup and it works).
>
> Tough luck.  Inmediately after this I noticed it breaks grub-setup (I tested 
> it
> by loading core.img directly).
>
> I found a few other callers that relied on the buggy behaviour.  Here's a new
> patch.

Thanks for fixing this.  I had a quick look and it looks sane at first
sight.  Did you have a look at the other modules as well?

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Enable writing to ATA devices, fix several bugs

[Marco Gerards]

Pavel Roskin <[EMAIL PROTECTED]> writes:

> We have save_env now, so we can use the write capability.  This also
> fixes the last compiler warning in GRUB.

Great!

> Sorry, Marco, please ignore the previous message, as it didn't get to
> the list.

The more patches I get, the better ;-)

> ChangeLog:
>
>   * disk/ata.c (grub_ata_pio_write): Check status before writing,
>   like we do in grub_ata_pio_read().
>
>   (grub_ata_readwrite): Always write individual sectors.  Fix the
>   sector count for the remainder.

Why?

>   (grub_ata_write): Enable writing to ATA devices.  Correctly
>   report error for ATAPI devices.


Great!  Did you test this?

If you can fix Roberts comment, it would be great!  Can you commit it
afterwards?

--
Marco




___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] LZMA support in i386-pc kernel

[Marco Gerards]

Hi,

Bean <[EMAIL PROTECTED]> writes:

> This is the new patch.

Can you please include a changelog entry?

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Fix warning in fs/xfs.c

[Marco Gerards]

Pavel Roskin <[EMAIL PROTECTED]> writes:

> ChangeLog:
>   * fs/xfs.c (struct grub_xfs_dir_header): Use names similar to
>   those in Linux XFS code.  Provide a way to access 64-bit parent
>   inode.
>   (grub_xfs_iterate_dir): Use the new names.  Avoid reading past
>   the end of struct grub_xfs_dir_header.

*please* do not look at Linux code or whatever *and* contribute to
 GRUB.  It might cause copyright troubles I will have to deal with :-/

I do not see the advantage of this patch.  Can you please explain why
we need these name changes?

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Save/Load environment variable support

[Pavel Roskin]

On Thu, 2008-07-03 at 20:04 +0200, Marco Gerards wrote:

> Great!  Can you explain how it works?

Very good question.  It's not "discoverable".  I could not find way to
figure out that /boot/grub/grubenv is the default without looking at the
code.

load_env without arguments merely prints "error: file not found" without
telling which file it needs.  Help texts are silent about the defaults.
Many commands ignore extra arguments silently.  The whole code needs a
serious cleanup with end users in mind.

We also have too many commands regarding environment.  There is even
freebsd_loadenv, which should probably merged with load_env somehow.  Or
maybe not.  Maybe all native environment files should be handled with
one command, such as "env".

We also need mechanisms to implement "savedefault" functionality,
perhaps with easy examples.  Maybe update-grub should use it.

Anyway, to start using it, create the environment file: grub-editenv (by
the way, grub-env would be a better name):

grub-editenv /boot/grub/grubenv create
grub-editenv /boot/grub/grubenv set foo=bar

Now you can inspect it with "list_env", load it into environment with
"load_env" and save variables into it with "save_env".

-- 
Regards,
Pavel Roskin


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] New x86_64 EFI patch

[Marco Gerards]

Hi,

Bean <[EMAIL PROTECTED]> writes:

> This new patch add the following function to the original x86_64 EFI patch:
>
> 1, Fix menu drawing problem
> It maps the unicode char to EFI char so that the rectangle box is
> showed properly

Doesn't it break other systems?

> 2, Handle command line option for chainloader command
>
> 3, Add new command appleloader
> The command is used to enable bios boot in apple's bootcamp, for example

I am not sure if appleloader is a good word.  Can you explain how it
works and what it actually does?  Perhaps something like "legacy"
would be a better word, especially since it is not apple specific.

btw, you forgot the changelog entry.

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] add a counter in grub_dprintf

[Marco Gerards]

Robert Millan <[EMAIL PROTECTED]> writes:

> How about adding a counter to grub_dprintf to make it easy to instrument
> GRUB and find which are the bottlenecks in boot time?
>
> Sidenote: perhaps it'd be a good idea to conditionalize all grub_dprintf
> calls with #ifdef DEBUG to obtain a smaller core.img.  It's not hard to
> ask a user to rebuild if dprintf is needed, and we can find non-ugly ways
> to do this without massive #ifdefs all over the code.

How about a configure time switch to *disable* dprintf.  I prefer to
have it enabled by default.  In that case distributions can create two
packages.  One for normal use and one for debugging.

--
Marco




___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Save/Load environment variable support

[Marco Gerards]

Bean <[EMAIL PROTECTED]> writes:

> On Tue, Jul 1, 2008 at 11:54 PM, Robert Millan <[EMAIL PROTECTED]> wrote:
>> On Mon, Jun 30, 2008 at 09:06:28PM +0800, Bean wrote:
>>> Hi,
>>>
>>> This is the new patch, some changes:
>>>
>>> 1, envblk.h, remove GRUB_ENVBLK_RDIR and the like, they're not needed
>>> in this patch.
>>> 2, util/envblk.c, use grub_* function for string manipulation.
>>> 3, commands/loadenv.c, use grub_disk_read/grub_disk_write to
>>> read/write disk, the problem of lower level api is that they don't
>>> update the cache.
>>>
>>> If there is no problem, I'd like to commit this soon.
>>
>> Fine with me!
>
> Committed.

Great!  Can you explain how it works?

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: Eliminating grub_size_t

[Marco Gerards]

Pavel Roskin <[EMAIL PROTECTED]> writes:

> On Wed, 2008-07-02 at 20:46 +0300, Vesa Jääskeläinen wrote:
>
>> If reiserfs is using it in wrong place, fix the reiserfs. If you are 
>> reading some file system variable, then you should use grub_uintN_t to 
>> specify storage size in bits.
>
> OK, I'll have another look at the code.
>
>> size_t is usually used as common index or offset (or size) to some 
>> buffer. size_t is returned by sizeof(). It is meant to be optimal size 
>> for platform. Eg. on 64bit memory bus it is 64bit and on 32bit memory 
>> bus it is 32bit. What grub is doing here is just defining yet another 
>> type for the same thing.
>> 
>> Google for size_t if you want to find out more about it.
>
> I know what it is.  I believe int should be as good as size_t for most
> purposes is we are not working with very large structures or read
> gigabytes of data from files at once.

Perhaps, but it doesn't hurt either.  I think it is a good thing to
have a type such that it is clear what kind of variable is used.

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: grub-probe detects ext4 wronly as ext2

[Javier Martín]

El jue, 03-07-2008 a las 16:02 +0200, Robert Millan escribió:
> On Wed, Jul 02, 2008 at 09:32:40PM +0200, Javier Martín wrote:
> > El mié, 02-07-2008 a las 16:22 +0200, Robert Millan escribió:
> > > On Wed, Jul 02, 2008 at 01:28:47AM +0200, Javier Martín wrote:
> > > > > A --ignore-incompatible flag doesn't sound like a nice thing to do;  
> > > > > it means
> > > > > we're passing our own problem to the user instead of solving it.
> > > > We don't have any "problem" to pass to users: ext4 is not supported
> > > 
> > > We don't have an urge to support ext4, but that doesn't mean we shouldn't
> > > consider it a problem.
> > > 
> > > I think adding an interface for the user to choose in which way to deal 
> > > with
> > > our limitations is a nasty thing.  I strongly object to that.
> > May I ask why? Is it thus better to impose our own way of dealing with
> > our limitations, unchangeable and possibly wrong in some instances (see
> > below for a possible scenario)? Sensible defaults are good, but choice
> > is one of the bases of freedom.
> 
> We're never in a position in which we can impose things, because GRUB is free
> software to begin with, and anyone can modify it to better suit their needs.
GRUB is bundled with many Linux distros, and while it can be substituted
for others, being the "default choice" should entail a bit of
responsibility. Please, don't be like M$ with their "oh, if the users
don't like us, they can always switch (after overriding lock-in)".

> The question here is whether we should increase complexity with interfaces
> that don't provide any usefulness to the user, just to make it easier to
> do potentially dangerous things.  If a user understands the implications
> and really doesn't mind making her system bootability unreliable, then she
> should have no problem modifiing the source to do it.
The system bootability is already unreliable with the current code, and
as I already explained, it will be unreliable as long as the filesystem
drivers use the "best-effort" politics you support. I'm just proposing
that we change the default politics in the bootloader to "nearly
conservative" and then having an user-controllable option to enable
"best-effort" behaviour. I've had GRUB since at least 2004, and I've
done a few nasty things in its command line from the start; but only now
I feel comfortable enough to modify its source, so don't assume that a
knowledgeable/advanced user will be brave enough to modify the source of
his _bootloader_ just like that. I don't understand why you're so
adamant against sensible defaults AND the possibility of a rational,
free choice.

> > but also ignoring it and reading
> > wrong data to memory.
> 
> This looks like a more general problem.  I wonder if we should really address
> it at the filesystem level.  e.g. the filesystem could be perfectly fine but
> the files in it went corrupt;  if the data you're reading is corrupt, does it
> matter at which point it was corrupted?
It does, if the data on disk is not corrupted and our filesystem driver
reads wrong data because in its "best-effort" trials to read the data it
forfeits the "specification"-mandated behaviour of aborting on finding
incompatible feature flags. We would be INTRODUCING errors in the data,
instead of just reading erroneous data because of a crash or something
like that.

> A more elegant solution (also may be interesting for security at some point)
> would be for update-grub to hash each file it generates access commands for
> and embed the sum in grub.cfg as a check parameter, like
> 
>   if verify_hash /file x ; then
> do_something_with_file /file
>   fi
This is fine for update-grub, but the GRUB2 scripting language is
complex enough that detecting every file access without missing any can
get quite complex, and we'd need to embed even more code in the kernel
(the hash verifier). I've implemented MD5 and SHA1 in various
programming languages as a simple challenge and I can tell you that,
while not the toughest thing in the world, it's not simple to get right
and it means even less space in an already big core.img.


> So, if we take for granted those two things:
> 
>   - That GRUB should never crash no matter what you feed to it.
>   - That update-grub instructs GRUB to verify file consistency via hashing.
> 
> does this address all of your concerns?
It would, on a perfect world, but making all the FS driver routines
tough enough to provably ensure that they will never crash no matter
what is fed into them will probably enlarge the code size too much. WRT
the proposed hashing, it does not take into account the use of the GRUB
command line and, as I already mentioned, can also fail.

I'm finding this discussion increasingly unproductive because it's
drifting over an ideological issue (whether or not switch the reading
policy to more conservative and give users an override over it), so I
will not push the issue much further. I'll probably send in a new
version of the patch with

Re: grub-probe detects ext4 wronly as ext2

[Isaac Dupree]

Robert Millan wrote:

A more elegant solution (also may be interesting for security at some point)
would be for update-grub to hash each file it generates access commands for
and embed the sum in grub.cfg as a check parameter, like

  if verify_hash /file x ; then
do_something_with_file /file
  fi

So, if we take for granted those two things:

  - That GRUB should never crash no matter what you feed to it.
  - That update-grub instructs GRUB to verify file consistency via hashing.


also?,
- That whenever someone wants to boot a new kernel (or whatever), 
they re-run update-grub.  Which definitely doesn't apply if they're 
interactively poking around with the GRUB commandline.  But it could be 
a safety check for some cases.


Would it ever make sense to *ask* the user whether to proceed, if the 
file is different? (they might have changed the file deliberately!) 
But, with that code you mentioned for grub.cfg, I suppose it can be 
adjusted to do that, if desired by whoever controls grub.cfg.


-Isaac


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Enable writing to ATA devices, fix several bugs

[Robert Millan]

On Wed, Jul 02, 2008 at 08:17:16PM -0400, Pavel Roskin wrote:
> +  if (grub_ata_regget (dev, GRUB_ATA_REG_STATUS) & 1)
> +return grub_ata_regget (dev, GRUB_ATA_REG_ERROR);

May I suggest a macro to describe this "1"?

-- 
Robert Millan

 I know my rights; I want my phone call!
 What good is a phone call… if you are unable to speak?
(as seen on /.)


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] LZMA support in i386-pc kernel

[Robert Millan]

On Wed, Jul 02, 2008 at 11:11:28PM +0800, Bean wrote:
> >
> > Lenny's most likely going to ship with lzma 4.43.  Is that good?
> 
> The c encoder first appears in 4.58 beta. Previous version of lzma
> only have cpp version.

That's too bad, I guess it'll have to be for lenny+1.

> >> +#define FIXED_PROPS
> >> [...]
> >> +#ifdef FIXED_PROPS
> >
> > What's this option for?  Some comment would be nice.
> 
> lzma have three properties that control its behavior, lc, lp and pb.
> We can use these values as variable or constant. The default value, 3,
> 0, 2 is nice in most case, we hardly need to change them. So I use it
> as constant to save some extra space.

Could you make a comment out of this? :-)

> >> +#if 0
> >> +
> >> +DbgOut:
> >
> > This is just for debugging, right?  Maybe "#ifdef DEBUG" or so would be
> > better, to make it clear.
> 
> Yes, this is used in debugging. Now it's working, it's not needed
> anymore. I think it's better to keep it disable like this. The kernel
> raw space is critical, if we happen to define DEBUG, the decoder code
> would expand and kernel.img would fail to compile any more.

Ok.

> >> +#ifndef ASM_FILE
> >> + xorl%eax, %eax
> >> +#endif
> >
> > Why?  I thought ASM_FILE always ought to be defined for asm files in GRUB.
> 
> This is also used in debugging. Previously, I link it with c program
> to check the decoder, which require to keep register like %esi, %edi,
> %ebx. But inside grub, there is no need to keep them. I use ASM_FILE
> to distinguish between these two conditions.

Ok.  If you intend to keep it, please add a note explaining why; it
looks a bit puzzling without it.

-- 
Robert Millan

 I know my rights; I want my phone call!
 What good is a phone call… if you are unable to speak?
(as seen on /.)


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: grub-probe detects ext4 wronly as ext2

[Robert Millan]

On Wed, Jul 02, 2008 at 09:32:40PM +0200, Javier Martín wrote:
> El mié, 02-07-2008 a las 16:22 +0200, Robert Millan escribió:
> > On Wed, Jul 02, 2008 at 01:28:47AM +0200, Javier Martín wrote:
> > > > A --ignore-incompatible flag doesn't sound like a nice thing to do;  it 
> > > > means
> > > > we're passing our own problem to the user instead of solving it.
> > > We don't have any "problem" to pass to users: ext4 is not supported
> > 
> > We don't have an urge to support ext4, but that doesn't mean we shouldn't
> > consider it a problem.
> > 
> > I think adding an interface for the user to choose in which way to deal with
> > our limitations is a nasty thing.  I strongly object to that.
> May I ask why? Is it thus better to impose our own way of dealing with
> our limitations, unchangeable and possibly wrong in some instances (see
> below for a possible scenario)? Sensible defaults are good, but choice
> is one of the bases of freedom.

We're never in a position in which we can impose things, because GRUB is free
software to begin with, and anyone can modify it to better suit their needs.

The question here is whether we should increase complexity with interfaces
that don't provide any usefulness to the user, just to make it easier to
do potentially dangerous things.  If a user understands the implications
and really doesn't mind making her system bootability unreliable, then she
should have no problem modifiing the source to do it.

> The problem with INCOMPAT_* flags is that we cannot always know what
> they mean, and thus, a "best-effort" reader risks not just bumping into
> metadata it does not understand (and thus crashing or spitting thousands
> of errors if it's not robust enough),

This should never happen;  see the remark about corrupt filesystems in my
previous mail.

> but also ignoring it and reading
> wrong data to memory.

This looks like a more general problem.  I wonder if we should really address
it at the filesystem level.  e.g. the filesystem could be perfectly fine but
the files in it went corrupt;  if the data you're reading is corrupt, does it
matter at which point it was corrupted?

A more elegant solution (also may be interesting for security at some point)
would be for update-grub to hash each file it generates access commands for
and embed the sum in grub.cfg as a check parameter, like

  if verify_hash /file x ; then
do_something_with_file /file
  fi

So, if we take for granted those two things:

  - That GRUB should never crash no matter what you feed to it.
  - That update-grub instructs GRUB to verify file consistency via hashing.

does this address all of your concerns?

-- 
Robert Millan

 I know my rights; I want my phone call!
 What good is a phone call… if you are unable to speak?
(as seen on /.)


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel