Re: [PATCH] Drivemap module

[Javier Martín]

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.

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

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

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

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

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

> (...)
> > +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? ;)

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)

> > +  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

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

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

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

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

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

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

> (...)
> > +  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?).

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

> 
> > +    {
> > +      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?

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

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

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

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

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

> > +/*
> > + *  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?? ;)

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

> > +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?

> (...)
> > +      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!


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

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