Re: [PATCH] Drivemap module

[Marco Gerards]

Javier Martín <address@hidden> writes:

> El dom, 20-07-2008 a las 21:40 +0200, Marco Gerards escribió:
>> Did you use code from other people or projects?
> No, as far as I can control my own mind: the assembly int13h handler is
> loosely based on that of GRUB Legacy, but heavily rewritten. All other
> code was written from scratch, even the crappy linked lists all over the
> place.

Okay :-)

Sorry I kept you waiting... again...

>> > 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.
>> 
>> 
>> I have copied the changelog entry from your other e-mail:
>> 
>>      * commands/i386/pc/drivemap.c : New file, main part of the new
>>      drivemap module allowing BIOS drive remapping not unlike the
>>      legacy "map" command. This allows to boot OSes with boot-time
>>      dependencies on the particular ordering of BIOS drives or
>>      trusting their own to be 0x80, like Windows XP, with
>>      non-standard boot configurations.
>> 
>> "New file." would be sufficient
>> 
>>      * commands/i386/pc/drivemap_int13h.S : New file, INT 13h handler
>>      for the drivemap module. Installed as a TSR routine by
>>      drivemap.c, performs the actual redirection of BIOS drives.
>> 
>> Same here.
> Hmm... isn't the ChangeLog too spartan? I thought it should be a bit
> informative - what about a single sentence per file?
>       * commands/i386/pc/drivemap.c : New file - drivemap command and
>       int13h installer
>       * commands/i386/pc/drivemap_int13h.S : New file, resident real
>       mode BIOS int13 handler

No, please just stick to "New file.".  The Changelog is used for
changes only.  If you add more information, it will be noise and it
will make my job harder.

>>     * conf/i386-pc.rmk : Added the new module
>> Please say which variables you added in this file.  You can find some
>> examples on how to do this in the ChangeLog 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
> And now we're being uselessly verbose. IMHO, ChangeLog should be more
> about semantic changes in the code and less about literal changes - we
> have `svn diff' for those.

You are wrong.  It is your opinion that this should go into a
changelog entry and I can understand that.  However, this is simply
how we do it ;)

>>      * include/grub/loader.h : Added a "just-before-boot" callback
>>      infrastructure used by drivemap.mod to install the INT13 handler
>>      only when the "boot" command has been issued.
>> 
>> Please describe changes, not effects.  So which prototypes and macros
>> did you add?
>> 
>       * include/grub/loader.h (grub_loader_register_preboot) : New
>       function (proto). Register a new pre-boot handler
>       (grub_loader_unregister_preboot) : Likewise. Unregister handler
>       (grub_preboot_hookid) : New typedef. Registered hook "handle"
>>      * kern/loader.c : Implement the preboot-hook described
>> 
>> Which functions did you change and how?  Please describe actual changes.
>       * kern/loader.c (grub_loader_register_preboot) : New function.
>       (grub_loader_unregister_preboot) : Likewise.
>       (preboot_hooks) : New variable. Linked list of preboot hooks
>       (grub_loader_boot) : Call the list of preboot-hooks before the
>       actual loader
>> 
>> The header is missing, please include it.  Also newlines between the
>> files make it easier to read.
> What header? The drivemap module itself has no .h files. The only header
> I touch is loader.h, and is both in the ChangeLog entry and the patch.

With header I meant the first line of the changelog entry that
includes your name and e-mail address.
 
>> Here follows a review.  Sorry I kept you waiting for this long, this
>> feature and your work is really appreciated!  Perhaps I can spot some
>> more problems after you fixed it and supplied an updated changelog
>> entry.  There are quite some comments, but please do not let this
>> demotivate you, it is mainly coding style related :-)
> Well, thanks to all the time I had free, I have nearly finished Final
> Fantasy XII, so the wait was not soo bad ^^

:-)

>> (...)
>> > +
>> > +/* 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
>> 
>> Please use the grub_dprintf infrastructure.
> Done. I didn't even know it existed :S

:-)
 
>> > +/* 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);
>> 
>> Please export stuff in header files, that's the normal practise in
>> this file as well, right?  What's not a typo?
> EXPORT_* macros removed; seemingly they are no longer needed because all
> code is in the same module (initially the assembly was in kernel). 
>
> What's not a typo is the definition of grub_symbol_t as "void" instead
> of something more sound to a C programmer, like "void *". I don't really
> know how to explain it in the source, but it's just a way to make the
> names known to the C compiler, so that I then take their addresses with
> the & operator. Such names are _not_ variables in the C file and they
> take no space or storage.

I see.  But I do not think a comment is needed here to say that this
is not a typo.
 
>> > +static grub_preboot_hookid insthandler_hook = 0;
>> 
>> You can leave the "= 0" away.
>> (...)
>> > +static drivemap_node_t *drivemap = 0;
>> 
>> Same here :-)
> Does GRUB zero-initialize the memory of modules when loading them? The
> first variable would be OK without the = 0, but not the second, since
> there are some checks of the form if(var) scattered all over the code -
> "drivemap" is a linked list, so 0 means EOL.

Yes, all globals are set to 0.

>> (...)
>> > +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. */
>> 
>> Please place the comments before the function.
>> (...)
>> > +  if (mapping)  /* There was a mapping already in place, modify it */
>> > +    mapping->redirto = redirto;
>> 
>> Please place the comment before the if statement.  Can you format the
>> comment such that it begins with a capital and ends with ".  */" (two
>> spaces)?  Can you do this for the other comments you added as well?
> Hmm... Is the formatting requirement due to some script-based code
> parsing or is it just OCD? ;)

OCD?

The reason is that is makes it easier for editors to automatically
break lines and stuff.  So this way an editor can parse the sentence,
we use that.

> Besides, saying "there was a mapping" before the "if" that checks it
> would be strange. I'd prefer to leave this line as is, or find an
> alternate wording for the comment (and I'm sleepy right now)

I think it would be clear if you moved the comment.  But rephrasing it
would be fine to me.

>> > +  else  /* Create a new mapping and add it to the head of the list */
>> > +    {
>> 
>> I would move the comment inside the braces.
> But I wouldn't - the "else" line is almost empty, and placing the
> comment two lines below does not help readability

Please stick to our coding style anyways.

>> (...)
>> > +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. */
>> 
>> Can you move the comments to before the function?  Please do not place
>> *'s on each line.  Without the second * it would be fine.  After a
>> ".", please insert two spaces.
> After _every_ "." or just after the final one? The "no * on each line"
> rule conflicts with the GPL notice comment, but OK.

The GPL notice is an exception.  Yes, after every "." that ends a
sentence.

>> > +{
>> > +  drivemap_node_t *mapping = 0, *search = drivemap, *previous = 0;
>> 
>> Please define one variable per line, can you break this down to 3
>> lines?
> I think the current version is more readable, but OK.
>
>> (...)
>> > +      else drivemap = mapping->next; /* Entry was head of list */
>> 
>> Please place the stuff after "else" on a new line.
> Done, I think.
>
>> 
>> > +      grub_free (mapping);
>> > +    }
>> > +}
>> > +
>> > +static grub_err_t
>> > +parse_biosdisk (const char *name, grub_uint8_t *disknum)
>> > +{
>> > +  if (!name) return GRUB_ERR_BAD_ARGUMENT;
>> 
>> Same here.  Please do not just return GRUB_ERR_BAD_ARGUMENT, use
>> grub_error so you can also define a string for the error.
> This is a function internal to drivemap, called from a single point. I
> don't think the additional information gained by the changes you suggest
> would be worth the space lost by two more error strings (of your
> additional suggestions for this function).

It means that you introduce different error handling.  Someone who is
familiar with GRUB but not with your code will misunderstand it.
Please change it to improve the maintainability of the code.

>> > +  if (*name == '(')
>> > +    name++; /* Skip the first ( in (hd0) - disk_open wants just the name! 
>> > */
>> > +  grub_disk_t disk = grub_disk_open (name);
>> 
>> Although mixed declarations and code are allowed, I personally would
>> avoid this.  It doesn't make the code easier to read, I think.
> What do you advocate, then? Declaring "disk" at the function start
> (three lines above) and then assigning to it? Hmm... maybe, but I find
> this version more sound. The function is small enough to avoid any
> confusion.

Yes, that is what I meant.

>> 
>> > +  else
>> > +    {
>> > +      enum grub_disk_dev_id id = disk->dev->id;
>> > +      if (disknum)
>> > +        *disknum = disk->id;   /* Only valid, of course if it's a 
>> > biosdisk */
>> 
>> Please place the comment before the if.
> The comment does not refer to the "if", but to the assignment: the
> "BIOS" drive num we get is only meaningful if the disk is actually
> managed by biosdisk (which is checked later on return). All other
> solutions (i.e. checking before) expand that section of the code by a
> few lines.

Can you rephrase the comment so you can move it?

>> (...)
>> > +        if (dnum == disk->id && GRUB_DISK_DEVICE_BIOSDISK_ID == 
>> > disk->dev->id)
>> 
>> 
>> This is not rally wrong, but doesn't feel right.  I would use:
>> 
>> disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID
> I find it more sound to place the constant before in the comparison,
> since if the code is modified later and the == is erroneously turned
> into =, my code would throw a compiler error (assignment to constant)
> while yours would happily proceed as a normal assignment inside the if,
> which is perfectly allowed in C, and fail at runtime.

Can you change it anyways?
 
>> I know, I am a pain in the ass right now :P
> I won't deny that, but criticism (particularly constructive criticism as
> yours) is a Good Thing (tm).

Great :-)

>> (...)
>> > +  grub_disk_dev_iterate (&find);
>> 
>> No need for the &.  I even wonder if it would work this way...
> Corrected.
>
>> > +  return ret;
>> > +}
>> > +
>> > +static grub_err_t
>> > +tryparse_diskstring (const char *str, grub_uint8_t *output)
>> > +{
>> > +  if (!str) return GRUB_ERR_BAD_ARGUMENT;
>> > +  if (*str == '(')
>> > +    str++;  /* Skip opening paren in order to allow both (hd0) and hd0 */
>> > +  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, &str, 0);
>> > +      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
>> > +        return GRUB_ERR_BAD_ARGUMENT; /* Could not convert, or num too 
>> > high for BIOS */
>> 
>> Use grub_error
> Same response as before: internal helper function, used at a single
> point with known error conditions which should be obvious. Moreover, the
> drivemap command uses this func on a "try-this-first" basis on its
> second argument, then resorts to raw number parsing. Thus, we do not
> want grub_error printing anything to screen (does it, by the way?).

It's how the grub_err_t interface is used.  Thus can you change it for
maintainability?

>> > +      else
>> > +       {
>> > +          bios_num |= drivenum;
>> > +          if (output)
>> > +            *output = bios_num;
>> > +          return GRUB_ERR_NONE;
>> > +        }
>> > +    }
>> > +  else return GRUB_ERR_BAD_ARGUMENT;
>> > +}
>> > +
>> > +static grub_err_t
>> > +grub_cmd_drivemap (struct grub_arg_list *state, int argc, char **args)
>> > +{
>> > +  if (state[0].set) /* Show: list mappings */
>> 
>> Like the comments before
> And like the answers before. Since this has been raised a lot of times,
> and in order to compare our thoughts on comments, I'll post what I think
> comments should tell in each pos:
>
>       /* Preconditions and check explanation */
>       if (blah && (foo || bar)) /* Cond-met: action to take (short) */
>       {
>               /* If too long to fit elsewhere: preconditions inside the IF 
> block,
> procedure to follow */
>               (...)
>       }
>
> The same for the "else if" / "else" blocks, and after all of it maybe an
> additional comment explaining the postconditions, if required.

Well, everyone has a different coding style.  I simply try to keep
GRUB consistent.  I hope you can appreciate that.
 
>> > +    {
>> > +      if (!drivemap)
>> > +        grub_printf ("No drives have been remapped");
>> > +      else
>> > +        {
>> > +          grub_printf ("Showing only remapped drives. Drives that have 
>> > had "
>> > +                       "their slot assigned to another one and have not 
>> > been "
>> > +                       "themselves remapped will become inaccessible 
>> > through "
>> > +                       "the BIOS routines to the booted OS.\n\n");
>> 
>> I do not think this message is immediatly clear to the user.  Perhaps
>> we should even leave this out and say something in the to-be-written
>> manual? :-)
> Absolutely true. This message is pathetic and crappy, but as long as
> there's no manual... Can anyone suggest an alternative, clearer wording
> for it?

By writing a manual? :-)  I think I would simply remove the entire
grub_printf.  Can you do that?  We should not compensate for a lacking
manual in the sourcecode.

>> > +          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 */
>> > +    {
>> > +      if (drivemap)
>> > +        {
>> > +          drivemap_node_t *curnode = drivemap, *prevnode = 0;
>> > +          while (curnode)
>> > +            {
>> > +              prevnode = curnode;
>> > +              curnode = curnode->next;
>> > +              grub_free (prevnode);
>> > +            }
>> > +          drivemap = 0;
>> > +        }
>> > +    }
>> > +  else
>> > +    {
>> > +      if (argc != 2)
>> > +        return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments 
>> > required");
>> > +      grub_uint8_t mapfrom = 0;
>> 
>> Please do not use mixed code/declarations here.
> The only mixed code/decl I find is "grub_err_t err = revparse_biosdisk
> (curnode->redirto, &dname);" and it's a call to an internal function
> whose conditions are well-known - safe. All others are declarations with
> initializations, which I find pretty normal.
>
> However, if you mean literally "mixing" them... do you really advocate
> the K&R approach of "declare everything at the function start"? Time
> proved that scheme suboptimal. As you can see, most declarations are
> either at the very start of a _block_ (the scope unit) or near it, right
> after precondition checks.

Yes, or *directly* at the beginning of a block.  But before the
precondition checks.

>> > +      grub_err_t err = parse_biosdisk (args[0], &mapfrom);
>> > +      if (err != GRUB_ERR_NONE)
>> > +        return grub_error (err, "invalid disk or not managed by the 
>> > BIOS");
>> > +
>> > +      grub_uint8_t mapto = 0xFF;
>> > +      err = tryparse_diskstring (args[1], &mapto);
>> > +      if (err != GRUB_ERR_NONE) /* Not a disk string. Maybe a raw num 
>> > then? */
>> > +        {    
>> > +          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;
>> > +        }
>> 
>> Do we really want to support BIOS disk numbers here?  I do not think
>> it is useful.
> I asked the same when I was starting to write this and I was told, more
> or less, that this was the way to go.

Who told you?  I think we should not do this.

>> Can you add newlines between the #defines and the comments, this will
>> get quite messy like this.
> Done.
>
>> 
>> > +    {
>> > +      entries++;
>> > +      curentry = curentry->next;
>> > +    }
>> > +  if (0 == entries)
>> 
>> if (! entries)
> Nope. You C infidels might make no distinction between int and bool, but
> I was raised in the Holy Truth of languages with a boolean type. ^^ 
> Semantically, "entries" represents an integer, and I want to check
> whether it is zero. I only use your form when checking for
> a)boolean-like conditions or b)non-null pointers.
>
> Even this last one would be "wrong" under my guidelines, since ANSI
> specifies that a null pointer is guaranteed to compare equal to zero,
> but in that case alone I let my laziness win and use the Unholy
> int->"boolean" interpretation of C's "if" statement.
>
> Most probably, the compiler will optimize the additional assembly space
> cost away if there's any, so no worries. I find the semantic
> distinctions important to keep, particularly if the cost is zero.

Can you at least change the comparison order? :)

>> (...)
>> > +GRUB_MOD_FINI (drivemap)
>> > +{
>> > +  grub_loader_unregister_preboot (insthandler_hook);
>> > +  insthandler_hook = 0;
>> > +  grub_unregister_command ("drivemap");
>> > +}
>> 
>> I wonder if this is not called, just before GRUB boots a kernel.  That
>> would cause a problem :-)
> AFAIK the "fini" functions are called by grub_machine_fini, which is
> called _after_ the preboot hook.

ok

>> > Index: commands/i386/pc/drivemap_int13h.S
>> > ===================================================================
>> > RCS file: commands/i386/pc/drivemap_int13h.S
>> > diff -N commands/i386/pc/drivemap_int13h.S
>> > --- /dev/null      1 Jan 1970 00:00:00 -0000
>> > +++ commands/i386/pc/drivemap_int13h.S     2 Jul 2008 01:12:36 -0000
> And here we go for the second round xD
>
>> (...)
>> > +
>> > +
>> > +/*
>> > + * 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.
>> > + */
>> >
>> 
>> I do not think this is required.  I mean, we know how GRUB is compiled
>> and how it deals with registers.
> But nevertheless each and every assembly file I looked at states the
> same. It's supposed to be useful when developing; although I did not use
> it, since my only assembly function is not called from C.

ok

>> > +/*
>> > + *  This is the area for all of the special variables.
>> > + */
>> 
>> I don't think this comment is useful.
> I dont remember writing that... Maybe I c-ped it from somewhere?
> Deleted.
>
>> > +#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. Pity we're not 
>> > on
>> > + * amd64: rIP-relative addressing would make life 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)
> I'm disappointed... Not a single comment on this file?? ;)

I am not an assembler/x86 guru ;-)

>> (...)
>> > Index: kern/loader.c
>> > ===================================================================
>> > RCS file: /sources/grub/grub2/kern/loader.c,v
>> > retrieving revision 1.9
>> > diff -u -r1.9 loader.c
>> > --- kern/loader.c  21 Jul 2007 23:32:26 -0000      1.9
>> > +++ kern/loader.c  2 Jul 2008 01:12:45 -0000
>> > @@ -61,11 +61,78 @@
>> >    grub_loader_loaded = 0;
>> >  }
>> >  
>> > +struct hooklist_node
>> > +{
>> > +  grub_err_t (*hook) (void);
>> > +  int abort_on_error;
>> > +  struct hooklist_node *next;
>> > +};
>> 
>> Isn't there something 
> Woot? What do you mean with that? o_O

I think I was distracted when I typed this, so I did not finish the

>> > +static struct hooklist_node *preboot_hooks = 0;
>> > +
>> > +grub_preboot_hookid
>> > +grub_loader_register_preboot(grub_err_t (*hook) (void), int 
>> > abort_on_error)
>> > +{
>> > +  if (0 == hook)
>> 
>> if (! hook)
> This time you're right, per my "how to write C comparisons" rationale
> stated above. Changed. 
>
>> > +    {
>> > +      grub_error (GRUB_ERR_BAD_ARGUMENT, "preboot hoook must not be 
>> > null");
>> 
>> hoook
>> 
>> null -> NULL
> Changed too.
>
>> (...)
>> > +
>> > +void
>> > +grub_loader_unregister_preboot(grub_preboot_hookid id)
>> > +{
>> > +  if (0 == id)
>> > +    return;
>> > +  grub_preboot_hookid entry = 0, search = preboot_hooks, previous = 0;
>> >
>> Can you move this up so this is not mixed.  Can you split this into 3 lines?
> As I understand coding style in C-like languages, precondition checks
> (if short enough) and declarations may be mixed as long as they don't
> get confusing; and I don't think this one is too confusing.
>
> On the other hand, variables splitted, but why the obsession with that
> particular rule?

For both issues, it is for consistency.  There is some rationale for
splitting variable declarations, although it does not apply in your
case.  Some people write:

int* pointer1, pointer2;

And sometimes it just gets messy.  The main reason is consistency
though.

>> (...)
>> > +      grub_err_t possible_error = entry->hook();
>> 
>> Just use "err"
> I think I did so in a previous version of this patch, and I was told to
> be a bit more explicit, but... changed.
>
> Phew! That was long, even after removing some parts. Well, this is the
> new version of the patch. Cheers!

I'll review the new patch...

> PS: I already signed the copyright assignment papers and sent them to
> the FSF. They should arrive within this week.

Cool :-)

--
Marco