[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Reimplementing the legacy "map" command
Re: [PATCH] Reimplementing the legacy "map" command
Sat, 07 Jun 2008 18:02:05 +0300
Thunderbird 184.108.40.206 (Windows/20080421)
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
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 --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
+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...