Re: status grub2 port of grub-legasy map command

[Pavel Roskin]

On Sun, 2009-05-03 at 02:02 +0200, Javier Martín wrote:
> I am glad to inform that, with the new version of the mmap patch,
> drivemap now boots all my hd1 installs of:
> - Windows XP (Pro x64)
> - ReactOS
> - FreeDOS

I confirm that 32-bit Windows XP is working too.

> I would suggest, however, that the return type of
> grub_mmap_malign_and_register be changed to void* from char*, just like
> the return type from malloc, because it's the meaningful data type to
> indicate a pointer to generic memory _and_ it automatically casts to any
> pointer type the caller uses (which is the reason it's used in malloc).

Done.

A few comments regarding the code.

The patch adds many trailing spaces.  I suggest that you run GNU indent
on drivemap.c.  It will take care of most of the trailing spaces.
Comments will still need to be fixed.

Assembler files use different formatting in GRUB.  Also, it's better to
use meaningful names for the labels.  Label 4 is unused.

Some comments are excessive or unnecessary.  "These functions defined in
this file may be called from C" - irrelevant, we have no such functions.
Complaints that the processor is not in 64-bit mode are also useless.  I
don't understand what "bundle" means in the comments.

Why do we have duplication between grub_drivemap_int13_mapstart and
grub_drivemap_int13_size?

What is "(void) mod;"?  It doesn't prevent any warnings for me.

grub_symbol_t is already used in kern/dl.c with a different meaning.
Why not just use void?

Please use "void" in the argument list if the function takes no
arguments, as in uninstall_int13_handler().

"two arguments required" may be misleading.  In some cases, only one
argument is required, such as "-l".  Let's make drivemap without
arguments show the map.

Improved patch is attached.

-- 
Regards,
Pavel Roskin

01-drivemap.patch
Description: Text Data