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

[Javier Martín]

El sáb, 07-06-2008 a las 18:02 +0300, Vesa Jääskeläinen escribió:
> 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.
Oops! (blushes) I'm used to GUI SVN tools diffing for me, and I thought
that cvs diff -u would do the trick. Seems that it doesn't u_u. My
apologies for making everyone waste their time because of my stupid
oversight...

> 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)
Nice idea for the future, as it might be a way to add such drives to
DOS-like OSes that delegate all work to the BIOS. However, drivers for
some things might need to stay resident (with the associated memory
consumption) and, if they're written for GRUB (32bit pmode), we'd need
so many things that just thinking about it makes my head spin.
Definitely not an easy task for now.

Oh, and the --clear option already exists, it's called -r or --reset,
which clears all the mappings. Mapping a drive to itself also clears the
mapping, since drivemap only stores remapped drives.

> 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)
This one could be implemented in short time, or might even be available
already, if the grub_disk_open function accepts UUIDs.

> >> 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.
Ok, will do. However, why are the .mk files kept in the CVS tree if they are
autogenerated? Is it because autogen.sh requires Ruby?

> >> +/* 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.
The "variable" (I assume we're talking about int13_handler_base) does
not point anywhere. Let me explicit it: there is no cabal, er...
variable. It is just a label in the assembly code, but it has no space
reserved to it (unlike variables, which are labels with some space after
them). Thus, it has no "content" at all; nothing is read or written to
it (in fact, that would overwrite the variable that is declared _after_
the label), and it's only used to take its address with &var.

The situation with int13_handler_mapstart is similar: it is just a
symbol (space for it is reserved at runtime), and the map proper is *at*
&int13_handler_mapstart. I hope the drivemap.c file will make it
clearer...

> >> 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...
The problem is that if we "still continue" a user could get stuck with
any of these non-informative possibilities, depending on the chainloaded
MBR/bootsector:
        * Nothing at all (Windows XP Pro x64, my PC)
        * An "invalid operating system" message (FreeDOS MBR)
        * A "cannot find kernel/ntldr/whatever" message (?)

I'm attaching the current version missing file (drivemap.c), just so
that it can be examined. However, I will be applying all your
suggestions (removing superfluous messages, moving the asm routine to
its own file, editing just the .rmk file, cleaning commented code
out...) and send an updated version of the full patch, either today or
tomorrow. Thanks!

Habbit

drivemap.c
Description: Text Data

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