From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1KKjgr-00033q-Dt for mharc-grub-devel@gnu.org; Sun, 20 Jul 2008 20:55:49 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KKjgo-00033Y-WB for grub-devel@gnu.org; Sun, 20 Jul 2008 20:55:47 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KKjgl-00033M-0G for grub-devel@gnu.org; Sun, 20 Jul 2008 20:55:45 -0400 Received: from [199.232.76.173] (port=60838 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KKjgk-00033J-R3 for grub-devel@gnu.org; Sun, 20 Jul 2008 20:55:42 -0400 Received: from nf-out-0910.google.com ([64.233.182.189]:60710) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KKjgk-0001pO-6F for grub-devel@gnu.org; Sun, 20 Jul 2008 20:55:42 -0400 Received: by nf-out-0910.google.com with SMTP id c7so4107085nfi.26 for ; Sun, 20 Jul 2008 17:55:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:subject:from:to:in-reply-to :references:content-type:date:message-id:mime-version:x-mailer; bh=IY65440DdYbK25PmPar33l+K9WCeY3jMES7PHqVwlPU=; b=hDHRba+bjvUW4NeN+yRq8t5YiMfLPgkRSAhMIIY3U66ltpM/cIdzhfPrG6XBcBe6X6 7Nrxtjz1sUiccwykSJGYXwmQEZ3B7jWpaSZaTwzxUZewmhoGZRcdtSsFPg/YRJtO4Lcc 0g44S4Ck4oOr8WT5kpaDQ62i9bxmCJPCfCAkc= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:in-reply-to:references:content-type:date:message-id :mime-version:x-mailer; b=CUEBi6umhwkj22kpXQQv3RWXLxQ1IbrIMmLl+3RQmnC/SVP6yyG0nbow5lyEioKoCj BFmoBuC/P2Rm1RFFN0lW1J5eNXKy615PLps1Po0aO0ftjxonuYd6Vhk3NgEwGiBrVArv pdwxADePY722+sCwAJNyQ3C4fAiq+maKWq9k8= Received: by 10.210.28.4 with SMTP id b4mr2704085ebb.40.1216601740371; Sun, 20 Jul 2008 17:55:40 -0700 (PDT) Received: from ?192.168.1.100? ( [213.37.137.93]) by mx.google.com with ESMTPS id y37sm6550921iky.8.2008.07.20.17.55.36 (version=SSLv3 cipher=RC4-MD5); Sun, 20 Jul 2008 17:55:38 -0700 (PDT) From: Javier =?ISO-8859-1?Q?Mart=EDn?= To: The development of GRUB 2 In-Reply-To: <87bq0sibu0.fsf@xs4all.nl> References: <1215137528.26019.58.camel@localhost> <87bq0sibu0.fsf@xs4all.nl> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-ASKs9eUn3+M9QCq/5v2X" Date: Mon, 21 Jul 2008 02:55:41 +0200 Message-Id: <1216601741.8334.122.camel@localhost> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 X-detected-kernel: by monty-python.gnu.org: Linux 2.6 (newer, 2) Subject: Re: [PATCH] Drivemap module X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Jul 2008 00:55:47 -0000 --=-ASKs9eUn3+M9QCq/5v2X Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable El dom, 20-07-2008 a las 21:40 +0200, Marco Gerards escribi=C3=B3: > Did you use code from other people or projects? No, as far as I can control my own mind: the assembly int13h handler is loosely based on that of GRUB Legacy, but heavily rewritten. All other code was written from scratch, even the crappy linked lists all over the place. >=20 > > For newcomers, full info on the patch is available on the list archives > > - it was proposed on June and its discussion deferred for "two or three > > weeks" because the developers were busy. >=20 >=20 > I have copied the changelog entry from your other e-mail: >=20 > * commands/i386/pc/drivemap.c : New file, main part of the new > drivemap module allowing BIOS drive remapping not unlike the > legacy "map" command. This allows to boot OSes with boot-time > dependencies on the particular ordering of BIOS drives or > trusting their own to be 0x80, like Windows XP, with > non-standard boot configurations. >=20 > "New file." would be sufficient >=20 > * commands/i386/pc/drivemap_int13h.S : New file, INT 13h handler > for the drivemap module. Installed as a TSR routine by > drivemap.c, performs the actual redirection of BIOS drives. >=20 > Same here. Hmm... isn't the ChangeLog too spartan? I thought it should be a bit informative - what about a single sentence per file? * commands/i386/pc/drivemap.c : New file - drivemap command and int13h installer * commands/i386/pc/drivemap_int13h.S : New file, resident real mode BIOS int13 handler >=EF=BB=BF * conf/i386-pc.rmk : Added the new module > Please say which variables you added in this file. You can find some > examples on how to do this in the ChangeLog file. * conf/i386-pc.rmk (pkglib_MODULES) : Added drivemap.mod (drivemap_mod_SOURCES) : New variable (drivemap_mod_ASFLAGS) : Likewise =EF=BB=BF(drivemap_mod_CFLAGS) : Likewise =EF=BB=BF(drivemap_mod_LDFLAGS) : Likewise And now we're being uselessly verbose. IMHO, ChangeLog should be more about semantic changes in the code and less about literal changes - we have `svn diff' for those. >=20 > * include/grub/loader.h : Added a "just-before-boot" callback > infrastructure used by drivemap.mod to install the INT13 handler > only when the "boot" command has been issued. >=20 > Please describe changes, not effects. So which prototypes and macros > did you add? >=20 * include/grub/loader.h (grub_loader_register_preboot) : New function (proto). Register a new pre-boot handler (grub_loader_unregister_preboot) : Likewise. Unregister handler (grub_preboot_hookid) : New typedef. Registered hook "handle" > * kern/loader.c : Implement the preboot-hook described >=20 > Which functions did you change and how? Please describe actual changes. * kern/loader.c =EF=BB=BF(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 >=20 > The header is missing, please include it. Also newlines between the > files make it easier to read. What header? The drivemap module itself has no .h files. The only header I touch is loader.h, and is both in the ChangeLog entry and the patch. >=20 >=20 > Here follows a review. Sorry I kept you waiting for this long, this > feature and your work is really appreciated! Perhaps I can spot some > more problems after you fixed it and supplied an updated changelog > entry. There are quite some comments, but please do not let this > demotivate you, it is mainly coding style related :-) Well, thanks to all the time I had free, I have nearly finished Final Fantasy XII, so the wait was not soo bad ^^ > (...) > > + > > +/* Uncomment the following line to enable debugging output */ > > +/* #define DRIVEMAP_DEBUG */ > > + > > +#ifdef DRIVEMAP_DEBUG > > +# define DBG_PRINTF(...) grub_printf(__VA_ARGS__) > > +#else > > +# define DBG_PRINTF(...) > > +#endif >=20 > Please use the grub_dprintf infrastructure. Done. I didn't even know it existed :S >=20 > > +/* realmode far ptr =3D 2 * 16b */ > > +extern grub_uint32_t EXPORT_VAR (grub_drivemap_int13_oldhandler); > > +/* Size of the section to be copied */ > > +extern grub_uint16_t EXPORT_VAR (grub_drivemap_int13_size); > > + > > +/* NOT a typo - just need the symbol's address with &symbol */ > > +typedef void grub_symbol_t; > > +extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_handler_base); > > +extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_mapstart); >=20 > Please export stuff in header files, that's the normal practise in > this file as well, right? What's not a typo? EXPORT_* macros removed; seemingly they are no longer needed because all code is in the same module (initially the assembly was in kernel).=20 What's not a typo is the definition of grub_symbol_t as "void" instead of something more sound to a C programmer, like "void *". I don't really know how to explain it in the source, but it's just a way to make the names known to the C compiler, so that I then take their addresses with the & operator. Such names are _not_ variables in the C file and they take no space or storage. >=20 > > +static grub_preboot_hookid insthandler_hook =3D 0; >=20 > You can leave the "=3D 0" away. > (...) > > +static drivemap_node_t *drivemap =3D 0; >=20 > Same here :-) Does GRUB zero-initialize the memory of modules when loading them? The first variable would be OK without the =3D 0, but not the second, since there are some checks of the form if(var) scattered all over the code - "drivemap" is a linked list, so 0 means EOL. > (...) > > +static grub_err_t > > +drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto) > > + /* Puts the specified mapping into the table, replacing an existing = mapping > > + * for newdrive or adding a new one if required. */ >=20 > Please place the comments before the function. > (...) > > + if (mapping) /* There was a mapping already in place, modify it */ > > + mapping->redirto =3D redirto; >=20 > Please place the comment before the if statement. Can you format the > comment such that it begins with a capital and ends with ". */" (two > spaces)? Can you do this for the other comments you added as well? Hmm... Is the formatting requirement due to some script-based code parsing or is it just OCD? ;) Besides, saying "there was a mapping" before the "if" that checks it would be strange. I'd prefer to leave this line as is, or find an alternate wording for the comment (and I'm sleepy right now) > > + else /* Create a new mapping and add it to the head of the list */ > > + { >=20 > I would move the comment inside the braces. But I wouldn't - the "else" line is almost empty, and placing the comment two lines below does not help readability > (...) > > +static void > > +drivemap_remove (grub_uint8_t newdrive) > > + /* Removes the mapping for newdrive from the table. If there is no m= apping, > > + * then this function behaves like a no-op on the map. */ >=20 > Can you move the comments to before the function? Please do not place > *'s on each line. Without the second * it would be fine. After a > ".", please insert two spaces. After _every_ "." or just after the final one? The "no * on each line" rule conflicts with the GPL notice comment, but OK. > > +{ > > + drivemap_node_t *mapping =3D 0, *search =3D drivemap, *previous =3D = 0; >=20 > Please define one variable per line, can you break this down to 3 > lines? I think the current version is more readable, but OK. > (...) > > + else drivemap =3D mapping->next; /* Entry was head of list */ >=20 > Please place the stuff after "else" on a new line. Done, I think. >=20 > > + grub_free (mapping); > > + } > > +} > > + > > +static grub_err_t > > +parse_biosdisk (const char *name, grub_uint8_t *disknum) > > +{ > > + if (!name) return GRUB_ERR_BAD_ARGUMENT; >=20 > Same here. Please do not just return GRUB_ERR_BAD_ARGUMENT, use > grub_error so you can also define a string for the error. This is a function internal to drivemap, called from a single point. I don't think the additional information gained by the changes you suggest would be worth the space lost by two more error strings (of your additional suggestions for this function). > > + if (*name =3D=3D '(') > > + name++; /* Skip the first ( in (hd0) - disk_open wants just the na= me! */ > > + grub_disk_t disk =3D grub_disk_open (name); >=20 > Although mixed declarations and code are allowed, I personally would > avoid this. It doesn't make the code easier to read, I think. What do you advocate, then? Declaring "disk" at the function start (three lines above) and then assigning to it? Hmm... maybe, but I find this version more sound. The function is small enough to avoid any confusion. >=20 > > + else > > + { > > + enum grub_disk_dev_id id =3D disk->dev->id; > > + if (disknum) > > + *disknum =3D disk->id; /* Only valid, of course if it's a bi= osdisk */ >=20 > Please place the comment before the if. The comment does not refer to the "if", but to the assignment: the "BIOS" drive num we get is only meaningful if the disk is actually managed by biosdisk (which is checked later on return). All other solutions (i.e. checking before) expand that section of the code by a few lines. > (...) > > + if (dnum =3D=3D disk->id && GRUB_DISK_DEVICE_BIOSDISK_ID =3D= =3D disk->dev->id) >=20 >=20 > This is not rally wrong, but doesn't feel right. I would use: >=20 > disk->dev->id =3D=3D GRUB_DISK_DEVICE_BIOSDISK_ID I find it more sound to place the constant before in the comparison, since if the code is modified later and the =3D=3D is erroneously turned into =3D, my code would throw a compiler error (assignment to constant) while yours would happily proceed as a normal assignment inside the if, which is perfectly allowed in C, and fail at runtime. >=20 > I know, I am a pain in the ass right now :P I won't deny that, but criticism (particularly constructive criticism as yours) is a Good Thing (tm). > (...) > > + grub_disk_dev_iterate (&find); >=20 > No need for the &. I even wonder if it would work this way... Corrected. > > + return ret; > > +} > > + > > +static grub_err_t > > +tryparse_diskstring (const char *str, grub_uint8_t *output) > > +{ > > + if (!str) return GRUB_ERR_BAD_ARGUMENT; > > + if (*str =3D=3D '(') > > + str++; /* Skip opening paren in order to allow both (hd0) and hd0= */ > > + if ((str[0] =3D=3D 'f' || str[0] =3D=3D 'h') && str[1] =3D=3D 'd') > > + { > > + grub_uint8_t bios_num =3D (str[0] =3D=3D 'h')? 0x80 : 0x00; > > + grub_errno =3D GRUB_ERR_NONE; > > + unsigned long drivenum =3D grub_strtoul (str + 2, &str, 0); > > + if (grub_errno !=3D GRUB_ERR_NONE || drivenum > 127) > > + return GRUB_ERR_BAD_ARGUMENT; /* Could not convert, or num too= high for BIOS */ >=20 > Use grub_error Same response as before: internal helper function, used at a single point with known error conditions which should be obvious. Moreover, the drivemap command uses this func on a "try-this-first" basis on its second argument, then resorts to raw number parsing. Thus, we do not want grub_error printing anything to screen (does it, by the way?). > > + else > > + { > > + bios_num |=3D drivenum; > > + if (output) > > + *output =3D bios_num; > > + return GRUB_ERR_NONE; > > + } > > + } > > + else return GRUB_ERR_BAD_ARGUMENT; > > +} > > + > > +static grub_err_t > > +grub_cmd_drivemap (struct grub_arg_list *state, int argc, char **args) > > +{ > > + if (state[0].set) /* Show: list mappings */ >=20 > Like the comments before And like the answers before. Since this has been raised a lot of times, and in order to compare our thoughts on comments, I'll post what I think comments should tell in each pos: /* Preconditions and check explanation */ if (blah && (foo || bar)) /* Cond-met: action to take (short) */ { /* If too long to fit elsewhere: preconditions inside the IF block, procedure to follow */ (...) } The same for the "else if" / "else" blocks, and after all of it maybe an additional comment explaining the postconditions, if required. >=20 > > + { > > + if (!drivemap) > > + grub_printf ("No drives have been remapped"); > > + else > > + { > > + grub_printf ("Showing only remapped drives. Drives that have= had " > > + "their slot assigned to another one and have no= t been " > > + "themselves remapped will become inaccessible t= hrough " > > + "the BIOS routines to the booted OS.\n\n"); >=20 > I do not think this message is immediatly clear to the user. Perhaps > we should even leave this out and say something in the to-be-written > manual? :-) Absolutely true. This message is pathetic and crappy, but as long as there's no manual... Can anyone suggest an alternative, clearer wording for it? > > + grub_printf ("Mapped\tGRUB\n"); > > + drivemap_node_t *curnode =3D drivemap; > > + while (curnode) > > + { > > + const char *dname =3D 0; > > + grub_err_t err =3D revparse_biosdisk (curnode->redirto, = &dname); > > + if (err !=3D 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 =3D curnode->next; > > + } > > + } > > + } > > + else if (state[1].set) /* Reset: just delete all mappings */ > > + { > > + if (drivemap) > > + { > > + drivemap_node_t *curnode =3D drivemap, *prevnode =3D 0; > > + while (curnode) > > + { > > + prevnode =3D curnode; > > + curnode =3D curnode->next; > > + grub_free (prevnode); > > + } > > + drivemap =3D 0; > > + } > > + } > > + else > > + { > > + if (argc !=3D 2) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments requi= red"); > > + grub_uint8_t mapfrom =3D 0; >=20 > Please do not use mixed code/declarations here. The only mixed code/decl I find is "=EF=BB=BFgrub_err_t err =3D revparse_bi= osdisk (curnode->redirto, &dname);" and it's a call to an internal function whose conditions are well-known - safe. All others are declarations with initializations, which I find pretty normal. However, if you mean literally "mixing" them... do you really advocate the K&R approach of "declare everything at the function start"? Time proved that scheme suboptimal. As you can see, most declarations are either at the very start of a _block_ (the scope unit) or near it, right after precondition checks. > > + grub_err_t err =3D parse_biosdisk (args[0], &mapfrom); > > + if (err !=3D GRUB_ERR_NONE) > > + return grub_error (err, "invalid disk or not managed by the BI= OS"); > > + > > + grub_uint8_t mapto =3D 0xFF; > > + err =3D tryparse_diskstring (args[1], &mapto); > > + if (err !=3D GRUB_ERR_NONE) /* Not a disk string. Maybe a raw nu= m then? */ > > + { =20 > > + grub_errno =3D GRUB_ERR_NONE; > > + unsigned long num =3D grub_strtoul (args[1], 0, 0); > > + if (grub_errno !=3D GRUB_ERR_NONE || num > 0xFF) /* Not a r= aw num or too high */ > > + return grub_error (grub_errno, > > + "Target specifier must be of the form (f= dN) or " > > + "(hdN), with 0 <=3D N < 128; or a plain = dec/hex " > > + "number between 0 and 255"); > > + else mapto =3D (grub_uint8_t)num; > > + } >=20 > Do we really want to support BIOS disk numbers here? I do not think > it is useful. I asked the same when I was starting to write this and I was told, more or less, that this was the way to go. > Can you add newlines between the #defines and the comments, this will > get quite messy like this. Done. >=20 > > + { > > + entries++; > > + curentry =3D curentry->next; > > + } > > + if (0 =3D=3D entries) >=20 > if (! entries) Nope. You C infidels might make no distinction between int and bool, but I was raised in the Holy Truth of languages with a boolean type. ^^=20 Semantically, "entries" represents an integer, and I want to check whether it is zero. I only use your form when checking for a)boolean-like conditions or b)non-null pointers. Even this last one would be "wrong" under my guidelines, since ANSI specifies that a null pointer is guaranteed to compare equal to zero, but in that case alone I let my laziness win and use the Unholy int->"boolean" interpretation of C's "if" statement. Most probably, the compiler will optimize the additional assembly space cost away if there's any, so no worries. I find the semantic distinctions important to keep, particularly if the cost is zero. > (...) > > +GRUB_MOD_FINI (drivemap) > > +{ > > + grub_loader_unregister_preboot (insthandler_hook); > > + insthandler_hook =3D 0; > > + grub_unregister_command ("drivemap"); > > +} >=20 > I wonder if this is not called, just before GRUB boots a kernel. That > would cause a problem :-) AFAIK the "fini" functions are called by grub_machine_fini, which is called _after_ the preboot hook. > > Index: commands/i386/pc/drivemap_int13h.S > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > RCS file: commands/i386/pc/drivemap_int13h.S > > diff -N commands/i386/pc/drivemap_int13h.S > > --- /dev/null 1 Jan 1970 00:00:00 -0000 > > +++ commands/i386/pc/drivemap_int13h.S 2 Jul 2008 01:12:36 -0000 =EF=BB=BFAnd here we go for the second round xD > (...) > > + > > + > > +/* > > + * 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 lik= e. > > + > > + 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=3D3. > > + * So the first three arguments are passed in %eax, %edx, and %e= cx, > > + * respectively, and if a function has a fixed number of argumen= ts > > + * and the number if greater than three, the function must retur= n > > + * with "ret $N" where N is ((the number of arguments) - 3) * 4. > > + */ > > >=20 > I do not think this is required. I mean, we know how GRUB is compiled > and how it deals with registers. But nevertheless each and every assembly file I looked at states the same. It's supposed to be useful when developing; although I did not use it, since my only assembly function is not called from C. > > +/* > > + * This is the area for all of the special variables. > > + */ >=20 > I don't think this comment is useful. I dont remember writing that... Maybe I c-ped it from somewhere? Deleted. > > +#include > > + > > +#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - grub_drivemap_int13_hand= ler_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, sinc= e we > > + * must make as few changes to the registers as possible. Pity we're n= ot on > > + * amd64: rIP-relative addressing would make life 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=3DDST =3D> 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 */ > > + =20 > > + 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 wor= ks */ > > + push 6(%bp) > > + movw (%bp), %bp /* Restore the caller BP (is this needed and/or sen= sible?) */ > > + lcall *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandle= r) > > + 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 includ= es it. > > + * The only func that does this seems to be origAH =3D 0x08, but man= y BIOS > > + * refs say retDL =3D # of drives connected. However, the GRUB Legac= y code > > + * treats this as the _drive number_ and "undoes" the remapping. Thu= s, > > + * 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=3DDST =3D> 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 instal= ler code > > + * reserves additional space for mappings at runtime and copies them o= ver 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) I'm disappointed... Not a single comment on this file?? ;) > (...) > > Index: kern/loader.c > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > RCS file: /sources/grub/grub2/kern/loader.c,v > > retrieving revision 1.9 > > diff -u -r1.9 loader.c > > --- kern/loader.c 21 Jul 2007 23:32:26 -0000 1.9 > > +++ kern/loader.c 2 Jul 2008 01:12:45 -0000 > > @@ -61,11 +61,78 @@ > > grub_loader_loaded =3D 0; > > } > > =20 > > +struct hooklist_node > > +{ > > + grub_err_t (*hook) (void); > > + int abort_on_error; > > + struct hooklist_node *next; > > +}; >=20 > Isn't there something=20 Woot? What do you mean with that? o_O > > +static struct hooklist_node *preboot_hooks =3D 0; > > + > > +grub_preboot_hookid > > +grub_loader_register_preboot(grub_err_t (*hook) (void), int abort_on_e= rror) > > +{ > > + if (0 =3D=3D hook) >=20 > if (! hook) This time you're right, per my "how to write C comparisons" rationale stated above. Changed.=20 > > + { > > + grub_error (GRUB_ERR_BAD_ARGUMENT, "preboot hoook must not be nu= ll"); >=20 > hoook >=20 > null -> NULL Changed too. > (...) > > + > > +void > > +grub_loader_unregister_preboot(grub_preboot_hookid id) > > +{ > > + if (0 =3D=3D id) > > + return; > > + grub_preboot_hookid entry =3D 0, search =3D preboot_hooks, previous = =3D 0; > > > Can you move this up so this is not mixed. Can you split this into 3 lin= es? As I understand coding style in C-like languages, precondition checks (if short enough) and declarations may be mixed as long as they don't get confusing; and I don't think this one is too confusing. On the other hand, variables splitted, but why the obsession with that particular rule? > (...) > > + grub_err_t possible_error =3D entry->hook(); >=20 > Just use "err" I think I did so in a previous version of this patch, and I was told to be a bit more explicit, but... changed. Phew! That was long, even after removing some parts. Well, this is the new version of the patch. Cheers! PS: I already signed the copyright assignment papers and sent them to the FSF. They should arrive within this week. --=-ASKs9eUn3+M9QCq/5v2X Content-Type: application/pgp-signature; name=signature.asc Content-Description: Esta parte del mensaje =?ISO-8859-1?Q?est=E1?= firmada digitalmente -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iQIVAwUASIPejaSl+Fbdeo72AQI/Vg//XG6JZzNlurqP+1YPeiT4ZRpZ2+x7pN99 1gOhEpot1muDl7Stq7H47RwEC+jqkoOIiVR7jKkJ2jv9P6y4RVe+JbdEePB2Xw3y tGGE8XGE+DftlSCqZTRoGhJM3+Vr7b9m3kVT6uk8IUTaoIqOW645BMC7RZvClFub tO6ELPtvovYfBiwPO9CjT0IH13tPXhNvbuk5ltmlCfcSi9jypXsp+aoS+EYi327W 1h0xZNXNP07rjS3pB1TOxffaOlrO+Tgl8z0GB19gHbCFyTLe9PntFmTTzuedEyUt cqdqXcAhTAxFGkoWz9UR3v0JqV7IXT/FJN4iCFGeRNnHwP8ChR5y/u7YGrWk2ukV fQPQ/pM+bf3WgjpY9k5UvXTWWA1gvAAAlUypGQB0d5FewpiCzGw9N3HLN6c+pTFU 0sDI7fRMdT5H4u1m6dOd7tDxLf1z6+/FAoyrXQKd7dhp08azXJf9Nvl9CcL08zg1 xj3+uKSP7ls6dCag3uMfGa1h2xLiYdEN6uvtLJKMTugqcxBHiIyA/VzlzPFBkpK4 ta5LumwRdH33NkWHpO9CPVvsJWVSNL7sb5hpedYMcM9TcrHGftIOcW95n4vRCbMh /vN2d4pf3cPNK9lPRM04z6IuWH1ZPkosyS1GuO+/mZU/pfyG+pPuOUlcjmBQYjEI 0RM5AN3E2yI= =r6RT -----END PGP SIGNATURE----- --=-ASKs9eUn3+M9QCq/5v2X--