Hi Javier, Javier Martín <[EMAIL PROTECTED]> writes:
> El lun, 04-08-2008 a las 22:51 +0200, Marco Gerards escribió: >> Javier Martín <[EMAIL PROTECTED]> writes: >> >> > 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; >> >> Please understand me correctly. Code just has to be written according >> to our coding standards before it can and will be included. We can >> discuss things endlessly but we will simply stick to the GNU Coding >> Standards as you might expect. >> >> I hope you can appreciate that all code of GRUB has the same coding >> style, if you like this style or not. > Big sigh. Function modified to conform to your preciousss coding style. > I might look a bit childish or arrogant saying this, but what is the > point upholding a coding style that, no matter how consistent it is, > hinders readability. With so much local variables, they are hard to keep > track of no matter the coding style, but with the old ("mixed, heretic") > style there was not need for a comment before each declaration: they > were declared and used when needed instead of at the start of a block, > because that function is inherently linear, not block-structured. In your opinion it is not a good coding style. I just avoid discussions about it because such discussions just waste my time. Even if you are able to convince me, GRUB is a GNU project and will remain to use the GCS. >> > - 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 >> >> Thanks a lot! >> >> > 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. >> >> I understand your concern, nevertheless, can you please change it? > You understand my concern, but seemingly do not understand that in order > to conform to the Holy Coding Style you are asking me to write code that > can become buggy (and with a very hard to trace bug) with a simple > deltion! (point: did you notice that last word _without_ a spelling > checker? Now try to do so in a multithousand-line program). Yes, I did notice it immediately. The coding style is not holy in any way. Everyone has its own coding style. You must understand I do not want to fully discuss it every time someone sends in a patch, especially since it will not change anyways. > While tools like `svn diff' can help in these kind of problems, their > utility is greatly diminished if the offending change is part of a > multi-line change. Besides, sometimes checks like "if (a=b)", or more > frequently "if (a=f())" are intentionally used in C, so the error might > become even more difficult to spot and correct. I ask for a deep > reflection on this issue: maybe I'm dead wrong and an arrogant brat in > my attempt to tackle the Holy GNU Coding Standards, but I'd like to ask > input from more people on this. I will refuse patches with "if (a = f())", if that makes you sleep better ;-) >> > 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. >> >> I personally do not care a lot if BIOS disk numbers are used. >> Although I do not see why it is useful. >> >> As for the syntax, I would prefer something more GRUB2ish, like: >> >> map --bios=(hd0) --os=(hd1) > I agree. What about "grub" for the source drive instead of "os", with > "bios" being the target drive? I mean: > drivemap --grub=(hd1) --bios=(hd0) > Would map 0x80 in the BIOS to grub's (hd1) drive which will most likely > be BIOS drive 0x81. It will not change the functionality which GRUB is active. But it will change it for the OS that is loaded. So I do not think --grub= is a good idea because of this. As for GRUB Legacy, it confused a lot of people, so making people explicitly say that it is changed for the OS, this confusion might go away :-) > However, I prefer not to change it right now. Maybe when there are no > other issues WRT to this patch we can set on to tackle this one. So first I'll review this patch, then you will send in a new one? >> Or so, perhaps the long argument names can be chosen in a more clever >> way :-) >> > The new version of the patch is attached, and here is my suggested CLog: >> > >> > 2008-08-XX Javier Martin <[EMAIL PROTECTED]> >> >> (newline) >> >> > * 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 >> >> No need how the change is used or why it was added. >> >> > (grub_loader_unregister_preboot) : Likewise. Unregister handler >> >> Same here. >> >> > (grub_preboot_hookid) : New typedef. Registered hook "handle" >> >> Same here. >> >> > * kern/loader.c (grub_loader_register_preboot) : New function. >> > (grub_loader_unregister_preboot) : Likewise. >> > (preboot_hooks) : New variable. Linked list of preboot hooks >> >> Same here. >> >> > (grub_loader_boot) : Call the list of preboot-hooks before the >> > actual loader >> >> What's the `'? > The what? o_O I see some weird character in your text. My font shows it as a block before every `*'. >> Please do not add a space before the ":" > Ok, everything corrected. New CL entry: > > 2008-08-XX Javier Martin <[EMAIL PROTECTED]> > > * 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. > (grub_loader_unregister_preboot): Likewise. > (grub_preboot_hookid): New typedef. > * kern/loader.c (grub_loader_register_preboot): New function. > (grub_loader_unregister_preboot): Likewise. > (preboot_hooks): New variable. > (grub_loader_boot): Call the list of preboot-hooks before the > actual loader Please add a `.' after "New variable" and "Likewise", same for the third and the last sentence. Sometimes you did it right :-). >> Some comments can be found below. Can you please fix the code mention >> in the review and similar code? I really want the code to be just >> like any other GRUB code. Please understand that we have to maintain >> this in the future. If everyone would use his own codingstyle, GRUB >> would become unmaintainable. >> >> > Index: commands/i386/pc/drivemap.c >> > =================================================================== >> > --- commands/i386/pc/drivemap.c (revisión: 0) >> > +++ commands/i386/pc/drivemap.c (revisión: 0) >> > @@ -0,0 +1,417 @@ >> > +/* drivemap.c - command to manage the BIOS drive mappings. */ >> > +/* >> > + * GRUB -- GRand Unified Bootloader >> > + * Copyright (C) 2008 Free Software Foundation, Inc. >> > + * >> > + * GRUB is free software: you can redistribute it and/or modify >> > + * it under the terms of the GNU General Public License as published by >> > + * the Free Software Foundation, either version 3 of the License, or >> > + * (at your option) any later version. >> > + * >> > + * GRUB is distributed in the hope that it will be useful, >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > + * GNU General Public License for more details. >> > + * >> > + * You should have received a copy of the GNU General Public License >> > + * along with GRUB. If not, see <http://www.gnu.org/licenses/>. >> > + */ >> > + >> > +#include <grub/normal.h> >> > +#include <grub/dl.h> >> > +#include <grub/mm.h> >> > +#include <grub/misc.h> >> > +#include <grub/disk.h> >> > +#include <grub/loader.h> >> > +#include <grub/machine/loader.h> >> > +#include <grub/machine/biosdisk.h> >> > + >> > +#define MODNAME "drivemap" >> > + >> > +static const struct grub_arg_option options[] = { >> > + {"list", 'l', 0, "show the current mappings", 0, 0}, >> > + {"reset", 'r', 0, "reset all mappings to the default values", 0, 0}, >> > + {0, 0, 0, 0, 0, 0} >> > +}; >> > + >> > +/* Syms/vars/funcs exported from drivemap_int13h.S - start. */ >> > + >> > +/* Realmode far ptr = 2 * 16b */ >> > +extern grub_uint32_t grub_drivemap_int13_oldhandler; >> > +/* Size of the section to be copied */ >> > +extern grub_uint16_t grub_drivemap_int13_size; >> > + >> > +/* This type is used for imported assembly labels, takes no storage and >> > is only >> > + used to take the symbol address with &label. Do NOT put void* here. >> > */ >> > +typedef void grub_symbol_t; >> > +extern grub_symbol_t grub_drivemap_int13_handler_base; >> > +extern grub_symbol_t grub_drivemap_int13_mapstart; >> > + >> > +void grub_drivemap_int13_handler (void); >> >> The lines above belong in a header file. > True, but they are used in a single file in the whole project and thus I > see it pointless to extract an unneeded header, which would become one > more SVN object to track. However, if you insist I will split the header > file at once. In particular, I think the grub_symbol_t typedef should go > into the "standard" GRUB headers somehow (but not in symbol.h, which is > included from assembly files). Please do so. Other people might want to comment on the symbol change. They will most likely miss it if we keep discussing it here ;-). Can you please send that in as a separate change to give other the opportunity to react on it? >> > +/* Syms/vars/funcs exported from drivemap_int13h.S - end. */ >> > + >> > + >> > +static grub_preboot_hookid insthandler_hook; >> > + >> > +typedef struct drivemap_node >> > +{ >> > + grub_uint8_t newdrive; >> > + grub_uint8_t redirto; >> > + struct drivemap_node *next; >> > +} drivemap_node_t; >> > + >> > +static drivemap_node_t *drivemap = 0; >> >> No need to set this to zero. > Yes, you said so already, but I wanted to make the initial state very > explicit to a future developer, since that variable is checked against > zero in several points. Given that the added source size is four bytes > and the added binary size is _zero_, is all the fuss really necessary? > Notice that I changed the other variable in which you pointed out this > issue, because it is not checked against zero anywhere. Please do so anyways. >> > +static grub_err_t install_int13_handler (void); >> > + >> > +/* Puts the specified mapping into the table, replacing an existing >> > mapping >> > + for newdrive or adding a new one if required. */ >> > +static grub_err_t >> > +drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto) >> > + >> >> Please do not add a newline here. > Oops, sorry. I forgot to remove it when moving the comment :-) >> > + drivemap_node_t *mapping = 0; >> > + drivemap_node_t *search = drivemap; >> > + while (search) >> > + { >> > + if (search->newdrive == newdrive) >> > + { >> > + mapping = search; >> > + break; >> > + } >> > + search = search->next; >> > + } >> > + >> > + >> > + /* Check for pre-existing mappings to modify before creating a new one. >> > */ >> > + if (mapping) >> > + mapping->redirto = redirto; >> > + else >> > + { >> > + mapping = grub_malloc (sizeof (drivemap_node_t)); >> > + if (!mapping) >> > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, >> > + "cannot allocate map entry, not enough >> > memory"); >> > + mapping->newdrive = newdrive; >> > + mapping->redirto = redirto; >> > + mapping->next = drivemap; >> > + drivemap = mapping; >> > + } >> > + return GRUB_ERR_NONE; >> > +} >> > + >> > +/* Removes the mapping for newdrive from the table. If there is no >> > mapping, >> > + then this function behaves like a no-op on the map. */ >> > +static void >> > +drivemap_remove (grub_uint8_t newdrive) >> > +{ >> > + drivemap_node_t *mapping = 0; >> > + drivemap_node_t *search = drivemap; >> > + drivemap_node_t *previous = 0; >> > + while (search) >> > + { >> > + if (search->newdrive == newdrive) >> > + { >> > + mapping = search; >> > + break; >> > + } >> > + previous = search; >> > + search = search->next; >> > + } >> > + if (mapping) /* Found. */ >> >> You forgot one. > Corrected. Sorry. >> >> > + { >> > + if (previous) >> > + previous->next = mapping->next; >> > + else /* Entry was head of list. */ >> > + drivemap = mapping->next; >> > + grub_free (mapping); >> > + } >> > +} >> > + >> > +/* Given a device name, resolves its BIOS disk number and stores it in the >> > + passed location, which should only be trusted if ERR_NONE is returned. >> > */ >> > +static grub_err_t >> > +parse_biosdisk (const char *name, grub_uint8_t *disknum) >> > +{ >> > + grub_disk_t disk; >> > + if (!name || 0 == *name) >> > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty"); >> > + /* Skip the first ( in (hd0) - disk_open wants just the name. */ >> > + if (*name == '(') >> > + name++; >> > + >> > + disk = grub_disk_open (name); >> > + if (!disk) >> > + return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"", >> > name); >> > + else >> > + { >> > + const enum grub_disk_dev_id id = disk->dev->id; >> > + /* The following assignment is only sound if the device is indeed a >> > + biosdisk. The caller must check the return value. */ >> > + if (disknum) >> > + *disknum = disk->id; >> > + grub_disk_close (disk); >> > + if (GRUB_DISK_DEVICE_BIOSDISK_ID == id) >> > + return GRUB_ERR_NONE; >> > + else return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS >> > disk", name); >> > + } >> > +} >> > + >> > +/* Given a BIOS disk number, returns its GRUB device name if it exists. >> > + For nonexisting BIOS dnums, this function returns ERR_UNKNOWN_DEVICE. >> > */ >> >> This is GRUB_ERR_UNKNOWN_DEVICE > I know, I consciously left the GRUB_ part out because 1) it would > require the line to be split and 2) that prefix is all over the place. > Corrected, however. >> >> > +static grub_err_t >> > +revparse_biosdisk(const grub_uint8_t dnum, const char **output) >> > +{ >> > + int found = 0; >> > + auto int find (const char *name); >> > + int find (const char *name) >> > + { >> > + const grub_disk_t disk = grub_disk_open (name); >> > + if (!disk) >> > + return 0; >> > + else >> > + { >> > + if (disk->id == dnum && GRUB_DISK_DEVICE_BIOSDISK_ID == >> > disk->dev->id) >> > + { >> > + found = 1; >> > + if (output) >> > + *output = name; >> > + } >> > + grub_disk_close (disk); >> > + return found; >> > + } >> > + } >> > + >> > + grub_disk_dev_iterate (find); >> > + if (found) >> > + return GRUB_ERR_NONE; >> > + else return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %d not >> > found", dnum); >> >> Please change this. > Em... to what? What is the problem? Do you want me to reverse the > comparison? i.e. if (!found) { return error; } else { return ok; } The return is on the same line as the else. >> > +/* Given a GRUB-like device name and a convenient location, stores the >> > related >> > + BIOS disk number. Accepts devices like \((f|h)dN\), with 0 <= N < >> > 128. */ >> > +static grub_err_t >> > +tryparse_diskstring (const char *str, grub_uint8_t *output) >> > +{ >> > + if (!str || 0 == *str) >> > + goto fail; >> > + /* Skip opening paren in order to allow both (hd0) and hd0. */ >> > + if (*str == '(') >> > + str++; >> > + if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd') >> > + { >> > + grub_uint8_t bios_num = (str[0] == 'h')? 0x80 : 0x00; >> > + grub_errno = GRUB_ERR_NONE; >> > + unsigned long drivenum = grub_strtoul (str + 2, 0, 0); >> > + if (grub_errno != GRUB_ERR_NONE || drivenum > 127) >> > + /* N not a number or out of range */ >> > + goto fail; >> >> Can you put this between braces, now comment was added. > Done. >> >> > + else >> > + { >> > + bios_num |= drivenum; >> > + if (output) >> > + *output = bios_num; >> > + return GRUB_ERR_NONE; >> > + } >> > + } >> > + else goto fail; >> >> ... > What's the problem here? The lack of braces? The goto (as used in the > ext2 code)? goto is on the same line as the else. >> > +fail: >> > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" >> > invalid: must" >> > + "be (f|h)dN, with 0 <= N < 128", str); >> > +} >> > + >> > +static grub_err_t >> > +grub_cmd_drivemap (struct grub_arg_list *state, int argc, char **args) >> > +{ >> > + if (state[0].set) >> > + { >> > + /* Show: list mappings. */ >> > + if (!drivemap) >> > + grub_printf ("No drives have been remapped"); >> > + else >> > + { >> > + grub_printf ("Showing only remapped drives.\n"); >> > + grub_printf ("Mapped\tGRUB\n"); >> > + drivemap_node_t *curnode = drivemap; >> > + while (curnode) >> > + { >> > + const char *dname = 0; >> > + grub_err_t err = revparse_biosdisk (curnode->redirto, >> > &dname); >> > + if (err != GRUB_ERR_NONE) >> > + return grub_error (err, "invalid mapping: non-existent >> > disk" >> > + "or not managed by the BIOS"); >> > + grub_printf("0x%02x\t%4s\n", curnode->newdrive, dname); >> > + curnode = curnode->next; >> > + } >> > + } >> > + } >> > + else if (state[1].set) >> > + { >> > + /* Reset: just delete all mappings, freeing their memory. */ >> > + drivemap_node_t *curnode = drivemap; >> > + drivemap_node_t *prevnode = 0; >> > + while (curnode) >> > + { >> > + prevnode = curnode; >> > + curnode = curnode->next; >> > + grub_free (prevnode); >> > + } >> > + drivemap = 0; >> > + } >> > + else >> > + { >> > + /* Neither flag: put mapping */ >> >> ". */ > Done >> >> > + grub_uint8_t mapfrom = 0; >> > + grub_uint8_t mapto = 0xFF; >> > + grub_err_t err; >> > + >> > + if (argc != 2) >> > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments >> > required"); >> > + >> > + err = parse_biosdisk (args[0], &mapfrom); >> > + if (err != GRUB_ERR_NONE) >> > + return err; >> > + >> > + err = tryparse_diskstring (args[1], &mapto); >> > + if (err != GRUB_ERR_NONE) /* Not a disk string. Maybe a raw num >> > then? */ >> >> Please move this up. > Done. >> >> > + { >> > + grub_errno = GRUB_ERR_NONE; >> > + unsigned long num = grub_strtoul (args[1], 0, 0); >> > + if (grub_errno != GRUB_ERR_NONE || num > 0xFF) /* Not a raw >> > num or too high. */ >> > + return grub_error (grub_errno, >> > + "Target specifier must be of the form (fdN) >> > or " >> > + "(hdN), with 0 <= N < 128; or a plain >> > dec/hex " >> > + "number between 0 and 255"); >> > + else mapto = (grub_uint8_t)num; >> > + } >> > + >> > + if (mapto == mapfrom) /* Reset to default. */ >> >> Same here. > Done. >> >> > + { >> > + grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)", >> > args[0], mapfrom); >> > + drivemap_remove (mapfrom); >> > + } >> > + else /* Map. */ >> >> Please move the comment inside the braces below. > Done, and reworded. >> >> > + { >> > + grub_dprintf (MODNAME, "Mapping %s (%02x) to %02x\n", args[0], >> > mapfrom, mapto); >> > + return drivemap_set ((grub_uint8_t)mapto, mapfrom); >> > + } >> > + } >> > + >> > + return GRUB_ERR_NONE; >> > +} >> > + >> > +typedef struct __attribute__ ((packed)) int13map_node >> > +{ >> > + grub_uint8_t disknum; >> > + grub_uint8_t mapto; >> > +} int13map_node_t; >> > + >> > +/* The min amount of mem that must remain free after installing the >> > handler. >> > + 32 KiB is just above 0x7C00-0x7E00, where the bootsector is loaded. */ >> > +#define MIN_FREE_MEM_KB 32 >> > +#define INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) - >> > ((grub_uint8_t*)&grub_drivemap_int13_handler_base) ) >> > +#define INT13H_REBASE(x) ( (void*) (((grub_uint8_t*)handler_base) + (x)) ) >> > +#define INT13H_TONEWADDR(x) INT13H_REBASE( INT13H_OFFSET( x ) ) >> > + >> > +/* Int13h handler installer - reserves conventional memory for the >> > handler, >> > + copies it over and sets the IVT entry for int13h. >> > + This code rests on the assumption that GRUB does not activate any kind >> > of >> > + memory mapping apart from identity paging, since it accesses realmode >> > + structures by their absolute addresses, like the IVT at 0 or the BDA at >> > + 0x400; and transforms a pmode pointer into a rmode seg:off far ptr. */ >> > +static grub_err_t >> > +install_int13_handler (void) >> > +{ >> > + grub_size_t entries = 0; >> > + drivemap_node_t *curentry = drivemap; >> > + while (curentry) /* Count entries to prepare a contiguous map block. >> > */ >> >> ... > Comment moved up. >> >> > + { >> > + entries++; >> > + curentry = curentry->next; >> > + } >> > + if (0 == entries) >> >> I know this is what you prefer, but can you change this nevertheless? > I refer to my objection near the top of the post. I know you object, but did you change it? >> > + grub_dprintf (MODNAME, "No drives marked as remapped, installation >> > of" >> > + "an int13h handler is not required."); >> > + return GRUB_ERR_NONE; /* No need to install the int13h handler. */ >> > + } >> > + else >> > + { >> > + grub_dprintf (MODNAME, "Installing int13h handler...\n"); >> > + grub_uint32_t *ivtslot = (grub_uint32_t*)0x0000004c; >> > + >> > + /* Save the pointer to the old int13h handler. */ >> > + grub_drivemap_int13_oldhandler = *ivtslot; >> > + grub_dprintf (MODNAME, "Old int13 handler at %04x:%04x\n", >> > + (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff, >> > + grub_drivemap_int13_oldhandler & 0x0ffff); >> > + >> > + /* Reserve a section of conventional memory as "BIOS memory" for >> > handler: >> > + BDA offset 0x13 contains the top of such memory. */ >> > + grub_uint16_t *bpa_freekb = (grub_uint16_t*)0x00000413; >> > + grub_dprintf (MODNAME, "Top of conventional memory: %u KiB\n", >> > *bpa_freekb); >> > + grub_size_t total_size = grub_drivemap_int13_size >> > + + (entries + 1) * sizeof(int13map_node_t); >> > + grub_uint16_t payload_sizekb = (total_size >> 10) + >> > + (((total_size % 1024) == 0) ? 0 : 1); >> > + if ((*bpa_freekb - payload_sizekb) < MIN_FREE_MEM_KB) >> > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "refusing to install" >> > + "int13 handler, not enough free memory after"); >> > + grub_dprintf (MODNAME, "Payload is %u b long, reserving %u Kb\n", >> > + total_size, payload_sizekb); >> > + *bpa_freekb -= payload_sizekb; >> > + >> > + /* Copy int13h handler chunk to reserved area. */ >> > + grub_uint8_t *handler_base = (grub_uint8_t*)(*bpa_freekb << 10); >> > + grub_dprintf (MODNAME, "Copying int13 handler to: %p\n", >> > handler_base); >> > + grub_memcpy (handler_base, &grub_drivemap_int13_handler_base, >> > + grub_drivemap_int13_size); >> > + >> > + /* Copy the mappings to the reserved area. */ >> > + curentry = drivemap; >> > + grub_size_t i; >> > + int13map_node_t *handler_map = (int13map_node_t*) >> > + INT13H_TONEWADDR (&grub_drivemap_int13_mapstart); >> > + grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n", >> > handler_map); >> > + for (i = 0; i < entries && curentry; i++, curentry = curentry->next) >> > + { >> > + handler_map[i].disknum = curentry->newdrive; >> > + handler_map[i].mapto = curentry->redirto; >> > + grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x\n", i, >> > + handler_map[i].disknum, >> > handler_map[i].mapto); >> > + } >> > + /* Signal end-of-map. */ >> > + handler_map[i].disknum = 0; >> > + handler_map[i].mapto = 0; >> > + grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x (end)\n", i, >> > + handler_map[i].disknum, >> > handler_map[i].mapto); >> > + >> > + /* Install our function as the int13h handler in the IVT. */ >> > + grub_uint32_t ivtentry = ((grub_uint32_t)handler_base) << 12; /* >> > Segment address. */ >> > + ivtentry |= (grub_uint16_t) >> > INT13H_OFFSET(grub_drivemap_int13_handler); >> > + grub_dprintf (MODNAME, "New int13 handler IVT pointer: %04x:%04x\n", >> > + (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff); >> > + *ivtslot = ivtentry; >> > + >> > + return GRUB_ERR_NONE; >> > + } >> > +} >> > + >> > +GRUB_MOD_INIT (drivemap) >> > +{ >> > + (void) mod; /* Stop warning. */ >> > + grub_register_command (MODNAME, grub_cmd_drivemap, >> > + GRUB_COMMAND_FLAG_BOTH, >> > + MODNAME " -s | -r | (hdX) >> > newdrivenum", >> > + "Manage the BIOS drive mappings", options); >> > + insthandler_hook = grub_loader_register_preboot >> > (&install_int13_handler, 1); >> > +} >> > + >> > +GRUB_MOD_FINI (drivemap) >> > +{ >> > + grub_loader_unregister_preboot (insthandler_hook); >> > + insthandler_hook = 0; >> > + grub_unregister_command (MODNAME); >> > +} >> > + >> > Index: commands/i386/pc/drivemap_int13h.S >> > =================================================================== >> > --- commands/i386/pc/drivemap_int13h.S (revisión: 0) >> > +++ commands/i386/pc/drivemap_int13h.S (revisión: 0) >> > @@ -0,0 +1,118 @@ >> > +/* >> > + * GRUB -- GRand Unified Bootloader >> > + * Copyright (C) 1999,2000,2001,2002,2003,2005,2006,2007,2008 Free >> > Software Foundation, Inc. >> > + * >> > + * GRUB is free software: you can redistribute it and/or modify >> > + * it under the terms of the GNU General Public License as published by >> > + * the Free Software Foundation, either version 3 of the License, or >> > + * (at your option) any later version. >> > + * >> > + * GRUB is distributed in the hope that it will be useful, >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > + * GNU General Public License for more details. >> > + * >> > + * You should have received a copy of the GNU General Public License >> > + * along with GRUB. If not, see <http://www.gnu.org/licenses/>. >> > + */ >> > + >> > + >> > +/* >> > + * Note: These functions defined in this file may be called from C. >> > + * Be careful of that you must not modify some registers. Quote >> > + * from gcc-2.95.2/gcc/config/i386/i386.h: >> > + >> > + 1 for registers not available across function calls. >> > + These must include the FIXED_REGISTERS and also any >> > + registers that can be used without being saved. >> > + The latter must include the registers where values are returned >> > + and the register where structure-value addresses are passed. >> > + Aside from that, you can include as many other registers as you like. >> > + >> > + ax,dx,cx,bx,si,di,bp,sp,st,st1,st2,st3,st4,st5,st6,st7,arg >> > +{ 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 } >> > + */ >> > + >> > +/* >> > + * Note: GRUB is compiled with the options -mrtd and -mregparm=3. >> > + * So the first three arguments are passed in %eax, %edx, and %ecx, >> > + * respectively, and if a function has a fixed number of arguments >> > + * and the number if greater than three, the function must return >> > + * with "ret $N" where N is ((the number of arguments) - 3) * 4. >> > + */ >> > + >> > +#include <grub/symbol.h> >> > + >> > +#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - >> > grub_drivemap_int13_handler_base) >> > + >> > +/* Copy starts here. When deployed, this label must be segment-aligned */ >> > +VARIABLE(grub_drivemap_int13_handler_base) >> > + >> > +VARIABLE(grub_drivemap_int13_oldhandler) >> > + .word 0xdead, 0xbeef >> > +/* Drivemap module - INT 13h handler - BIOS HD map */ >> > +/* We need to use relative addressing, and with CS to top it all, since we >> > + must make as few changes to the registers as possible. IP-relative >> > + addressing like on amd64 would make life way easier here. */ >> > +.code16 >> > +FUNCTION(grub_drivemap_int13_handler) >> > + push %bp >> > + mov %sp, %bp >> > + push %ax /* We'll need it later to determine the used BIOS function */ >> > + >> > + /* Map the drive number (always in DL?) */ >> > + push %ax >> > + push %bx >> > + push %si >> > + mov $GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx >> > + xor %si, %si >> > +1:movw %cs:(%bx,%si), %ax >> > + cmp %ah, %al >> > + jz 3f /* DRV=DST => map end - drive not remapped, leave DL as-is */ >> > + cmp %dl, %al >> > + jz 2f /* Found - drive remapped, modify DL */ >> > + add $2, %si >> > + jmp 1b /* Not found, but more remaining, loop */ >> > +2:mov %ah, %dl >> > +3:pop %si >> > + pop %bx >> > + xchgw %ax, -4(%bp) /* Recover the old AX and save the map entry for >> > later */ >> > + >> > + push %bp >> > + /* Simulate interrupt call: push flags and do a far call in order to set >> > + the stack the way the old handler expects it so that its iret works >> > */ >> > + push 6(%bp) >> > + movw (%bp), %bp /* Restore the caller BP (is this needed and/or >> > sensible?) */ >> > + lcall *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler) >> > + pop %bp /* The pushed flags were removed by iret */ >> > + /* Set the saved flags to what the int13h handler returned */ >> > + push %ax >> > + pushf >> > + pop %ax >> > + movw %ax, 6(%bp) >> > + pop %ax >> > + >> > + /* Reverse map any returned drive number if the data returned includes >> > it. >> > + The only func that does this seems to be origAH = 0x08, but many BIOS >> > + refs say retDL = # of drives connected. However, the GRUB Legacy >> > code >> > + treats this as the _drive number_ and "undoes" the remapping. Thus, >> > + this section has been disabled for testing if it's required */ >> > +# cmpb $0x08, -1(%bp) /* Caller's AH */ >> > +# jne 4f >> > +# xchgw %ax, -4(%bp) /* Map entry used */ >> > +# cmp %ah, %al /* DRV=DST => drive not remapped */ >> > +# je 4f >> > +# mov %ah, %dl /* Undo remap */ >> > + >> > +4:mov %bp, %sp >> > + pop %bp >> > + iret >> > +/* This label MUST be at the end of the copied block, since the installer >> > code >> > + reserves additional space for mappings at runtime and copies them over >> > it */ >> > +.align 2 >> > +VARIABLE(grub_drivemap_int13_mapstart) >> > +/* Copy stops here */ >> > +.code32 >> > +VARIABLE(grub_drivemap_int13_size) >> > + .word GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_size) >> > + >> > Index: conf/i386-pc.rmk >> > =================================================================== >> > --- conf/i386-pc.rmk (revisión: 1766) >> > +++ conf/i386-pc.rmk (copia de trabajo) >> > @@ -158,7 +158,7 @@ >> > vbe.mod vbetest.mod vbeinfo.mod video.mod gfxterm.mod \ >> > videotest.mod play.mod bitmap.mod tga.mod cpuid.mod serial.mod \ >> > ata.mod vga.mod memdisk.mod jpeg.mod png.mod pci.mod lspci.mod \ >> > - aout.mod _bsd.mod bsd.mod >> > + aout.mod _bsd.mod bsd.mod drivemap.mod >> > >> > # For biosdisk.mod. >> > biosdisk_mod_SOURCES = disk/i386/pc/biosdisk.c >> > @@ -325,4 +325,11 @@ >> > bsd_mod_CFLAGS = $(COMMON_CFLAGS) >> > bsd_mod_LDFLAGS = $(COMMON_LDFLAGS) >> > >> > +# For drivemap.mod. >> > +drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \ >> > + commands/i386/pc/drivemap_int13h.S >> > +drivemap_mod_ASFLAGS = $(COMMON_ASFLAGS) >> > +drivemap_mod_CFLAGS = $(COMMON_CFLAGS) >> > +drivemap_mod_LDFLAGS = $(COMMON_LDFLAGS) >> > + >> > include $(srcdir)/conf/common.mk >> > Index: include/grub/loader.h >> > =================================================================== >> > --- include/grub/loader.h (revisión: 1766) >> > +++ include/grub/loader.h (copia de trabajo) >> > @@ -37,6 +37,22 @@ >> > /* Unset current loader, if any. */ >> > void EXPORT_FUNC(grub_loader_unset) (void); >> > >> > +typedef struct hooklist_node *grub_preboot_hookid; >> > + >> > +/* Register a function to be called before the boot hook. Returns an id >> > that >> > + can be later used to unregister the preboot (i.e. on module unload). >> > If >> > + abort_on_error is set, the boot sequence will abort if any of the >> > registered >> > + functions return anything else than GRUB_ERR_NONE. >> > + On error, the return value will compare equal to 0 and the error >> > information >> > + will be available in errno and errmsg. However, if the call is >> > successful >> > + those variables are _not_ modified. */ >> >> No need to mention errmsg, it's internal to GRUB. As for errno (which >> is grub_errno, actually) it does not need to be mentioned, otherwise >> we would have to do so everywhere. Please capitalize HOOK and >> ABORT_ON_ERROR in the comments above. > Done. "hook" removed because it referred to the loader module boot > function. >> >> > +grub_preboot_hookid EXPORT_FUNC(grub_loader_register_preboot) >> > + (grub_err_t (*hook) (void), int abort_on_error); >> > + >> > +/* Unregister a preboot hook by the id returned by >> > loader_register_preboot. >> > + This functions becomes a no-op if no such function is registered */ >> > +void EXPORT_FUNC(grub_loader_unregister_preboot) (grub_preboot_hookid id); >> > + >> > /* Call the boot hook in current loader. This may or may not return, >> > depending on the setting by grub_loader_set. */ >> >> Nitpick: "loader. This..." > Are you a bot? ¬¬ Corrected It would make like much simpler if I were ;-). What makes you think so? >> > grub_err_t EXPORT_FUNC(grub_loader_boot) (void); >> > Index: kern/loader.c >> > =================================================================== >> > --- kern/loader.c (revisión: 1766) >> > +++ kern/loader.c (copia de trabajo) >> > @@ -61,11 +61,82 @@ >> > grub_loader_loaded = 0; >> > } >> > >> > +struct hooklist_node >> > +{ >> > + grub_err_t (*hook) (void); >> > + int abort_on_error; >> > + struct hooklist_node *next; >> > +}; >> > + >> > +static struct hooklist_node *preboot_hooks = 0; >> > + >> > +grub_preboot_hookid >> > +grub_loader_register_preboot(grub_err_t (*hook) (void), int >> > abort_on_error) >> > +{ >> > + if (!hook) >> > + { >> > + grub_error (GRUB_ERR_BAD_ARGUMENT, "preboot hook must not be NULL"); >> > + return 0; >> > + } >> > + grub_preboot_hookid newentry = grub_malloc (sizeof (struct >> > hooklist_node)); >> >> Mixed declarations/code. > Oops, sorry. I put most of my attention on drivemap.c (and even then > many comments slipped through). Corrected. Please re-check them, I might have missed things this time... >> > + if (!newentry) >> > + { >> > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot alloc a hookinfo >> > structure"); >> > + return 0; >> > + } >> > + else >> > + { >> > + newentry->hook = hook; >> > + newentry->abort_on_error = abort_on_error; >> > + newentry->next = preboot_hooks; >> > + preboot_hooks = newentry; >> > + return newentry; >> > + } >> > +} >> > + >> > +void >> > +grub_loader_unregister_preboot(grub_preboot_hookid id) >> >> "preboot (grub" > Corrected on both functions ;) >> >> > +{ >> > + grub_preboot_hookid entry = 0; >> > + grub_preboot_hookid search = preboot_hooks; >> > + grub_preboot_hookid previous = 0; >> > + >> > + if (0 == id) >> > + return; >> >> ... > ... xD >> >> > + while (search) >> > + { >> > + if (search == id) >> > + { >> > + entry = search; >> > + break; >> > + } >> > + previous = search; >> > + search = search->next; >> > + } >> > + if (entry) /* Found */ >> >> ... > Comment removed, was unnecessary. >> >> > + { >> > + if (previous) >> > + previous->next = entry->next; >> > + else preboot_hooks = entry->next; /* Entry was head of list */ >> > + grub_free (entry); >> > + } >> > +} >> > + >> > grub_err_t >> > grub_loader_boot (void) >> > { >> > if (! grub_loader_loaded) >> > return grub_error (GRUB_ERR_NO_KERNEL, "no loaded kernel"); >> > + >> > + grub_preboot_hookid entry = preboot_hooks; >> >> Mixed declarations/code. > Moved the whole line up. >> >> > + while (entry) >> > + { >> > + grub_err_t possible_error = entry->hook(); >> > + if (possible_error != GRUB_ERR_NONE && entry->abort_on_error) >> > + return possible_error; >> > + entry = entry->next; >> > + } >> > >> > if (grub_loader_noreturn) >> > grub_machine_fini (); _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel