Re: [PATCH] Drivemap module

[Javier Martín]

Hi there,

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

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.
> 
> > 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.
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.
> 
> 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
> 
> 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
> 
> 
> 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).
> 
> > +/* 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.
> 
> > +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; }
> 
> > +}
> > +
> > +/* 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)?
> 
> > +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.
> 
> > +    {
> > +      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
> 
> >  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.
> 
> > +  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 ();
> > _______________________________________________
> > Grub-devel mailing list
> > address@hidden
> > http://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel

New version of the patch follows. I must say that the discussion here
has become pretty interesting without degrading into flamewarring - who
whats to call Mr. Torvalds and Prof. Tanembaum? ^^

drivemap.patch.5
Description: Text Data

signature.asc
Description: Esta parte del mensaje está firmada digitalmente