Re: [PATCH] Drivemap module

[Marco Gerards]

Hi Javier,

Javier Martín <address@hidden> writes:

> El lun, 04-08-2008 a las 22:51 +0200, Marco Gerards escribió:
>> Javier Martín <address@hidden> writes:
>> 
>> > After your latest replay, I "reevaluated" my stubbornness WRT some of
>> > your advices, and I've changed a few things:
>> >
>> > - Variables are now declared (and, if possible, initialized) before
>> > precondition checks, even simple ones. The install_int13_handler
>> > function has not been modified, however, since I find it a bit
>> > nonsensical to put bunch of declarations without an obvious meaning just
>> > after the "else" line:
>> >       grub_uint32_t *ivtslot;
>> >       grub_uint16_t *bpa_freekb;
>> >       grub_size_t total_size;
>> >       grub_uint16_t payload_sizekb;
>> >       grub_uint8_t *handler_base;
>> >       int13map_node_t *handler_map;
>> >       grub_uint32_t ivtentry;
>> 
>> Please understand me correctly.  Code just has to be written according
>> to our coding standards before it can and will be included.  We can
>> discuss things endlessly but we will simply stick to the GNU Coding
>> Standards as you might expect.
>> 
>> I hope you can appreciate that all code of GRUB has the same coding
>> style, if you like this style or not.
> Big sigh. Function modified to conform to your preciousss coding style.
> I might look a bit childish or arrogant saying this, but what is the
> point upholding a coding style that, no matter how consistent it is,
> hinders readability. With so much local variables, they are hard to keep
> track of no matter the coding style, but with the old ("mixed, heretic")
> style there was not need for a comment before each declaration: they
> were declared and used when needed instead of at the start of a block,
> because that function is inherently linear, not block-structured.

In your opinion it is not a good coding style.  I just avoid
discussions about it because such discussions just waste my time.
Even if you are able to convince me, GRUB is a GNU project and will
remain to use the GCS.


>> > - Only one declaration per line: even though C is a bit absurd in not
>> > recognizing T* as a first class type and instead thinking of * as a
>> > qualifier to the variable name; and even though my code does not incur
>> > into such ambiguities.
>> > - Comments moved as you required, reworded as needed
>> > - Extensive printf showing quasi-help in the "show" mode trimmed down to
>> > just the first sentence.
>> > - Internal helper functions now use the standard error handling, i.e.
>> > return grub_error (err, fmt, ...)
>> > - Comment about the strange "void" type instead of "void*" rephrased to
>> > be clearer
>> 
>> Thanks a lot!
>> 
>> > There is, however, one point in which I keep my objection: comparisons
>> > between a variable and a constant should be of the form CONSTANT ==
>> > variable and not in the reverse order, since an erroneous but quite
>> > possible change of == for = results in a compile-time error instead of a
>> > _extremely_ difficult to trace runtime bug. Such kind of bugs are quite
>> > excruciating to find in userspace applications within an IDE, so I can't
>> > even consider the pain to debug them in a bootloader.
>> 
>> I understand your concern, nevertheless, can you please change it?
> You understand my concern, but seemingly do not understand that in order
> to conform to the Holy Coding Style you are asking me to write code that
> can become buggy (and with a very hard to trace bug) with a simple
> deltion! (point: did you notice that last word _without_ a spelling
> checker? Now try to do so in a multithousand-line program).

Yes, I did notice it immediately.

The coding style is not holy in any way.  Everyone has its own coding
style.  You must understand I do not want to fully discuss it every
time someone sends in a patch, especially since it will not change
anyways.

> While tools like `svn diff' can help in these kind of problems, their
> utility is greatly diminished if the offending change is part of a
> multi-line change. Besides, sometimes checks like "if (a=b)", or more
> frequently "if (a=f())" are intentionally used in C, so the error might
> become even more difficult to spot and correct. I ask for a deep
> reflection on this issue: maybe I'm dead wrong and an arrogant brat in
> my attempt to tackle the Holy GNU Coding Standards, but I'd like to ask
> input from more people on this.

I will refuse patches with "if (a = f())", if that makes you sleep
better ;-)
 
>> > WRT accepting raw BIOS disk numbers, I agree with you in principle, but
>> > I'm keeping the functionality for now, since I don't quite like the
>> > "drivemap (hd0) (hd1)" syntax - which device is which?. I'd rather have
>> > something akin to "drivemap (hd0) (bios:hd1)", but I want to hear the
>> > opinions of people in this list.
>> 
>> I personally do not care a lot if BIOS disk numbers are used.
>> Although I do not see why it is useful.
>> 
>> As for the syntax, I would prefer something more GRUB2ish, like:
>> 
>> map --bios=(hd0) --os=(hd1)
> I agree. What about "grub" for the source drive instead of "os", with
> "bios" being the target drive? I mean:
>       drivemap --grub=(hd1) --bios=(hd0)
> Would map 0x80 in the BIOS to grub's (hd1) drive which will most likely
> be BIOS drive 0x81.

It will not change the functionality which GRUB is active.  But it
will change it for the OS that is loaded.  So I do not think --grub=
is a good idea because of this.  As for GRUB Legacy, it confused a lot
of people, so making people explicitly say that it is changed for the
OS, this confusion might go away :-)

> However, I prefer not to change it right now. Maybe when there are no
> other issues WRT to this patch we can set on to tackle this one.

So first I'll review this patch, then you will send in a new one?
 
>> Or so, perhaps the long argument names can be chosen in a more clever
>> way :-)
>> > The new version of the patch is attached, and here is my suggested CLog:
>> >
>> > 2008-08-XX  Javier Martin  <address@hidden>
>> 
>> (newline)
>> 
>> >         * commands/i386/pc/drivemap.c : New file.
>> >         * commands/i386/pc/drivemap_int13h.S : New file.
>> >         * conf/i386-pc.rmk (pkglib_MODULES) : Added drivemap.mod
>> >         (drivemap_mod_SOURCES) : New variable
>> >         (drivemap_mod_ASFLAGS) : Likewise
>> >         (drivemap_mod_CFLAGS) : Likewise
>> >         (drivemap_mod_LDFLAGS) : Likewise
>> >         * include/grub/loader.h (grub_loader_register_preboot) : New
>> >         function prototype. Register a new pre-boot handler
>> 
>> No need how the change is used or why it was added.
>> 
>> >         (grub_loader_unregister_preboot) : Likewise. Unregister handler
>> 
>> Same here.
>> 
>> >         (grub_preboot_hookid) : New typedef. Registered hook "handle"
>> 
>> Same here.
>> 
>> >         * kern/loader.c (grub_loader_register_preboot) : New function.
>> >         (grub_loader_unregister_preboot) : Likewise.
>> >         (preboot_hooks) : New variable. Linked list of preboot hooks
>> 
>> Same here.
>> 
>> >         (grub_loader_boot) : Call the list of preboot-hooks before the
>> >         actual loader
>> 
>> What's the `'?
> The what? o_O

I see some weird character in your text.  My font shows it as a block
before every `*'.

>> Please do not add a space before the ":" 
> Ok, everything corrected. New CL entry:
>
> 2008-08-XX  Javier Martin  <address@hidden>
>
>         * commands/i386/pc/drivemap.c: New file.
>         * commands/i386/pc/drivemap_int13h.S: New file.
>         * conf/i386-pc.rmk (pkglib_MODULES): Added drivemap.mod
>         (drivemap_mod_SOURCES): New variable
>         (drivemap_mod_ASFLAGS): Likewise
>         (drivemap_mod_CFLAGS): Likewise
>         (drivemap_mod_LDFLAGS): Likewise
>         * include/grub/loader.h (grub_loader_register_preboot): New
>         function prototype.
>         (grub_loader_unregister_preboot): Likewise.
>         (grub_preboot_hookid): New typedef.
>         * kern/loader.c (grub_loader_register_preboot): New function.
>         (grub_loader_unregister_preboot): Likewise.
>         (preboot_hooks): New variable.
>         (grub_loader_boot): Call the list of preboot-hooks before the
>         actual loader

Please add a `.' after "New variable" and "Likewise", same for the
third and the last sentence.  Sometimes you did it right :-).

>> Some comments can be found below.  Can you please fix the code mention
>> in the review and similar code?  I really want the code to be just
>> like any other GRUB code.  Please understand that we have to maintain
>> this in the future.  If everyone would use his own codingstyle, GRUB
>> would become unmaintainable.
>> 
>> > Index: commands/i386/pc/drivemap.c
>> > ===================================================================
>> > --- commands/i386/pc/drivemap.c    (revisión: 0)
>> > +++ commands/i386/pc/drivemap.c    (revisión: 0)
>> > @@ -0,0 +1,417 @@
>> > +/* 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 <http://www.gnu.org/licenses/>.
>> > + */
>> > +
>> > +#include <grub/normal.h>
>> > +#include <grub/dl.h>
>> > +#include <grub/mm.h>
>> > +#include <grub/misc.h>
>> > +#include <grub/disk.h>
>> > +#include <grub/loader.h>
>> > +#include <grub/machine/loader.h>
>> > +#include <grub/machine/biosdisk.h>
>> > +
>> > +#define MODNAME "drivemap"
>> > +
>> > +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}
>> > +};
>> > +
>> > +/* Syms/vars/funcs exported from drivemap_int13h.S - start.  */
>> > +
>> > +/* Realmode far ptr = 2 * 16b */
>> > +extern grub_uint32_t grub_drivemap_int13_oldhandler;
>> > +/* Size of the section to be copied */
>> > +extern grub_uint16_t grub_drivemap_int13_size;
>> > +
>> > +/* This type is used for imported assembly labels, takes no storage and 
>> > is only
>> > +   used to take the symbol address with &label.  Do NOT put void* here.  
>> > */
>> > +typedef void grub_symbol_t;
>> > +extern grub_symbol_t grub_drivemap_int13_handler_base;
>> > +extern grub_symbol_t grub_drivemap_int13_mapstart;
>> > +
>> > +void grub_drivemap_int13_handler (void);
>> 
>> The lines above belong in a header file.
> True, but they are used in a single file in the whole project and thus I
> see it pointless to extract an unneeded header, which would become one
> more SVN object to track. However, if you insist I will split the header
> file at once. In particular, I think the grub_symbol_t typedef should go
> into the "standard" GRUB headers somehow (but not in symbol.h, which is
> included from assembly files).

Please do so.

Other people might want to comment on the symbol change.  They will
most likely miss it if we keep discussing it here ;-).  Can you please
send that in as a separate change to give other the opportunity to
react on it?

>> > +/* Syms/vars/funcs exported from drivemap_int13h.S - end.  */
>> > +
>> > +
>> > +static grub_preboot_hookid insthandler_hook;
>> > +
>> > +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;
>> 
>> No need to set this to zero.
> Yes, you said so already, but I wanted to make the initial state very
> explicit to a future developer, since that variable is checked against
> zero in several points. Given that the added source size is four bytes
> and the added binary size is _zero_, is all the fuss really necessary?
> Notice that I changed the other variable in which you pointed out this
> issue, because it is not checked against zero anywhere.

Please do so anyways.

>> > +static grub_err_t install_int13_handler (void);
>> > +
>> > +/* Puts the specified mapping into the table, replacing an existing 
>> > mapping
>> > +   for newdrive or adding a new one if required.  */
>> > +static grub_err_t
>> > +drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
>> > +
>> 
>> Please do not add a newline here.
> Oops, sorry. I forgot to remove it when moving the comment

:-)

>> > +  drivemap_node_t *mapping = 0;
>> > +  drivemap_node_t *search = drivemap;
>> > +  while (search)
>> > +    {
>> > +      if (search->newdrive == newdrive)
>> > +        {
>> > +          mapping = search;
>> > +          break;
>> > +        }
>> > +      search = search->next;
>> > +    }
>> > +
>> > +  
>> > +  /* Check for pre-existing mappings to modify before creating a new one. 
>> >  */
>> > +  if (mapping)
>> > +    mapping->redirto = redirto;
>> > +  else 
>> > +    {
>> > +      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;
>> > +}
>> > +
>> > +/* Removes the mapping for newdrive from the table.  If there is no 
>> > mapping,
>> > +   then this function behaves like a no-op on the map.  */
>> > +static void
>> > +drivemap_remove (grub_uint8_t newdrive)
>> > +{
>> > +  drivemap_node_t *mapping = 0;
>> > +  drivemap_node_t *search = drivemap;
>> > +  drivemap_node_t *previous = 0;
>> > +  while (search)
>> > +    {
>> > +      if (search->newdrive == newdrive)
>> > +        {
>> > +          mapping = search;
>> > +          break;
>> > +        }
>> > +      previous = search;
>> > +      search = search->next;
>> > +    }
>> > +  if (mapping) /* Found.  */
>> 
>> You forgot one.
> Corrected. Sorry.
>> 
>> > +    {
>> > +      if (previous)
>> > +        previous->next = mapping->next;
>> > +      else /* Entry was head of list.  */
>> > +        drivemap = mapping->next;
>> > +      grub_free (mapping);
>> > +    }
>> > +}
>> > +
>> > +/* Given a device name, resolves its BIOS disk number and stores it in the
>> > +   passed location, which should only be trusted if ERR_NONE is returned. 
>> >  */
>> > +static grub_err_t
>> > +parse_biosdisk (const char *name, grub_uint8_t *disknum)
>> > +{
>> > +  grub_disk_t disk;
>> > +  if (!name || 0 == *name)
>> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");
>> > +  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
>> > +  if (*name == '(')
>> > +    name++;
>> > +  
>> > +  disk = grub_disk_open (name);
>> > +  if (!disk)
>> > +    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"", 
>> > name);
>> > +  else
>> > +    {
>> > +      const enum grub_disk_dev_id id = disk->dev->id;
>> > +      /* The following assignment is only sound if the device is indeed a
>> > +         biosdisk.  The caller must check the return value.  */
>> > +      if (disknum)
>> > +        *disknum = disk->id;
>> > +      grub_disk_close (disk);
>> > +      if (GRUB_DISK_DEVICE_BIOSDISK_ID == id)
>> > +        return GRUB_ERR_NONE;
>> > +      else return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS 
>> > disk", name);
>> > +    }
>> > +}
>> > +
>> > +/* Given a BIOS disk number, returns its GRUB device name if it exists.
>> > +   For nonexisting BIOS dnums, this function returns ERR_UNKNOWN_DEVICE.  
>> > */
>> 
>> This is GRUB_ERR_UNKNOWN_DEVICE
> I know, I consciously left the GRUB_ part out because 1) it would
> require the line to be split and 2) that prefix is all over the place.
> Corrected, however.
>> 
>> > +static grub_err_t
>> > +revparse_biosdisk(const grub_uint8_t dnum, const char **output)
>> > +{
>> > +  int found = 0;
>> > +  auto int find (const char *name);
>> > +  int find (const char *name)
>> > +  {
>> > +    const grub_disk_t disk = grub_disk_open (name);
>> > +    if (!disk)
>> > +      return 0;
>> > +    else
>> > +      {
>> > +        if (disk->id == dnum && GRUB_DISK_DEVICE_BIOSDISK_ID == 
>> > disk->dev->id)
>> > +          {
>> > +            found = 1;
>> > +            if (output)
>> > +              *output = name;
>> > +          }
>> > +        grub_disk_close (disk);
>> > +        return found;
>> > +      }
>> > +  }
>> > +
>> > +  grub_disk_dev_iterate (find);
>> > +  if (found)
>> > +    return GRUB_ERR_NONE;
>> > +  else return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %d not 
>> > found", dnum);
>> 
>> Please change this.
> Em... to what? What is the problem? Do you want me to reverse the
> comparison? i.e. if (!found) { return error; } else { return ok; }

The return is on the same line as the else.

>> > +/* Given a GRUB-like device name and a convenient location, stores the 
>> > related
>> > +   BIOS disk number.  Accepts devices like \((f|h)dN\), with 0 <= N < 
>> > 128.  */
>> > +static grub_err_t
>> > +tryparse_diskstring (const char *str, grub_uint8_t *output)
>> > +{
>> > +  if (!str || 0 == *str)
>> > +    goto fail;
>> > +  /* Skip opening paren in order to allow both (hd0) and hd0.  */
>> > +  if (*str == '(')
>> > +    str++;
>> > +  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
>> > +    {
>> > +      grub_uint8_t bios_num = (str[0] == 'h')? 0x80 : 0x00;
>> > +      grub_errno = GRUB_ERR_NONE;
>> > +      unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
>> > +      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
>> > +        /* N not a number or out of range */
>> > +        goto fail;
>> 
>> Can you put this between braces, now comment was added.
> Done.
>> 
>> > +      else
>> > +        {
>> > +          bios_num |= drivenum;
>> > +          if (output)
>> > +            *output = bios_num;
>> > +          return GRUB_ERR_NONE;
>> > +        }
>> > +    }
>> > +  else goto fail;
>> 
>> ...
> What's the problem here? The lack of braces? The goto (as used in the
> ext2 code)?

goto is on the same line as the else.

>> > +fail:
>> > +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" 
>> > invalid: must"
>> > +                     "be (f|h)dN, with 0 <= N < 128", str);
>> > +}
>> > +
>> > +static grub_err_t
>> > +grub_cmd_drivemap (struct grub_arg_list *state, int argc, char **args)
>> > +{
>> > +  if (state[0].set)
>> > +    {
>> > +      /* Show: list mappings.  */
>> > +      if (!drivemap)
>> > +        grub_printf ("No drives have been remapped");
>> > +      else
>> > +        {
>> > +          grub_printf ("Showing only remapped drives.\n");
>> > +          grub_printf ("Mapped\tGRUB\n");
>> > +          drivemap_node_t *curnode = drivemap;
>> > +          while (curnode)
>> > +            {
>> > +              const char *dname = 0;
>> > +              grub_err_t err = revparse_biosdisk (curnode->redirto, 
>> > &dname);
>> > +              if (err != GRUB_ERR_NONE)
>> > +                return grub_error (err, "invalid mapping: non-existent 
>> > disk"
>> > +                                        "or not managed by the BIOS");
>> > +              grub_printf("0x%02x\t%4s\n", curnode->newdrive, dname);
>> > +              curnode = curnode->next;
>> > +            }
>> > +        }
>> > +    }
>> > +  else if (state[1].set)
>> > +    {
>> > +      /* Reset: just delete all mappings, freeing their memory.  */
>> > +      drivemap_node_t *curnode = drivemap;
>> > +      drivemap_node_t *prevnode = 0;
>> > +      while (curnode)
>> > +        {
>> > +          prevnode = curnode;
>> > +          curnode = curnode->next;
>> > +          grub_free (prevnode);
>> > +        }
>> > +      drivemap = 0;
>> > +    }
>> > +  else
>> > +    {
>> > +      /* Neither flag: put mapping */
>> 
>> ".  */
> Done
>> 
>> > +      grub_uint8_t mapfrom = 0;
>> > +      grub_uint8_t mapto = 0xFF;
>> > +      grub_err_t err;
>> > +      
>> > +      if (argc != 2)
>> > +        return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments 
>> > required");
>> > +
>> > +      err = parse_biosdisk (args[0], &mapfrom);
>> > +      if (err != GRUB_ERR_NONE)
>> > +        return err;
>> > +
>> > +      err = tryparse_diskstring (args[1], &mapto);
>> > +      if (err != GRUB_ERR_NONE) /* Not a disk string. Maybe a raw num 
>> > then?  */
>> 
>> Please move this up.
> Done.
>> 
>> > +        {    
>> > +          grub_errno = GRUB_ERR_NONE;
>> > +          unsigned long num = grub_strtoul (args[1], 0, 0);
>> > +          if (grub_errno != GRUB_ERR_NONE || num > 0xFF)  /* Not a raw 
>> > num or too high.  */
>> > +            return grub_error (grub_errno,
>> > +                              "Target specifier must be of the form (fdN) 
>> > or "
>> > +                              "(hdN), with 0 <= N < 128; or a plain 
>> > dec/hex "
>> > +                              "number between 0 and 255");
>> > +          else mapto = (grub_uint8_t)num;
>> > +        }
>> > +      
>> > +      if (mapto == mapfrom)  /* Reset to default.  */
>> 
>> Same here.
> Done.
>> 
>> > +        {
>> > +          grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)", 
>> > args[0], mapfrom);
>> > +          drivemap_remove (mapfrom);
>> > +        }
>> > +      else  /* Map.  */
>> 
>> Please move the comment inside the braces below.
> Done, and reworded.
>> 
>> > +        {
>> > +          grub_dprintf (MODNAME, "Mapping %s (%02x) to %02x\n", args[0], 
>> > mapfrom, mapto);
>> > +          return drivemap_set ((grub_uint8_t)mapto, mapfrom);
>> > +        }
>> > +    }
>> > +
>> > +  return GRUB_ERR_NONE;
>> > +}
>> > +
>> > +typedef struct __attribute__ ((packed)) int13map_node
>> > +{
>> > +  grub_uint8_t disknum;
>> > +  grub_uint8_t mapto;
>> > +} int13map_node_t;
>> > +
>> > +/* The min amount of mem that must remain free after installing the 
>> > handler.
>> > +   32 KiB is just above 0x7C00-0x7E00, where the bootsector is loaded.  */
>> > +#define MIN_FREE_MEM_KB 32
>> > +#define INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) - 
>> > ((grub_uint8_t*)&grub_drivemap_int13_handler_base) )
>> > +#define INT13H_REBASE(x) ( (void*) (((grub_uint8_t*)handler_base) + (x)) )
>> > +#define INT13H_TONEWADDR(x) INT13H_REBASE( INT13H_OFFSET( x ) )
>> > +
>> > +/* Int13h handler installer - reserves conventional memory for the 
>> > handler,
>> > +   copies it over and sets the IVT entry for int13h.  
>> > +   This code rests on the assumption that GRUB does not activate any kind 
>> > of
>> > +   memory mapping apart from identity paging, since it accesses realmode
>> > +   structures by their absolute addresses, like the IVT at 0 or the BDA at
>> > +   0x400; and transforms a pmode pointer into a rmode seg:off far ptr.  */
>> > +static grub_err_t
>> > +install_int13_handler (void)
>> > +{
>> > +  grub_size_t entries = 0;
>> > +  drivemap_node_t *curentry = drivemap;
>> > +  while (curentry)  /* Count entries to prepare a contiguous map block.  
>> > */
>> 
>> ...
> Comment moved up.
>> 
>> > +    {
>> > +      entries++;
>> > +      curentry = curentry->next;
>> > +    }
>> > +  if (0 == entries)
>> 
>> I know this is what you prefer, but can you change this nevertheless?
> I refer to my objection near the top of the post.

I know you object, but did you change it?

>> > +      grub_dprintf (MODNAME, "No drives marked as remapped, installation 
>> > of"
>> > +                  "an int13h handler is not required.");
>> > +      return GRUB_ERR_NONE;  /* No need to install the int13h handler.  */
>> > +    }
>> > +  else
>> > +    {
>> > +      grub_dprintf (MODNAME, "Installing int13h handler...\n");
>> > +      grub_uint32_t *ivtslot = (grub_uint32_t*)0x0000004c;
>> > +      
>> > +      /* Save the pointer to the old int13h handler.  */
>> > +      grub_drivemap_int13_oldhandler = *ivtslot;
>> > +      grub_dprintf (MODNAME, "Old int13 handler at %04x:%04x\n",
>> > +                  (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff,
>> > +                  grub_drivemap_int13_oldhandler & 0x0ffff);
>> > +
>> > +      /* Reserve a section of conventional memory as "BIOS memory" for 
>> > handler:
>> > +         BDA offset 0x13 contains the top of such memory.  */
>> > +      grub_uint16_t *bpa_freekb = (grub_uint16_t*)0x00000413;
>> > +      grub_dprintf (MODNAME, "Top of conventional memory: %u KiB\n", 
>> > *bpa_freekb);
>> > +      grub_size_t total_size = grub_drivemap_int13_size
>> > +                            + (entries + 1) * sizeof(int13map_node_t);
>> > +      grub_uint16_t payload_sizekb = (total_size >> 10) +
>> > +                                    (((total_size % 1024) == 0) ? 0 : 1);
>> > +      if ((*bpa_freekb - payload_sizekb) < MIN_FREE_MEM_KB)
>> > +        return grub_error (GRUB_ERR_OUT_OF_MEMORY, "refusing to install"
>> > +                           "int13 handler, not enough free memory after");
>> > +      grub_dprintf (MODNAME, "Payload is %u b long, reserving %u Kb\n",
>> > +                                  total_size, payload_sizekb);
>> > +      *bpa_freekb -= payload_sizekb;
>> > +
>> > +      /* Copy int13h handler chunk to reserved area.  */
>> > +      grub_uint8_t *handler_base = (grub_uint8_t*)(*bpa_freekb << 10);
>> > +      grub_dprintf (MODNAME, "Copying int13 handler to: %p\n", 
>> > handler_base);
>> > +      grub_memcpy (handler_base, &grub_drivemap_int13_handler_base,
>> > +                   grub_drivemap_int13_size);
>> > +
>> > +      /* Copy the mappings to the reserved area.  */
>> > +      curentry = drivemap;
>> > +      grub_size_t i;
>> > +      int13map_node_t *handler_map = (int13map_node_t*)
>> > +                      INT13H_TONEWADDR (&grub_drivemap_int13_mapstart);
>> > +      grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n", 
>> > handler_map);
>> > +      for (i = 0; i < entries && curentry; i++, curentry = curentry->next)
>> > +        {
>> > +          handler_map[i].disknum = curentry->newdrive;
>> > +          handler_map[i].mapto = curentry->redirto;
>> > +          grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x\n", i,
>> > +                                          handler_map[i].disknum, 
>> > handler_map[i].mapto);
>> > +        }
>> > +      /* Signal end-of-map.  */
>> > +      handler_map[i].disknum = 0;
>> > +      handler_map[i].mapto = 0;
>> > +      grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x (end)\n", i,
>> > +                                  handler_map[i].disknum, 
>> > handler_map[i].mapto);
>> > +
>> > +      /* Install our function as the int13h handler in the IVT.  */
>> > +      grub_uint32_t ivtentry = ((grub_uint32_t)handler_base) << 12; /* 
>> > Segment address.  */
>> > +      ivtentry |= (grub_uint16_t) 
>> > INT13H_OFFSET(grub_drivemap_int13_handler);
>> > +      grub_dprintf (MODNAME, "New int13 handler IVT pointer: %04x:%04x\n",
>> > +                  (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);
>> > +      *ivtslot = ivtentry;
>> > +      
>> > +      return GRUB_ERR_NONE;
>> > +    }
>> > +}
>> > +
>> > +GRUB_MOD_INIT (drivemap)
>> > +{
>> > +  (void) mod;                     /* Stop warning.  */
>> > +  grub_register_command (MODNAME, grub_cmd_drivemap,
>> > +                         GRUB_COMMAND_FLAG_BOTH,
>> > +                                     MODNAME " -s | -r | (hdX) 
>> > newdrivenum",
>> > +                         "Manage the BIOS drive mappings", options);
>> > +  insthandler_hook = grub_loader_register_preboot 
>> > (&install_int13_handler, 1);
>> > +}
>> > +
>> > +GRUB_MOD_FINI (drivemap)
>> > +{
>> > +  grub_loader_unregister_preboot (insthandler_hook);
>> > +  insthandler_hook = 0;
>> > +  grub_unregister_command (MODNAME);
>> > +}
>> > +
>> > Index: commands/i386/pc/drivemap_int13h.S
>> > ===================================================================
>> > --- commands/i386/pc/drivemap_int13h.S     (revisión: 0)
>> > +++ commands/i386/pc/drivemap_int13h.S     (revisión: 0)
>> > @@ -0,0 +1,118 @@
>> > +/*
>> > + *  GRUB  --  GRand Unified Bootloader
>> > + *  Copyright (C) 1999,2000,2001,2002,2003,2005,2006,2007,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 <http://www.gnu.org/licenses/>.
>> > + */
>> > +
>> > +
>> > +/*
>> > + * Note: These functions defined in this file may be called from C.
>> > + *       Be careful of that you must not modify some registers. Quote
>> > + *       from gcc-2.95.2/gcc/config/i386/i386.h:
>> > +
>> > +   1 for registers not available across function calls.
>> > +   These must include the FIXED_REGISTERS and also any
>> > +   registers that can be used without being saved.
>> > +   The latter must include the registers where values are returned
>> > +   and the register where structure-value addresses are passed.
>> > +   Aside from that, you can include as many other registers as you like.
>> > +
>> > +  ax,dx,cx,bx,si,di,bp,sp,st,st1,st2,st3,st4,st5,st6,st7,arg
>> > +{  1, 1, 1, 0, 0, 0, 0, 1, 1,  1,  1,  1,  1,  1,  1,  1,  1 }
>> > + */
>> > +
>> > +/*
>> > + * Note: GRUB is compiled with the options -mrtd and -mregparm=3.
>> > + *       So the first three arguments are passed in %eax, %edx, and %ecx,
>> > + *       respectively, and if a function has a fixed number of arguments
>> > + *       and the number if greater than three, the function must return
>> > + *       with "ret $N" where N is ((the number of arguments) - 3) * 4.
>> > + */
>> > +
>> > +#include <grub/symbol.h>
>> > +
>> > +#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - 
>> > grub_drivemap_int13_handler_base)
>> > +
>> > +/* Copy starts here. When deployed, this label must be segment-aligned */
>> > +VARIABLE(grub_drivemap_int13_handler_base)
>> > +
>> > +VARIABLE(grub_drivemap_int13_oldhandler)
>> > +  .word 0xdead, 0xbeef
>> > +/* Drivemap module - INT 13h handler - BIOS HD map */
>> > +/* We need to use relative addressing, and with CS to top it all, since we
>> > +   must make as few changes to the registers as possible.  IP-relative
>> > +   addressing like on amd64 would make life way easier here. */
>> > +.code16
>> > +FUNCTION(grub_drivemap_int13_handler)
>> > +  push %bp
>> > +  mov %sp, %bp
>> > +  push %ax  /* We'll need it later to determine the used BIOS function */
>> > +
>> > +  /* Map the drive number (always in DL?) */
>> > +  push %ax
>> > +  push %bx
>> > +  push %si
>> > +  mov $GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx
>> > +  xor %si, %si
>> > +1:movw %cs:(%bx,%si), %ax
>> > +  cmp %ah, %al
>> > +  jz 3f /* DRV=DST => map end - drive not remapped, leave DL as-is */
>> > +  cmp %dl, %al
>> > +  jz 2f /* Found - drive remapped, modify DL */
>> > +  add $2, %si
>> > +  jmp 1b /* Not found, but more remaining, loop  */
>> > +2:mov %ah, %dl
>> > +3:pop %si
>> > +  pop %bx
>> > +  xchgw %ax, -4(%bp) /* Recover the old AX and save the map entry for 
>> > later */
>> > +  
>> > +  push %bp
>> > +  /* Simulate interrupt call: push flags and do a far call in order to set
>> > +     the stack the way the old handler expects it so that its iret works 
>> > */
>> > +  push 6(%bp)
>> > +  movw (%bp), %bp  /* Restore the caller BP (is this needed and/or 
>> > sensible?) */
>> > +  lcall *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
>> > +  pop %bp /* The pushed flags were removed by iret */
>> > +  /* Set the saved flags to what the int13h handler returned */
>> > +  push %ax
>> > +  pushf
>> > +  pop %ax
>> > +  movw %ax, 6(%bp)
>> > +  pop %ax
>> > +
>> > +  /* Reverse map any returned drive number if the data returned includes 
>> > it.  
>> > +     The only func that does this seems to be origAH = 0x08, but many BIOS
>> > +     refs say retDL = # of drives connected.  However, the GRUB Legacy 
>> > code
>> > +     treats this as the _drive number_ and "undoes" the remapping.  Thus,
>> > +     this section has been disabled for testing if it's required */
>> > +#  cmpb $0x08, -1(%bp) /* Caller's AH */
>> > +#  jne 4f
>> > +#  xchgw %ax, -4(%bp) /* Map entry used */
>> > +#  cmp %ah, %al  /* DRV=DST => drive not remapped */
>> > +#  je 4f
>> > +#  mov %ah, %dl  /* Undo remap */
>> > +
>> > +4:mov %bp, %sp
>> > +  pop %bp
>> > +  iret
>> > +/* This label MUST be at the end of the copied block, since the installer 
>> > code
>> > +   reserves additional space for mappings at runtime and copies them over 
>> > it */
>> > +.align 2
>> > +VARIABLE(grub_drivemap_int13_mapstart)
>> > +/* Copy stops here */
>> > +.code32
>> > +VARIABLE(grub_drivemap_int13_size)
>> > +  .word GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_size)
>> > +
>> > Index: conf/i386-pc.rmk
>> > ===================================================================
>> > --- conf/i386-pc.rmk       (revisión: 1766)
>> > +++ conf/i386-pc.rmk       (copia de trabajo)
>> > @@ -158,7 +158,7 @@
>> >    vbe.mod vbetest.mod vbeinfo.mod video.mod gfxterm.mod \
>> >    videotest.mod play.mod bitmap.mod tga.mod cpuid.mod serial.mod  \
>> >    ata.mod vga.mod memdisk.mod jpeg.mod png.mod pci.mod lspci.mod \
>> > -  aout.mod _bsd.mod bsd.mod
>> > +  aout.mod _bsd.mod bsd.mod drivemap.mod
>> >  
>> >  # For biosdisk.mod.
>> >  biosdisk_mod_SOURCES = disk/i386/pc/biosdisk.c
>> > @@ -325,4 +325,11 @@
>> >  bsd_mod_CFLAGS = $(COMMON_CFLAGS)
>> >  bsd_mod_LDFLAGS = $(COMMON_LDFLAGS)
>> >  
>> > +# For drivemap.mod.
>> > +drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \
>> > +                       commands/i386/pc/drivemap_int13h.S
>> > +drivemap_mod_ASFLAGS = $(COMMON_ASFLAGS)
>> > +drivemap_mod_CFLAGS = $(COMMON_CFLAGS)
>> > +drivemap_mod_LDFLAGS = $(COMMON_LDFLAGS)
>> > +
>> >  include $(srcdir)/conf/common.mk
>> > Index: include/grub/loader.h
>> > ===================================================================
>> > --- include/grub/loader.h  (revisión: 1766)
>> > +++ include/grub/loader.h  (copia de trabajo)
>> > @@ -37,6 +37,22 @@
>> >  /* Unset current loader, if any.  */
>> >  void EXPORT_FUNC(grub_loader_unset) (void);
>> >  
>> > +typedef struct hooklist_node *grub_preboot_hookid;
>> > +
>> > +/* Register a function to be called before the boot hook.  Returns an id 
>> > that
>> > +   can be later used to unregister the preboot (i.e. on module unload).  
>> > If
>> > +   abort_on_error is set, the boot sequence will abort if any of the 
>> > registered
>> > +   functions return anything else than GRUB_ERR_NONE.
>> > +   On error, the return value will compare equal to 0 and the error 
>> > information
>> > +   will be available in errno and errmsg.  However, if the call is 
>> > successful
>> > +   those variables are _not_ modified. */
>> 
>> No need to mention errmsg, it's internal to GRUB.  As for errno (which
>> is grub_errno, actually) it does not need to be mentioned, otherwise
>> we would have to do so everywhere.  Please capitalize HOOK and
>> ABORT_ON_ERROR in the comments above.
> Done. "hook" removed because it referred to the loader module boot
> function.
>> 
>> > +grub_preboot_hookid EXPORT_FUNC(grub_loader_register_preboot)
>> > +           (grub_err_t (*hook) (void), int abort_on_error);
>> > +
>> > +/* Unregister a preboot hook by the id returned by 
>> > loader_register_preboot.
>> > +   This functions becomes a no-op if no such function is registered */
>> > +void EXPORT_FUNC(grub_loader_unregister_preboot) (grub_preboot_hookid id);
>> > +
>> >  /* Call the boot hook in current loader. This may or may not return,
>> >     depending on the setting by grub_loader_set.  */
>> 
>> Nitpick: "loader.  This..."
> Are you a bot? ¬¬ Corrected

It would make like much simpler if I were ;-).  What makes you think
so?

>> >  grub_err_t EXPORT_FUNC(grub_loader_boot) (void);
>> > Index: kern/loader.c
>> > ===================================================================
>> > --- kern/loader.c  (revisión: 1766)
>> > +++ kern/loader.c  (copia de trabajo)
>> > @@ -61,11 +61,82 @@
>> >    grub_loader_loaded = 0;
>> >  }
>> >  
>> > +struct hooklist_node
>> > +{
>> > +  grub_err_t (*hook) (void);
>> > +  int abort_on_error;
>> > +  struct hooklist_node *next;
>> > +};
>> > +
>> > +static struct hooklist_node *preboot_hooks = 0;
>> > +
>> > +grub_preboot_hookid
>> > +grub_loader_register_preboot(grub_err_t (*hook) (void), int 
>> > abort_on_error)
>> > +{
>> > +  if (!hook)
>> > +    {
>> > +      grub_error (GRUB_ERR_BAD_ARGUMENT, "preboot hook must not be NULL");
>> > +      return 0;
>> > +    }
>> > +  grub_preboot_hookid newentry = grub_malloc (sizeof (struct 
>> > hooklist_node));
>> 
>> Mixed declarations/code.
> Oops, sorry. I put most of my attention on drivemap.c (and even then
> many comments slipped through). Corrected.

Please re-check them, I might have missed things this time...

>> > +  if (!newentry)
>> > +    {
>> > +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot alloc a hookinfo 
>> > structure");
>> > +      return 0;
>> > +    }
>> > +  else
>> > +    {
>> > +      newentry->hook = hook;
>> > +      newentry->abort_on_error = abort_on_error;
>> > +      newentry->next = preboot_hooks;
>> > +      preboot_hooks = newentry;
>> > +      return newentry;
>> > +    }
>> > +}
>> > +
>> > +void
>> > +grub_loader_unregister_preboot(grub_preboot_hookid id)
>> 
>> "preboot (grub"
> Corrected on both functions ;)
>> 
>> > +{
>> > +  grub_preboot_hookid entry = 0;
>> > +  grub_preboot_hookid search = preboot_hooks;
>> > +  grub_preboot_hookid previous = 0;
>> > +
>> > +  if (0 == id)
>> > +    return;
>> 
>> ...
> ... xD
>> 
>> > +  while (search)
>> > +    {
>> > +      if (search == id)
>> > +        {
>> > +          entry = search;
>> > +          break;
>> > +        }
>> > +      previous = search;
>> > +      search = search->next;
>> > +    }
>> > +  if (entry) /* Found */
>> 
>> ...
> Comment removed, was unnecessary.
>> 
>> > +    {
>> > +      if (previous)
>> > +        previous->next = entry->next;
>> > +      else preboot_hooks = entry->next; /* Entry was head of list */
>> > +      grub_free (entry);
>> > +    }
>> > +}
>> > +
>> >  grub_err_t
>> >  grub_loader_boot (void)
>> >  {
>> >    if (! grub_loader_loaded)
>> >      return grub_error (GRUB_ERR_NO_KERNEL, "no loaded kernel");
>> > +  
>> > +  grub_preboot_hookid entry = preboot_hooks;
>> 
>> Mixed declarations/code.
> Moved the whole line up.
>> 
>> > +  while (entry)
>> > +    {
>> > +      grub_err_t possible_error = entry->hook();
>> > +      if (possible_error != GRUB_ERR_NONE && entry->abort_on_error)
>> > +        return possible_error;
>> > +      entry = entry->next;
>> > +    }
>> >  
>> >    if (grub_loader_noreturn)
>> >      grub_machine_fini ();