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

[Vesa Jääskeläinen]

Javier Martín wrote:
> 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.

diff -pruN ...

New files were missing from the patch. Anyway it is a good idea to 
review the patch before sending it.

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

I have no idea what you have there as it wasn't in patch. Perhaps 
differnet message in this case:

"Mapping (hd0) as BIOS device 0x80."

Shown during when hook is called?

During registration of drive mapping (drivemap command), you then check 
if this is possible or not and can give error or warning to screen. But 
if everything is good no messages during processing of command. Only 
show the stuff what is actually happening during the boot.

Future extensions could be:

drivemap --clear
drivemap --ata (ata0) 0x80
drivemap --ata (ata1) --next-device
drivemap --usb (usb0) --next-device
drivemap --eltorito (cd0) --next-device
drivemap --memdisk (hd0)/floppy-image.flp 0x00

--ata would install ATA handler (if loaded)
--usb would install USB handler (if loaded)
--eltorito would install el torito handler (if loaded)
--memdisk would use memory disk for mapping drive (if loaded)

After this sequence following would apply:

0x00: Floppy disk image on memdisk
0x80: ata0 device
0x81: ata1 device
0x82: usb device
0x83: cd-rom with el torito emulation

Another extesion could be that if you can specify your harddisk with 
some UUID it could then be used to identify disk as a source and grub 
would find device matching it. (or use search command)

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

Well I think the first one to do so is gdb debugging patch recently 
presnted on mailinglist. Check .rmk file for ideas.

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

There is space... if you do not have space, then where does the pointer 
point :) ? If you provide external symbol let it be variable or function 
there is always an address and space for it. Even if you say here that 
this is uint8_t then you get same address that if you had uint32_t 
declared as an "extern". extern there it give idea for compiler what 
kind if data there is. It would be best to match what actually is there.

So if you have 32 bits on assembler module you could do

typedef grub_uint32_t my_custom_type_t;

extern my_custom_type_t EXPORT_VAR(my_variable);

Or just grub_uint32_t as a type.

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

You modify .rmk files then run autogen.sh script on the root.

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

Right. So it might be a good idea to show message to screen and still 
continue. User can easily see that there is a problem with something if 
there is a clue on the screen. For some users this might generate 
support request somewhere, but hey... it would generate it anyway should 
it return back to grub...