Re: [PATCH] Reimplementing the legacy "map" command

[Javier Martín]

El vie, 06-06-2008 a las 19:31 +0300, Vesa Jääskeläinen escribió:
> Did you forgot something from this patch :) ?
Er... not that I know of. What do you mean? Did I leave something
important off? If it's the licensing info, I put it under the same as
the whole GRUB2 project, I suppose. The int13 handler code, however, is
based (though changed) on the equivalent code in GRUB legacy.

> No commented out code unless there is a really good reason for it.
Ok, I'll remove the "debug" sections in the int13 handler. Should I also
remove the non-error text output of the command? (like "mapping (hd1) to
0x80")

> Try to move int13h handle to module not to kernel. We do not want to put
> anything more to kernel unless there is a really good reason to do so.
Seems A Good Thing (tm). However, all the loaders have their assembler
code in the kernel, and I have yet to find a single assembly file out of
the /kern dir (except for the setjmp.S support routines). As I state
below, I don't understand the GRUB build system quite well, and would
appreciate to have it explained so that I can break the asm part off the
kernel loader.S and into its own drivemap.S file.

> +/* This is NOT a typo: we just want this "void" var in order to take its
> + * address with &var - used for relative addressing within the int13 
> handler */
> +extern void EXPORT_VAR(grub_drivemap_int13_handler_base);
> 
> Create new type for it. Or use variable type that eats that space. This 
> way you do not need this comment to confuse people.
Well, seems the comment was not as effective as I thought ^.^ - there is
_no_ space, the only purpose of that pseudo-variable is having its
address taken with the & operator. The only sensible "new type" for it
would be something akin to 
        typedef void grub_symbol_t;
Which would be also puzzling and require a comment to stop people from
changing it to void*. However, the information would be more centralized
then and it would cause less head-scratching.

> Please do not include .mk files on next patch.
I don't quite understand the GRUB2 build system right now, but if .mk
files don't get patched, how does it know they exist? Are those files
autogenerated somehow? I didn't find the autotools files...

> Abort on error?... Ok... do you go to deinit everything before that was 
> successfully installed or just give warning or ?
I don't know; either choice might be sensible and still has drawbacks
(increased complexity, potentially undoable actions, etc). My design of
this new functionality was a bit ad-hoc, and I added that flag thinking
"if the drives cannot be mapped, then the user wouldn't want the system
started". However, as you reason, with multiple preboot hooks the system
could be let in a potentially inconsistent state.

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