Re: [PATCH] Drivemap module

[Javier Martín]

After your latest replay, I "reevaluated" my stubbornness WRT some of
your advices, and I've changed a few things:

- Variables are now declared (and, if possible, initialized) before
precondition checks, even simple ones. The install_int13_handler
function has not been modified, however, since I find it a bit
nonsensical to put bunch of declarations without an obvious meaning just
after the "else" line:
      grub_uint32_t *ivtslot;
      grub_uint16_t *bpa_freekb;
      grub_size_t total_size;
      grub_uint16_t payload_sizekb;
      grub_uint8_t *handler_base;
      int13map_node_t *handler_map;
      grub_uint32_t ivtentry;
- Only one declaration per line: even though C is a bit absurd in not
recognizing T* as a first class type and instead thinking of * as a
qualifier to the variable name; and even though my code does not incur
into such ambiguities.
- Comments moved as you required, reworded as needed
- Extensive printf showing quasi-help in the "show" mode trimmed down to
just the first sentence.
- Internal helper functions now use the standard error handling, i.e.
return grub_error (err, fmt, ...)
- Comment about the strange "void" type instead of "void*" rephrased to
be clearer

There is, however, one point in which I keep my objection: comparisons
between a variable and a constant should be of the form CONSTANT ==
variable and not in the reverse order, since an erroneous but quite
possible change of == for = results in a compile-time error instead of a
_extremely_ difficult to trace runtime bug. Such kind of bugs are quite
excruciating to find in userspace applications within an IDE, so I can't
even consider the pain to debug them in a bootloader.

WRT accepting raw BIOS disk numbers, I agree with you in principle, but
I'm keeping the functionality for now, since I don't quite like the
"drivemap (hd0) (hd1)" syntax - which device is which?. I'd rather have
something akin to "drivemap (hd0) (bios:hd1)", but I want to hear the
opinions of people in this list.

The new version of the patch is attached, and here is my suggested CLog:

2008-08-XX  Javier Martin  <address@hidden>
        * commands/i386/pc/drivemap.c : New file.
        * commands/i386/pc/drivemap_int13h.S : New file.
        * conf/i386-pc.rmk (pkglib_MODULES) : Added drivemap.mod
        (drivemap_mod_SOURCES) : New variable
        (drivemap_mod_ASFLAGS) : Likewise
        (drivemap_mod_CFLAGS) : Likewise
        (drivemap_mod_LDFLAGS) : Likewise
        * include/grub/loader.h (grub_loader_register_preboot) : New
        function prototype. Register a new pre-boot handler
        (grub_loader_unregister_preboot) : Likewise. Unregister handler
        (grub_preboot_hookid) : New typedef. Registered hook "handle"
        * kern/loader.c (grub_loader_register_preboot) : New function.
        (grub_loader_unregister_preboot) : Likewise.
        (preboot_hooks) : New variable. Linked list of preboot hooks
        (grub_loader_boot) : Call the list of preboot-hooks before the
        actual loader

drivemap.patch.4
Description: Text Data

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