From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1KQkEV-0001UU-V4 for mharc-grub-devel@gnu.org; Wed, 06 Aug 2008 10:43:24 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KQkET-0001TF-Ni for grub-devel@gnu.org; Wed, 06 Aug 2008 10:43:21 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KQkEP-0001Qs-TS for grub-devel@gnu.org; Wed, 06 Aug 2008 10:43:20 -0400 Received: from [199.232.76.173] (port=58236 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KQkEP-0001Qf-J7 for grub-devel@gnu.org; Wed, 06 Aug 2008 10:43:17 -0400 Received: from web31608.mail.mud.yahoo.com ([68.142.198.154]:20584) by monty-python.gnu.org with smtp (Exim 4.60) (envelope-from ) id 1KQkEN-0006Sk-Js for grub-devel@gnu.org; Wed, 06 Aug 2008 10:43:17 -0400 Received: (qmail 55969 invoked by uid 60001); 6 Aug 2008 14:43:14 -0000 DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s1024; d=yahoo.com; h=Received:X-Mailer:Date:From:Subject:To:MIME-Version:Content-Type:Message-ID; b=h/5oURsrfKRJxsaErg5sjrGWKztDPEl8rKLQuecXQzsTaJHXtdLEDNO2eWlsZJ2BVjWWUua+MLzxhnyu3EC9IkqgwGXs5Grm/stCoau2rU/NE2w1kTV+aCLvhsxgCqiTYYmTTRVFP8NX6QLQivbHS7X05pNhh+IqyryeSL4lUo4=; Received: from [202.62.94.130] by web31608.mail.mud.yahoo.com via HTTP; Wed, 06 Aug 2008 07:43:14 PDT X-Mailer: YahooMailRC/1042.48 YahooMailWebService/0.7.218 Date: Wed, 6 Aug 2008 07:43:14 -0700 (PDT) From: Viswesh S To: The development of GRUB 2 MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="0-1133575639-1218033794=:55348" Message-ID: <721379.55348.qm@web31608.mail.mud.yahoo.com> X-detected-kernel: by monty-python.gnu.org: FreeBSD 6.x (1) 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: Wed, 06 Aug 2008 14:43:22 -0000 --0-1133575639-1218033794=:55348 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi,=0ACould you let all know how to use your drivemap command.Is it similar= to the map command in legacy grub.=0Adrivemap (hd0) (hd1)=0Adrivemap (hd1)= (hd0)=0Awith the grub.cfg to be =0Amenuentry "Linux" {=0Alinux ....=0Ainit= rd ...=0A}=0Amenuentry "Windows" {=0A=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 = set root=3D(hd1,1)=0A=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 chainloader +1= =0A}=0AViswesh=0A=0A=0A=0A----- Original Message ----=0AFrom: Javier Mart= =C3=ADn =0ATo: The development of GRUB 2 =0ASent: Tuesday, 5 August, 2008 10:09:41 PM=0ASubject: Re: [PATCH]= Drivemap module=0A=0AOk, I will reply to your last three messages on a sin= gle post:=0AHi there!=0A=0A=EF=BB=BFEl mar, 05-08-2008 a las 13:31 +0200, M= arco Gerards escribi=C3=B3: =0A> Hi,=0A> =0A> Javier Mart=C3=ADn writes:=0A> =0A> >> Anyway, since "they" are more likely to ma= intain the code in =0A> >> the long run than you, in general, the question = is whether =0A> >> the code is more likely to become buggy by their hacking= on =0A> >> it, if it follows project coding style or someone else's =0A> >= > (your) "safer" coding style.=C2=A0 Likely it's safer if using a =0A> >> c= onsistent programming style.=C2=A0 Although I personally don't =0A> >> see = that it's very helpful to have a style that makes things =0A> >> down to th= e order of "=3D=3D" arguments be consistent within the =0A> >> project; hap= hazard only slows reading the tiniest bit, and I =0A> >> don't think it mak= es a different what order the arguments are...=0A> > =EF=BB=BF=0A> > Hmm...= I was partially expecting a flamefest to start. Pity ^^=0A> > Well, let's = spill a little napalm: the GNU style bracing is extremely=0A> > silly!! Why= the hell are the "if" and "else" blocks indented differently=0A> > in this= example?=0A> >=C2=A0 if (condition)=0A> >=C2=A0 =C2=A0 return 0;=0A> >=C2= =A0 else=0A> >=C2=A0 =C2=A0 {=0A> >=C2=A0 =C2=A0 =C2=A0 return -1;=0A> >=C2= =A0 =C2=A0 }=0A> > Nah, I'm not really bringing that issue, I was just joki= ng, and in fact=0A> > I'm reconsidering my objections to the operator=3D=3D= arguments order rule,=0A> > even though I still consider my style safer an= d more sensible. If=0A> > someone else wants to express their opinion on th= at issue, do it fast=0A> > before I completely submit to Master Marco's wil= l :D=0A> =0A> Please don't be sarcastic, start flame wars or call names.=C2= =A0 It will not=0A> help anyone.=0A=0AOk, sorry, picturing you as a whip-cr= acking dominatrix was really not proper ^^. I was just joking about how fas= t can flamewars be started.=0A=0A> =0A> --=0A> Marco=0A> =0A> =0A> =0A> ___= ____________________________________________=0A> Grub-devel mailing list=0A= > Grub-devel@gnu.org=0A> http://lists.gnu.org/mailman/listinfo/grub-devel= =0A=EF=BB=BF=0AEl mar, 05-08-2008 a las 13:23 +0200, Marco Gerards escribi= =C3=B3:=0A> Hi Javier,=0A> =0A> Javier Mart=C3=ADn w= rites:=0A> =0A> > El lun, 04-08-2008 a las 22:51 +0200, Marco Gerards escri= bi=C3=B3:=0A> >> Javier Mart=C3=ADn writes:=0A> >> = =0A> >> > After your latest replay, I "reevaluated" my stubbornness WRT som= e of=0A> >> > your advices, and I've changed a few things:=0A> >> >=0A> >> = > - Variables are now declared (and, if possible, initialized) before=0A> >= > > precondition checks, even simple ones. The install_int13_handler=0A> >>= > function has not been modified, however, since I find it a bit=0A> >> > = nonsensical to put bunch of declarations without an obvious meaning just=0A= > >> > after the "else" line:=0A> >> >=C2=A0 =C2=A0 =C2=A0 grub_uint32_t *i= vtslot;=0A> >> >=C2=A0 =C2=A0 =C2=A0 grub_uint16_t *bpa_freekb;=0A> >> >=C2= =A0 =C2=A0 =C2=A0 grub_size_t total_size;=0A> >> >=C2=A0 =C2=A0 =C2=A0 grub= _uint16_t payload_sizekb;=0A> >> >=C2=A0 =C2=A0 =C2=A0 grub_uint8_t *handle= r_base;=0A> >> >=C2=A0 =C2=A0 =C2=A0 int13map_node_t *handler_map;=0A> >> >= =C2=A0 =C2=A0 =C2=A0 grub_uint32_t ivtentry;=0A> >> =0A> >> Please understa= nd me correctly.=C2=A0 Code just has to be written according=0A> >> to our = coding standards before it can and will be included.=C2=A0 We can=0A> >> di= scuss things endlessly but we will simply stick to the GNU Coding=0A> >> St= andards as you might expect.=0A> >> =0A> >> I hope you can appreciate that = all code of GRUB has the same coding=0A> >> style, if you like this style o= r not.=0A> > Big sigh. Function modified to conform to your preciousss codi= ng style.=0A> > I might look a bit childish or arrogant saying this, but wh= at is the=0A> > point upholding a coding style that, no matter how consiste= nt it is,=0A> > hinders readability. With so much local variables, they are= hard to keep=0A> > track of no matter the coding style, but with the old (= "mixed, heretic")=0A> > style there was not need for a comment before each = declaration: they=0A> > were declared and used when needed instead of at th= e start of a block,=0A> > because that function is inherently linear, not b= lock-structured.=0A> =0A> In your opinion it is not a good coding style.=C2= =A0 I just avoid=0A> discussions about it because such discussions just was= te my time.=0A> Even if you are able to convince me, GRUB is a GNU project = and will=0A> remain to use the GCS.=0A=0AWhich is fine, but the order of th= e arguments to =3D=3D is specified nowhere in the GCS: the order you defend= only appears in examples less than five times. Thus, your insistence is a = matter of internal consistence within GRUB and is only based on examples in= the GCS. Additionally, your previous suggestion to change checks line "ent= ries =3D=3D 0 " to "!entries" are _against_ the examples of the GCS, even f= or pointers.=0A=0AThen, given that the _examples_ in the GCS are non-author= itative, I was advocating a simple change for better resilience against fut= ure changes. You want to keep consistency with the current sources, and tha= t I understand, so I will perform the required trivial changes in my code t= o use the style used in other GRUB code, but I still think it is worse and = more error-prone.=0A=0A> =0A> =0A> >> > - Only one declaration per line: ev= en though C is a bit absurd in not=0A> >> > recognizing T* as a first class= type and instead thinking of * as a=0A> >> > qualifier to the variable nam= e; and even though my code does not incur=0A> >> > into such ambiguities.= =0A> >> > - Comments moved as you required, reworded as needed=0A> >> > - E= xtensive printf showing quasi-help in the "show" mode trimmed down to=0A> >= > > just the first sentence.=0A> >> > - Internal helper functions now use t= he standard error handling, i.e.=0A> >> > return grub_error (err, fmt, ...)= =0A> >> > - Comment about the strange "void" type instead of "void*" rephra= sed to=0A> >> > be clearer=0A> >> =0A> >> Thanks a lot!=0A> >> =0A> >> > Th= ere is, however, one point in which I keep my objection: comparisons=0A> >>= > between a variable and a constant should be of the form CONSTANT =3D=3D= =0A> >> > variable and not in the reverse order, since an erroneous but qui= te=0A> >> > possible change of =3D=3D for =3D results in a compile-time err= or instead of a=0A> >> > _extremely_ difficult to trace runtime bug. Such k= ind of bugs are quite=0A> >> > excruciating to find in userspace applicatio= ns within an IDE, so I can't=0A> >> > even consider the pain to debug them = in a bootloader.=0A> >> =0A> >> I understand your concern, nevertheless, ca= n you please change it?=0A> > You understand my concern, but seemingly do n= ot understand that in order=0A> > to conform to the Holy Coding Style you a= re asking me to write code that=0A> > can become buggy (and with a very har= d to trace bug) with a simple=0A> > deltion! (point: did you notice that la= st word _without_ a spelling=0A> > checker? Now try to do so in a multithou= sand-line program).=0A> =0A> Yes, I did notice it immediately.=0A> =0A> The= coding style is not holy in any way.=C2=A0 Everyone has its own coding=0A>= style.=C2=A0 You must understand I do not want to fully discuss it every= =0A> time someone sends in a patch, especially since it will not change=0A>= anyways.=0A=0ABy the way, I just noticed that you write _normal_ text with= two spaces after a dot, just like comments in the code! o_O=0A> =0A> > Whi= le tools like `svn diff' can help in these kind of problems, their=0A> > ut= ility is greatly diminished if the offending change is part of a=0A> > mult= i-line change. Besides, sometimes checks like "if (a=3Db)", or more=0A> > f= requently "if (a=3Df())" are intentionally used in C, so the error might=0A= > > become even more difficult to spot and correct. I ask for a deep=0A> > = reflection on this issue: =EF=BB=BFmaybe I'm dead wrong and an arrogant bra= t in=0A> > my attempt to tackle the Holy GNU Coding Standards, but I'd like= to ask=0A> > input from more people on this.=0A> =0A> I will refuse patche= s with "if (a =3D f())", if that makes you sleep=0A> better ;-)=0AIn fact t= he GCS discourages that use, but allows the same in loop checks, particular= ly "while" loops.=0A>=C2=A0 =0A> >> > WRT accepting raw BIOS disk numbers, = I agree with you in principle, but=0A> >> > I'm keeping the functionality f= or now, since I don't quite like the=0A> >> > "drivemap (hd0) (hd1)" syntax= - which device is which?. I'd rather have=0A> >> > something akin to "driv= emap (hd0) (bios:hd1)", but I want to hear the=0A> >> > opinions of people = in this list.=0A> >> =0A> >> I personally do not care a lot if BIOS disk nu= mbers are used.=0A> >> Although I do not see why it is useful.=0A> >> =0A> = >> As for the syntax, I would prefer something more GRUB2ish, like:=0A> >> = =0A> >> map --bios=3D(hd0) --os=3D(hd1)=0A> > I agree. What about "grub" fo= r the source drive instead of "os", with=0A> > "bios" being the target driv= e? I mean:=0A> > =C2=A0=C2=A0=C2=A0 drivemap --grub=3D(hd1) --bios=3D(hd0)= =0A> > Would map 0x80 in the BIOS to grub's (hd1) drive which will most lik= ely=0A> > be BIOS drive 0x81.=0A> =0A> It will not change the functionality= which GRUB is active.=C2=A0 But it=0A> will change it for the OS that is l= oaded.=C2=A0 So I do not think --grub=3D=0A> is a good idea because of this= .=C2=A0 As for GRUB Legacy, it confused a lot=0A> of people, so making peop= le explicitly say that it is changed for the=0A> OS, this confusion might g= o away :-)=0A> =0A> > However, I prefer not to change it right now. Maybe w= hen there are no=0A> > other issues WRT to this patch we can set on to tack= le this one.=0A> =0A> So first I'll review this patch, then you will send i= n a new one?=0A=0AOf course. I meant that I prefer to smash all "syntactic"= changes in the patch before introducing a functionality change that could = lead to new syntactic issues.=0A>=C2=A0 =0A> >> Or so, perhaps the long arg= ument names can be chosen in a more clever=0A> >> way :-)=0A> >> > The new = version of the patch is attached, and here is my suggested CLog:=0A> >> >= =0A> >> > 2008-08-XX=C2=A0 Javier Martin=C2=A0 =0A> >= > =0A> >> (newline)=0A> >> =0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF* = commands/i386/pc/drivemap.c : New file.=0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0= * commands/i386/pc/drivemap_int13h.S : New file.=0A> >> >=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =EF=BB=BF* conf/i386-pc.rmk (pkglib_MODULES) : Added drivemap.mo= d=0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 (drivemap_mod_SOURCES) : New variable= =0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 (drivemap_mod_ASFLAGS) : Likewise=0A> = >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF(drivemap_mod_CFLAGS) : Likewise= =0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF(drivemap_mod_LDFLAGS) : Like= wise=0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF* include/grub/loader.h (= grub_loader_register_preboot) : New=0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 fun= ction prototype. Register a new pre-boot handler=0A> >> =0A> >> No need how= the change is used or why it was added.=0A> >> =0A> >> >=C2=A0 =C2=A0 =C2= =A0 =C2=A0 (grub_loader_unregister_preboot) : Likewise. Unregister handler= =0A> >> =0A> >> Same here.=0A> >> =0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 (gru= b_preboot_hookid) : New typedef. Registered hook "handle"=0A> >> =0A> >> Sa= me here.=0A> >> =0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF* kern/loader= .c =EF=BB=BF(grub_loader_register_preboot) : New function.=0A> >> >=C2=A0 = =C2=A0 =C2=A0 =C2=A0 (grub_loader_unregister_preboot) : Likewise.=0A> >> >= =C2=A0 =C2=A0 =C2=A0 =C2=A0 (preboot_hooks) : New variable. Linked list of = preboot hooks=0A> >> =0A> >> Same here.=0A> >> =0A> >> >=C2=A0 =C2=A0 =C2= =A0 =C2=A0 (grub_loader_boot) : Call the list of preboot-hooks before the= =0A> >> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 actual loader=0A> >> =0A> >> What's th= e `=EF=BB=BF'?=0A> > The what? o_O=0A> =0A> I see some weird character in y= our text.=C2=A0 My font shows it as a block=0A> before every `*'.=0AI see n= othing: "What's the `'?". Maybe it's some kind of tab?=0A> =0A> >> Please d= o not add a space before the ":" =0A> > Ok, everything corrected. New CL en= try:=0A> >=0A> > =EF=BB=BF2008-08-XX=C2=A0 Javier Martin=C2=A0 =0A> >=0A> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF* commands/i386/= pc/drivemap.c: New file.=0A> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 * commands/i386/p= c/drivemap_int13h.S: New file.=0A> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF* = conf/i386-pc.rmk (pkglib_MODULES): Added drivemap.mod=0A> >=C2=A0 =C2=A0 = =C2=A0 =C2=A0 (drivemap_mod_SOURCES): New variable=0A> >=C2=A0 =C2=A0 =C2= =A0 =C2=A0 (drivemap_mod_ASFLAGS): Likewise=0A> >=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =EF=BB=BF(drivemap_mod_CFLAGS): Likewise=0A> >=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =EF=BB=BF(drivemap_mod_LDFLAGS): Likewise=0A> >=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =EF=BB=BF* include/grub/loader.h (grub_loader_register_preboot): New=0A= > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 function prototype.=0A> >=C2=A0 =C2=A0 =C2= =A0 =C2=A0 (grub_loader_unregister_preboot): Likewise.=0A> >=C2=A0 =C2=A0 = =C2=A0 =C2=A0 (grub_preboot_hookid): New typedef.=0A> >=C2=A0 =C2=A0 =C2=A0= =C2=A0 =EF=BB=BF* kern/loader.c =EF=BB=BF(grub_loader_register_preboot): N= ew function.=0A> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 (grub_loader_unregister_prebo= ot): Likewise.=0A> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 (preboot_hooks): New variab= le.=0A> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 (grub_loader_boot): Call the list of p= reboot-hooks before the=0A> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 actual loader=0A> = =0A> Please add a `.' after "New variable" and "Likewise", same for the=0A>= third and the last sentence.=C2=A0 Sometimes you did it right :-).=0A> =0A= Done. New CL entry will go at the end of this neverending post.=0A=0A> =0A>= >> Some comments can be found below.=C2=A0 Can you please fix the code men= tion=0A> >> in the review and similar code?=C2=A0 I really want the code to= be just=0A> >> like any other GRUB code.=C2=A0 Please understand that we h= ave to maintain=0A> >> this in the future.=C2=A0 If everyone would use his = own codingstyle, GRUB=0A> >> would become unmaintainable.=0A> >> =0A> >> > = Index: commands/i386/pc/drivemap.c=0A> >> > =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=0A> >> > --- commands/i386/pc/drivemap.c=C2=A0=C2=A0= =C2=A0 (revisi=C3=B3n: 0)=0A> >> > +++ commands/i386/pc/drivemap.c=C2=A0=C2= =A0=C2=A0 (revisi=C3=B3n: 0)=0A> >> > @@ -0,0 +1,417 @@=0A> >> > +/* drivem= ap.c - command to manage the BIOS drive mappings.=C2=A0 */=0A> >> > +/*=0A>= >> > + *=C2=A0 GRUB=C2=A0 --=C2=A0 GRand Unified Bootloader=0A> >> > + *= =C2=A0 Copyright (C) 2008=C2=A0 Free Software Foundation, Inc.=0A> >> > + *= =0A> >> > + *=C2=A0 GRUB is free software: you can redistribute it and/or m= odify=0A> >> > + *=C2=A0 it under the terms of the GNU General Public Licen= se as published by=0A> >> > + *=C2=A0 the Free Software Foundation, either = version 3 of the License, or=0A> >> > + *=C2=A0 (at your option) any later = version.=0A> >> > + *=0A> >> > + *=C2=A0 GRUB is distributed in the hope th= at it will be useful,=0A> >> > + *=C2=A0 but WITHOUT ANY WARRANTY; without = even the implied warranty of=0A> >> > + *=C2=A0 MERCHANTABILITY or FITNESS = FOR A PARTICULAR PURPOSE.=C2=A0 See the=0A> >> > + *=C2=A0 GNU General Publ= ic License for more details.=0A> >> > + *=0A> >> > + *=C2=A0 You should hav= e received a copy of the GNU General Public License=0A> >> > + *=C2=A0 alon= g with GRUB.=C2=A0 If not, see .=0A> >> > + *= /=0A> >> > +=0A> >> > +#include =0A> >> > +#include =0A> >> > +#include =0A> >> > +#include =0A> >> = > +#include =0A> >> > +#include =0A> >> > +#inc= lude =0A> >> > +#include = =0A> >> > +=0A> >> > +#define MODNAME "drivemap"=0A> >> > +=0A> >> > +stati= c const struct grub_arg_option options[] =3D {=0A> >> > +=C2=A0 {"list", 'l= ', 0, "show the current mappings", 0, 0},=0A> >> > +=C2=A0 {"reset", 'r', 0= , "reset all mappings to the default values", 0, 0},=0A> >> > +=C2=A0 {0, 0= , 0, 0, 0, 0}=0A> >> > +};=0A> >> > +=0A> >> > +/* Syms/vars/funcs exported= from drivemap_int13h.S - start.=C2=A0 */=0A> >> > +=0A> >> > +/* Realmode = far ptr =3D 2 * 16b */=0A> >> > +extern grub_uint32_t grub_drivemap_int13_o= ldhandler;=0A> >> > +/* Size of the section to be copied */=0A> >> > +exter= n grub_uint16_t grub_drivemap_int13_size;=0A> >> > +=0A> >> > +/* This type= is used for imported assembly labels, takes no storage and is only=0A> >> = > +=C2=A0 used to take the symbol address with &label.=C2=A0 Do NOT put voi= d* here.=C2=A0 */=0A> >> > +typedef void grub_symbol_t;=0A> >> > +extern gr= ub_symbol_t grub_drivemap_int13_handler_base;=0A> >> > +extern grub_symbol_= t grub_drivemap_int13_mapstart;=0A> >> > +=0A> >> > +void grub_drivemap_int= 13_handler (void);=0A> >> =0A> >> The lines above belong in a header file.= =0A> > True, but they are used in a single file in the whole project and th= us I=0A> > see it pointless to extract an unneeded header, which would beco= me one=0A> > more SVN object to track. However, if you insist I will split = the header=0A> > file at once. In particular, I think the grub_symbol_t typ= edef should go=0A> > into the "standard" GRUB headers somehow (but not in s= ymbol.h, which is=0A> > included from assembly files).=0A> =0A> Please do s= o.=0A> =0A> Other people might want to comment on the symbol change.=C2=A0 = They will=0A> most likely miss it if we keep discussing it here ;-).=C2=A0 = Can you please=0A> send that in as a separate change to give other the oppo= rtunity to=0A> react on it?=0AOk, so in a first stage I will put the discus= sed block in a drivemap.h file in include/, then when that's committed star= t a discussion about moving grub_symbol_t from drivemap.h to another header= file.=0A> =0A> >> > +/* Syms/vars/funcs exported from drivemap_int13h.S - = end.=C2=A0 */=0A> >> > +=0A> >> > +=0A> >> > +static grub_preboot_hookid in= sthandler_hook;=0A> >> > +=0A> >> > +typedef struct drivemap_node=0A> >> > = +{=0A> >> > +=C2=A0 grub_uint8_t newdrive;=0A> >> > +=C2=A0 grub_uint8_t re= dirto;=0A> >> > +=C2=A0 struct drivemap_node *next;=0A> >> > +} drivemap_no= de_t;=0A> >> > +=0A> >> > +static drivemap_node_t *drivemap =3D 0;=0A> >> = =0A> >> No need to set this to zero.=0A> > Yes, you said so already, but I = wanted to make the initial state very=0A> > explicit to a future developer,= since that variable is checked against=0A> > zero in several points. Given= that the added source size is four bytes=0A> > and the added binary size i= s _zero_, is all the fuss really necessary?=0A> > Notice that I changed the= other variable in which you pointed out this=0A> > issue, because it is no= t checked against zero anywhere.=0A> =0A> Please do so anyways.=0A> =0A> >>= > +static grub_err_t install_int13_handler (void);=0A> >> > +=0A> >> > +/*= Puts the specified mapping into the table, replacing an existing mapping= =0A> >> > +=C2=A0 for newdrive or adding a new one if required.=C2=A0 */=0A= > >> > +static grub_err_t=0A> >> > +drivemap_set (grub_uint8_t newdrive, gr= ub_uint8_t redirto)=0A> >> > +=0A> >> =0A> >> Please do not add a newline h= ere.=0A> > Oops, sorry. I forgot to remove it when moving the comment=0A> = =0A> :-)=0A> =0A> >> > +=C2=A0 drivemap_node_t *mapping =3D 0;=0A> >> > += =C2=A0 drivemap_node_t *search =3D drivemap;=0A> >> > +=C2=A0 while (search= )=0A> >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (search->newd= rive =3D=3D newdrive)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 {=0A> >> > +=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mapping =3D search;=0A> >> > +=C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 break;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }=0A> >>= > +=C2=A0 =C2=A0 =C2=A0 search =3D search->next;=0A> >> > +=C2=A0 =C2=A0 }= =0A> >> > +=0A> >> > +=C2=A0 =0A> >> > +=C2=A0 /* Check for pre-existing ma= ppings to modify before creating a new one.=C2=A0 */=0A> >> > +=C2=A0 if (m= apping)=0A> >> > +=C2=A0 =C2=A0 mapping->redirto =3D redirto;=0A> >> > +=C2= =A0 else =0A> >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 mapping = =3D grub_malloc (sizeof (drivemap_node_t));=0A> >> > +=C2=A0 =C2=A0 =C2=A0 = if (!mapping)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return grub_error (GRUB= _ERR_OUT_OF_MEMORY,=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "cannot allocate map entry, n= ot enough memory");=0A> >> > +=C2=A0 =C2=A0 =C2=A0 mapping->newdrive =3D ne= wdrive;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 mapping->redirto =3D redirto;=0A> >>= > +=C2=A0 =C2=A0 =C2=A0 mapping->next =3D drivemap;=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 drivemap =3D mapping;=0A> >> > +=C2=A0 =C2=A0 }=0A> >> > +=C2=A0= return GRUB_ERR_NONE;=0A> >> > +}=0A> >> > +=0A> >> > +/* Removes the mapp= ing for newdrive from the table.=C2=A0 If there is no mapping,=0A> >> > += =C2=A0 then this function behaves like a no-op on the map.=C2=A0 */=0A> >> = > +static void=0A> >> > +drivemap_remove (grub_uint8_t newdrive)=0A> >> > += {=0A> >> > +=C2=A0 drivemap_node_t *mapping =3D 0;=0A> >> > +=C2=A0 drivema= p_node_t *search =3D drivemap;=0A> >> > +=C2=A0 drivemap_node_t *previous = =3D 0;=0A> >> > +=C2=A0 while (search)=0A> >> > +=C2=A0 =C2=A0 {=0A> >> > += =C2=A0 =C2=A0 =C2=A0 if (search->newdrive =3D=3D newdrive)=0A> >> > +=C2=A0= =C2=A0 =C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mappin= g =3D search;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;=0A> >> > = +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }=0A> >> > +=C2=A0 =C2=A0 =C2=A0 previous =3D = search;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 search =3D search->next;=0A> >> > += =C2=A0 =C2=A0 }=0A> >> > +=C2=A0 if (mapping) /* Found.=C2=A0 */=0A> >> =0A= > >> You forgot one.=0A> > Corrected. Sorry.=0A> >> =0A> >> > +=C2=A0 =C2= =A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (previous)=0A> >> > +=C2=A0 =C2=A0 = =C2=A0 =C2=A0 previous->next =3D mapping->next;=0A> >> > +=C2=A0 =C2=A0 =C2= =A0 else /* Entry was head of list.=C2=A0 */=0A> >> > +=C2=A0 =C2=A0 =C2=A0= =C2=A0 drivemap =3D mapping->next;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_fre= e (mapping);=0A> >> > +=C2=A0 =C2=A0 }=0A> >> > +}=0A> >> > +=0A> >> > +/* = Given a device name, resolves its BIOS disk number and stores it in the=0A>= >> > +=C2=A0 passed location, which should only be trusted if ERR_NONE is = returned.=C2=A0 */=0A> >> > +static grub_err_t=0A> >> > +parse_biosdisk (co= nst char *name, grub_uint8_t *disknum)=0A> >> > +{=0A> >> > +=C2=A0 grub_di= sk_t disk;=0A> >> > +=C2=A0 if (!name || 0 =3D=3D *name)=0A> >> > +=C2=A0 = =C2=A0 return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");=0A> = >> > +=C2=A0 /* Skip the first ( in (hd0) - disk_open wants just the name.= =C2=A0 */=0A> >> > +=C2=A0 if (*name =3D=3D '(')=0A> >> > +=C2=A0 =C2=A0 na= me++;=0A> >> > +=C2=A0 =0A> >> > +=C2=A0 disk =3D grub_disk_open (name);=0A= > >> > +=C2=A0 if (!disk)=0A> >> > +=C2=A0 =C2=A0 return grub_error (GRUB_E= RR_UNKNOWN_DEVICE, "unknown device \"%s\"", name);=0A> >> > +=C2=A0 else=0A= > >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 const enum grub_disk= _dev_id id =3D disk->dev->id;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 /* The followi= ng assignment is only sound if the device is indeed a=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 =C2=A0 biosdisk.=C2=A0 The caller must check the return value.= =C2=A0 */=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (disknum)=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 =C2=A0 *disknum =3D disk->id;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 gru= b_disk_close (disk);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (GRUB_DISK_DEVICE_BI= OSDISK_ID =3D=3D id)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return GRUB_ERR_= NONE;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 else return grub_error (GRUB_ERR_BAD_D= EVICE, "%s is not a BIOS disk", name);=0A> >> > +=C2=A0 =C2=A0 }=0A> >> > += }=0A> >> > +=0A> >> > +/* Given a BIOS disk number, returns its GRUB device= name if it exists.=0A> >> > +=C2=A0 For nonexisting BIOS dnums, this funct= ion returns ERR_UNKNOWN_DEVICE.=C2=A0 */=0A> >> =0A> >> This is GRUB_ERR_UN= KNOWN_DEVICE=0A> > I know, I consciously left the GRUB_ part out because 1)= it would=0A> > require the line to be split and 2) that prefix is all over= the place.=0A> > Corrected, however.=0A> >> =0A> >> > +static grub_err_t= =0A> >> > +revparse_biosdisk(const grub_uint8_t dnum, const char **output)= =0A> >> > +{=0A> >> > +=C2=A0 int found =3D 0;=0A> >> > +=C2=A0 auto int fi= nd (const char *name);=0A> >> > +=C2=A0 int find (const char *name)=0A> >> = > +=C2=A0 {=0A> >> > +=C2=A0 =C2=A0 const grub_disk_t disk =3D grub_disk_op= en (name);=0A> >> > +=C2=A0 =C2=A0 if (!disk)=0A> >> > +=C2=A0 =C2=A0 =C2= =A0 return 0;=0A> >> > +=C2=A0 =C2=A0 else=0A> >> > +=C2=A0 =C2=A0 =C2=A0 {= =0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (disk->id =3D=3D dnum && GRUB_DIS= K_DEVICE_BIOSDISK_ID =3D=3D disk->dev->id)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 found = =3D 1;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (output)=0A> = >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *output =3D name;=0A= > >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }=0A> >> > +=C2=A0 =C2=A0 =C2=A0= =C2=A0 grub_disk_close (disk);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 retur= n found;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 }=0A> >> > +=C2=A0 }=0A> >> > +=0A>= >> > +=C2=A0 grub_disk_dev_iterate (find);=0A> >> > +=C2=A0 if (found)=0A>= >> > +=C2=A0 =C2=A0 return GRUB_ERR_NONE;=0A> >> > +=C2=A0 else return gru= b_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %d not found", dnum);=0A> >> = =0A> >> Please change this.=0A> > Em... to what? What is the problem? Do yo= u want me to reverse the=0A> > comparison? i.e. if (!found) { return error;= } else { return ok; }=0A> =0A> The return is on the same line as the else.= =0A> =0A=0ACorrected.=0A=0A> =0A> >> > +/* Given a GRUB-like device name an= d a convenient location, stores the related=0A> >> > +=C2=A0 BIOS disk numb= er.=C2=A0 Accepts devices like \((f|h)dN\), with 0 <=3D N < 128.=C2=A0 */= =0A> >> > +static grub_err_t=0A> >> > +tryparse_diskstring (const char *str= , grub_uint8_t *output)=0A> >> > +{=0A> >> > +=C2=A0 if (!str || 0 =3D=3D *= str)=0A> >> > +=C2=A0 =C2=A0 goto fail;=0A> >> > +=C2=A0 /* Skip opening pa= ren in order to allow both (hd0) and hd0.=C2=A0 */=0A> >> > +=C2=A0 if (*st= r =3D=3D '(')=0A> >> > +=C2=A0 =C2=A0 str++;=0A> >> > +=C2=A0 if ((str[0] = =3D=3D 'f' || str[0] =3D=3D 'h') && str[1] =3D=3D 'd')=0A> >> > +=C2=A0 =C2= =A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_uint8_t bios_num =3D (str[0] =3D= =3D 'h')? 0x80 : 0x00;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_errno =3D GRUB_E= RR_NONE;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 unsigned long drivenum =3D grub_str= toul (str + 2, 0, 0);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (grub_errno !=3D GR= UB_ERR_NONE || drivenum > 127)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* N n= ot a number or out of range */=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 goto f= ail;=0A> >> =0A> >> Can you put this between braces, now comment was added.= =0A> > Done.=0A> >> =0A> >> > +=C2=A0 =C2=A0 =C2=A0 else=0A> >> > +=C2=A0 = =C2=A0 =C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bios_nu= m |=3D drivenum;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (output)= =0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *output =3D bios_num;= =0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return GRUB_ERR_NONE;=0A> >> = > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }=0A> >> > +=C2=A0 =C2=A0 }=0A> >> > +=C2=A0= else goto fail;=0A> >> =0A> >> ...=0A> > What's the problem here? The lack= of braces? The goto (as used in the=0A> > ext2 code)?=0A> =0A> goto is on = the same line as the else.=0A=0ACorrected, though I find this change less l= ogic than the last one (splitting the else retun grub_error(...)) line beca= use this generates two _extremely_ short lines;=0A> =0A> >> > +fail:=0A> >>= > +=C2=A0 return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" = invalid: must"=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 "be (f|h)dN, with 0 <=3D N < 128", str);=0A> >> > +}= =0A> >> > +=0A> >> > +static grub_err_t=0A> >> > +grub_cmd_drivemap (struct= grub_arg_list *state, int argc, char **args)=0A> >> > +{=0A> >> > +=C2=A0 = if (state[0].set)=0A> >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 = /* Show: list mappings.=C2=A0 */=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (!drivem= ap)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 grub_printf ("No drives have been= remapped");=0A> >> > +=C2=A0 =C2=A0 =C2=A0 else=0A> >> > +=C2=A0 =C2=A0 = =C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 grub_printf ("= Showing only remapped drives.\n");=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 grub_printf ("Mapped\tGRUB\n");=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 drivemap_node_t *curnode =3D drivemap;=0A> >> > +=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 while (curnode)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = const char *dname =3D 0;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 grub_err_t err =3D revparse_biosdisk (curnode->redirto, &dname);= =0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (err !=3D GR= UB_ERR_NONE)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 return grub_error (err, "invalid mapping: non-existent disk"=0A> >> > += =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "or not = managed by the BIOS");=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 grub_printf("0x%02x\t%4s\n", curnode->newdrive, dname);=0A> >> > += =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 curnode =3D curnode->next;= =0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 =C2=A0 }=0A> >> > +=C2=A0 =C2=A0 }=0A> >> > +=C2=A0 else if (sta= te[1].set)=0A> >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 /* Rese= t: just delete all mappings, freeing their memory.=C2=A0 */=0A> >> > +=C2= =A0 =C2=A0 =C2=A0 drivemap_node_t *curnode =3D drivemap;=0A> >> > +=C2=A0 = =C2=A0 =C2=A0 drivemap_node_t *prevnode =3D 0;=0A> >> > +=C2=A0 =C2=A0 =C2= =A0 while (curnode)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 {=0A> >> > +=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 prevnode =3D curnode;=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 curnode =3D curnode->next;=0A> >> > +=C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 grub_free (prevnode);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 = =C2=A0 }=0A> >> > +=C2=A0 =C2=A0 =C2=A0 drivemap =3D 0;=0A> >> > +=C2=A0 = =C2=A0 }=0A> >> > +=C2=A0 else=0A> >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 = =C2=A0 =C2=A0 /* Neither flag: put mapping */=0A> >> =0A> >> ".=C2=A0 */=0A= > > Done=0A> >> =0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_uint8_t mapfrom =3D 0;= =0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_uint8_t mapto =3D 0xFF;=0A> >> > +=C2= =A0 =C2=A0 =C2=A0 grub_err_t err;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =0A> >> > = +=C2=A0 =C2=A0 =C2=A0 if (argc !=3D 2)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2= =A0 return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");=0A= > >> > +=0A> >> > +=C2=A0 =C2=A0 =C2=A0 err =3D parse_biosdisk (args[0], &m= apfrom);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (err !=3D GRUB_ERR_NONE)=0A> >> = > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;=0A> >> > +=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 err =3D tryparse_diskstring (args[1], &mapto);=0A> >> > +=C2=A0 = =C2=A0 =C2=A0 if (err !=3D GRUB_ERR_NONE) /* Not a disk string. Maybe a raw= num then?=C2=A0 */=0A> >> =0A> >> Please move this up.=0A> > Done.=0A> >> = =0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 {=C2=A0 =C2=A0 =0A> >> > +=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 grub_errno =3D GRUB_ERR_NONE;=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 unsigned long num =3D grub_strtoul (args[1], 0, 0)= ;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (grub_errno !=3D GRUB_ERR= _NONE || num > 0xFF)=C2=A0 /* Not a raw num or too high.=C2=A0 */=0A> >> > = +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return grub_error (grub_errno,= =0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "Target specifier must be of the = form (fdN) or "=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "(hdN), with 0 <=3D= N < 128; or a plain dec/hex "=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "num= ber between 0 and 255");=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else = mapto =3D (grub_uint8_t)num;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }=0A> >>= > +=C2=A0 =C2=A0 =C2=A0 =0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (mapto =3D=3D m= apfrom)=C2=A0 /* Reset to default.=C2=A0 */=0A> >> =0A> >> Same here.=0A> >= Done.=0A> >> =0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 grub_dprintf (MODNAME, "Removing the mapping for %= s (%02x)", args[0], mapfrom);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = drivemap_remove (mapfrom);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }=0A> >> >= +=C2=A0 =C2=A0 =C2=A0 else=C2=A0 /* Map.=C2=A0 */=0A> >> =0A> >> Please mo= ve the comment inside the braces below.=0A> > Done, and reworded.=0A> >> = =0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 grub_dprintf (MODNAME, "Mapping %s (%02x) to %02x\n", args[0], m= apfrom, mapto);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return drivema= p_set ((grub_uint8_t)mapto, mapfrom);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0= }=0A> >> > +=C2=A0 =C2=A0 }=0A> >> > +=0A> >> > +=C2=A0 return GRUB_ERR_NO= NE;=0A> >> > +}=0A> >> > +=0A> >> > +typedef struct __attribute__ ((packed)= ) int13map_node=0A> >> > +{=0A> >> > +=C2=A0 grub_uint8_t disknum;=0A> >> >= +=C2=A0 grub_uint8_t mapto;=0A> >> > +} int13map_node_t;=0A> >> > +=0A> >>= > +/* The min amount of mem that must remain free after installing the han= dler.=0A> >> > +=C2=A0 32 KiB is just above 0x7C00-0x7E00, where the bootse= ctor is loaded.=C2=A0 */=0A> >> > +#define MIN_FREE_MEM_KB 32=0A> >> > +#de= fine INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) - ((grub_uint8_t*)&grub_drivem= ap_int13_handler_base) )=0A> >> > +#define INT13H_REBASE(x) ( (void*) (((gr= ub_uint8_t*)handler_base) + (x)) )=0A> >> > +#define INT13H_TONEWADDR(x) IN= T13H_REBASE( INT13H_OFFSET( x ) )=0A> >> > +=0A> >> > +/* Int13h handler in= staller - reserves conventional memory for the handler,=0A> >> > +=C2=A0 co= pies it over and sets the IVT entry for int13h.=C2=A0 =0A> >> > +=C2=A0 Thi= s code rests on the assumption that GRUB does not activate any kind of=0A> = >> > +=C2=A0 memory mapping apart from identity paging, since it accesses r= ealmode=0A> >> > +=C2=A0 structures by their absolute addresses, like the I= VT at 0 or the BDA at=0A> >> > +=C2=A0 0x400; and transforms a pmode pointe= r into a rmode seg:off far ptr.=C2=A0 */=0A> >> > +static grub_err_t=0A> >>= > +install_int13_handler (void)=0A> >> > +{=0A> >> > +=C2=A0 grub_size_t e= ntries =3D 0;=0A> >> > +=C2=A0 drivemap_node_t *curentry =3D drivemap;=0A> = >> > +=C2=A0 while (curentry)=C2=A0 /* Count entries to prepare a contiguou= s map block.=C2=A0 */=0A> >> =0A> >> ...=0A> > Comment moved up.=0A> >> =0A= > >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 entries++;=0A> >> > = +=C2=A0 =C2=A0 =C2=A0 curentry =3D curentry->next;=0A> >> > +=C2=A0 =C2=A0 = }=0A> >> > +=C2=A0 if (0 =3D=3D entries)=0A> >> =0A> >> I know this is what= you prefer, but can you change this nevertheless?=0A> > I refer to my obje= ction near the top of the post.=0A> =0A> I know you object, but did you cha= nge it?=0ADone.=0A> =0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_dprintf (MODNAME, = "No drives marked as remapped, installation of"=0A> >> > +=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "an int13h handler is not req= uired.");=0A> >> > +=C2=A0 =C2=A0 =C2=A0 return GRUB_ERR_NONE;=C2=A0 /* No = need to install the int13h handler.=C2=A0 */=0A> >> > +=C2=A0 =C2=A0 }=0A> = >> > +=C2=A0 else=0A> >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 = grub_dprintf (MODNAME, "Installing int13h handler...\n");=0A> >> > +=C2=A0 = =C2=A0 =C2=A0 grub_uint32_t *ivtslot =3D (grub_uint32_t*)0x0000004c;=0A> >>= > +=C2=A0 =C2=A0 =C2=A0 =0A> >> > +=C2=A0 =C2=A0 =C2=A0 /* Save the pointe= r to the old int13h handler.=C2=A0 */=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_d= rivemap_int13_oldhandler =3D *ivtslot;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_= dprintf (MODNAME, "Old int13 handler at %04x:%04x\n",=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (grub_drivemap_int13_o= ldhandler >> 16) & 0x0ffff,=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 grub_drivemap_int13_oldhandler & 0x0ffff);=0A> = >> > +=0A> >> > +=C2=A0 =C2=A0 =C2=A0 /* Reserve a section of conventional = memory as "BIOS memory" for handler:=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 = BDA offset 0x13 contains the top of such memory.=C2=A0 */=0A> >> > +=C2=A0 = =C2=A0 =C2=A0 grub_uint16_t *bpa_freekb =3D (grub_uint16_t*)0x00000413;=0A>= >> > +=C2=A0 =C2=A0 =C2=A0 grub_dprintf (MODNAME, "Top of conventional mem= ory: %u KiB\n", *bpa_freekb);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_size_t to= tal_size =3D grub_drivemap_int13_size=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 + (e= ntries + 1) * sizeof(int13map_node_t);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_= uint16_t payload_sizekb =3D (total_size >> 10) +=0A> >> > +=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (((total_size % 1024) =3D=3D 0) ? 0 = : 1);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if ((*bpa_freekb - payload_sizekb) < M= IN_FREE_MEM_KB)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return grub_error (GR= UB_ERR_OUT_OF_MEMORY, "refusing to install"=0A> >> > +=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "int1= 3 handler, not enough free memory after");=0A> >> > +=C2=A0 =C2=A0 =C2=A0 g= rub_dprintf (MODNAME, "Payload is %u b long, reserving %u Kb\n",=0A> >> > += =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 total_size, payload_sizekb);=0A> >> > +=C2=A0 =C2=A0 = =C2=A0 *bpa_freekb -=3D payload_sizekb;=0A> >> > +=0A> >> > +=C2=A0 =C2=A0 = =C2=A0 /* Copy int13h handler chunk to reserved area.=C2=A0 */=0A> >> > += =C2=A0 =C2=A0 =C2=A0 grub_uint8_t *handler_base =3D (grub_uint8_t*)(*bpa_fr= eekb << 10);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_dprintf (MODNAME, "Copying= int13 handler to: %p\n", handler_base);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 gru= b_memcpy (handler_base, &grub_drivemap_int13_handler_base,=0A> >> > +=C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 grub_drivemap_int1= 3_size);=0A> >> > +=0A> >> > +=C2=A0 =C2=A0 =C2=A0 /* Copy the mappings to = the reserved area.=C2=A0 */=0A> >> > +=C2=A0 =C2=A0 =C2=A0 curentry =3D dri= vemap;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_size_t i;=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 int13map_node_t *handler_map =3D (int13map_node_t*)=0A> >> > += =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 INT13H_TONEWADDR (&grub_drivemap_int13_mapstart);=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n"= , handler_map);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 for (i =3D 0; i < entries &&= curentry; i++, curentry =3D curentry->next)=0A> >> > +=C2=A0 =C2=A0 =C2=A0= =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 handler_map[i].diskn= um =3D curentry->newdrive;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 han= dler_map[i].mapto =3D curentry->redirto;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x\n", i,=0A> >> > = +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2= =A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 handler_map[i].disknum, handler_m= ap[i].mapto);=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }=0A> >> > +=C2=A0 =C2= =A0 =C2=A0 /* Signal end-of-map.=C2=A0 */=0A> >> > +=C2=A0 =C2=A0 =C2=A0 ha= ndler_map[i].disknum =3D 0;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 handler_map[i].m= apto =3D 0;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_dprintf (MODNAME, "\t#%d: 0= x%02x <- 0x%02x (end)\n", i,=0A> >> > +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2= =A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 handler_map[i]= .disknum, handler_map[i].mapto);=0A> >> > +=0A> >> > +=C2=A0 =C2=A0 =C2=A0 = /* Install our function as the int13h handler in the IVT.=C2=A0 */=0A> >> >= +=C2=A0 =C2=A0 =C2=A0 grub_uint32_t ivtentry =3D ((grub_uint32_t)handler_b= ase) << 12; /* Segment address.=C2=A0 */=0A> >> > +=C2=A0 =C2=A0 =C2=A0 ivt= entry |=3D (grub_uint16_t) INT13H_OFFSET(grub_drivemap_int13_handler);=0A> = >> > +=C2=A0 =C2=A0 =C2=A0 grub_dprintf (MODNAME, "New int13 handler IVT po= inter: %04x:%04x\n",=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);=0A> >= > > +=C2=A0 =C2=A0 =C2=A0 *ivtslot =3D ivtentry;=0A> >> > +=C2=A0 =C2=A0 = =C2=A0 =0A> >> > +=C2=A0 =C2=A0 =C2=A0 return GRUB_ERR_NONE;=0A> >> > +=C2= =A0 =C2=A0 }=0A> >> > +}=0A> >> > +=0A> >> > +GRUB_MOD_INIT (drivemap)=0A> = >> > +{=0A> >> > +=C2=A0 (void) mod;=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 = =C2=A0=C2=A0=C2=A0 /* Stop warning.=C2=A0 */=0A> >> > +=C2=A0 grub_register= _command (MODNAME, grub_cmd_drivemap,=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 GRUB_COMMAND_FLAG_= BOTH,=0A> >> > +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 MODNAME " -s= | -r | (hdX) newdrivenum",=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "Manage the BIOS drive map= pings", options);=0A> >> > +=C2=A0 insthandler_hook =3D grub_loader_registe= r_preboot (&install_int13_handler, 1);=0A> >> > +}=0A> >> > +=0A> >> > +GRU= B_MOD_FINI (drivemap)=0A> >> > +{=0A> >> > +=C2=A0 grub_loader_unregister_p= reboot (insthandler_hook);=0A> >> > +=C2=A0 insthandler_hook =3D 0;=0A> >> = > +=C2=A0 grub_unregister_command (MODNAME);=0A> >> > +}=0A> >> > +=0A> >> = > Index: commands/i386/pc/drivemap_int13h.S=0A> >> > =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=0A> >> > --- commands/i386/pc/drivemap_int13= h.S=C2=A0=C2=A0=C2=A0 (revisi=C3=B3n: 0)=0A> >> > +++ commands/i386/pc/driv= emap_int13h.S=C2=A0=C2=A0=C2=A0 (revisi=C3=B3n: 0)=0A> >> > @@ -0,0 +1,118 = @@=0A> >> > +/*=0A> >> > + *=C2=A0 GRUB=C2=A0 --=C2=A0 GRand Unified Bootlo= ader=0A> >> > + *=C2=A0 Copyright (C) 1999,2000,2001,2002,2003,2005,2006,20= 07,2008 Free Software Foundation, Inc.=0A> >> > + *=0A> >> > + *=C2=A0 GRUB= is free software: you can redistribute it and/or modify=0A> >> > + *=C2=A0= it under the terms of the GNU General Public License as published by=0A> >= > > + *=C2=A0 the Free Software Foundation, either version 3 of the License= , or=0A> >> > + *=C2=A0 (at your option) any later version.=0A> >> > + *=0A= > >> > + *=C2=A0 GRUB is distributed in the hope that it will be useful,=0A= > >> > + *=C2=A0 but WITHOUT ANY WARRANTY; without even the implied warrant= y of=0A> >> > + *=C2=A0 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE= .=C2=A0 See the=0A> >> > + *=C2=A0 GNU General Public License for more deta= ils.=0A> >> > + *=0A> >> > + *=C2=A0 You should have received a copy of the= GNU General Public License=0A> >> > + *=C2=A0 along with GRUB.=C2=A0 If no= t, see .=0A> >> > + */=0A> >> > +=0A> >> > += =0A> >> > +/*=0A> >> > + * Note: These functions defined in this file may b= e called from C.=0A> >> > + *=C2=A0 =C2=A0 =C2=A0 Be careful of that you mu= st not modify some registers. Quote=0A> >> > + *=C2=A0 =C2=A0 =C2=A0 from g= cc-2.95.2/gcc/config/i386/i386.h:=0A> >> > +=0A> >> > +=C2=A0 1 for registe= rs not available across function calls.=0A> >> > +=C2=A0 These must include= the FIXED_REGISTERS and also any=0A> >> > +=C2=A0 registers that can be us= ed without being saved.=0A> >> > +=C2=A0 The latter must include the regist= ers where values are returned=0A> >> > +=C2=A0 and the register where struc= ture-value addresses are passed.=0A> >> > +=C2=A0 Aside from that, you can = include as many other registers as you like.=0A> >> > +=0A> >> > +=C2=A0 ax= ,dx,cx,bx,si,di,bp,sp,st,st1,st2,st3,st4,st5,st6,st7,arg=0A> >> > +{=C2=A0 = 1, 1, 1, 0, 0, 0, 0, 1, 1,=C2=A0 1,=C2=A0 1,=C2=A0 1,=C2=A0 1,=C2=A0 1,=C2= =A0 1,=C2=A0 1,=C2=A0 1 }=0A> >> > + */=0A> >> > +=0A> >> > +/*=0A> >> > + = * Note: GRUB is compiled with the options -mrtd and -mregparm=3D3.=0A> >> >= + *=C2=A0 =C2=A0 =C2=A0 So the first three arguments are passed in %eax, %= edx, and %ecx,=0A> >> > + *=C2=A0 =C2=A0 =C2=A0 respectively, and if a func= tion has a fixed number of arguments=0A> >> > + *=C2=A0 =C2=A0 =C2=A0 and t= he number if greater than three, the function must return=0A> >> > + *=C2= =A0 =C2=A0 =C2=A0 with "ret $N" where N is ((the number of arguments) - 3) = * 4.=0A> >> > + */=0A> >> > +=0A> >> > +#include =0A> >> > += =0A> >> > +#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - grub_drivemap_int1= 3_handler_base)=0A> >> > +=0A> >> > +/* Copy starts here. When deployed, th= is label must be segment-aligned */=0A> >> > +VARIABLE(grub_drivemap_int13_= handler_base)=0A> >> > +=0A> >> > +VARIABLE(grub_drivemap_int13_oldhandler)= =0A> >> > +=C2=A0 .word 0xdead, 0xbeef=0A> >> > +/* Drivemap module - INT 1= 3h handler - BIOS HD map */=0A> >> > +/* We need to use relative addressing= , and with CS to top it all, since we=0A> >> > +=C2=A0 must make as few cha= nges to the registers as possible.=C2=A0 IP-relative=0A> >> > +=C2=A0 addre= ssing like on amd64 would make life way easier here. */=0A> >> > +.code16= =0A> >> > +FUNCTION(grub_drivemap_int13_handler)=0A> >> > +=C2=A0 push %bp= =0A> >> > +=C2=A0 mov %sp, %bp=0A> >> > +=C2=A0 push %ax=C2=A0 /* We'll nee= d it later to determine the used BIOS function */=0A> >> > +=0A> >> > +=C2= =A0 /* Map the drive number (always in DL?) */=0A> >> > +=C2=A0 push %ax=0A= > >> > +=C2=A0 push %bx=0A> >> > +=C2=A0 push %si=0A> >> > +=C2=A0 mov $GRU= B_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx=0A> >> > +=C2= =A0 xor %si, %si=0A> >> > +1:movw %cs:(%bx,%si), %ax=0A> >> > +=C2=A0 cmp %= ah, %al=0A> >> > +=C2=A0 jz 3f /* DRV=3DDST =3D> map end - drive not remapp= ed, leave DL as-is */=0A> >> > +=C2=A0 cmp %dl, %al=0A> >> > +=C2=A0 jz 2f = /* Found - drive remapped, modify DL */=0A> >> > +=C2=A0 add $2, %si=0A> >>= > +=C2=A0 jmp 1b /* Not found, but more remaining, loop=C2=A0 */=0A> >> > = +2:mov %ah, %dl=0A> >> > +3:pop %si=0A> >> > +=C2=A0 pop %bx=0A> >> > +=C2= =A0 xchgw %ax, -4(%bp) /* Recover the old AX and save the map entry for lat= er */=0A> >> > +=C2=A0 =0A> >> > +=C2=A0 push %bp=0A> >> > +=C2=A0 /* Simul= ate interrupt call: push flags and do a far call in order to set=0A> >> > += =C2=A0 =C2=A0 the stack the way the old handler expects it so that its iret= works */=0A> >> > +=C2=A0 push 6(%bp)=0A> >> > +=C2=A0 movw (%bp), %bp=C2= =A0 /* Restore the caller BP (is this needed and/or sensible?) */=0A> >> > = +=C2=A0 lcall *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandl= er)=0A> >> > +=C2=A0 pop %bp /* The pushed flags were removed by iret */=0A= > >> > +=C2=A0 /* Set the saved flags to what the int13h handler returned *= /=0A> >> > +=C2=A0 push %ax=0A> >> > +=C2=A0 pushf=0A> >> > +=C2=A0 pop %ax= =0A> >> > +=C2=A0 movw %ax, 6(%bp)=0A> >> > +=C2=A0 pop %ax=0A> >> > +=0A> = >> > +=C2=A0 /* Reverse map any returned drive number if the data returned = includes it.=C2=A0 =0A> >> > +=C2=A0 =C2=A0 The only func that does this se= ems to be origAH =3D 0x08, but many BIOS=0A> >> > +=C2=A0 =C2=A0 refs say r= etDL =3D # of drives connected.=C2=A0 However, the GRUB Legacy code=0A> >> = > +=C2=A0 =C2=A0 treats this as the _drive number_ and "undoes" the remappi= ng.=C2=A0 Thus,=0A> >> > +=C2=A0 =C2=A0 this section has been disabled for = testing if it's required */=0A> >> > +#=C2=A0 cmpb $0x08, -1(%bp) /* Caller= 's AH */=0A> >> > +#=C2=A0 jne 4f=0A> >> > +#=C2=A0 xchgw %ax, -4(%bp) /* M= ap entry used */=0A> >> > +#=C2=A0 cmp %ah, %al=C2=A0 /* DRV=3DDST =3D> dri= ve not remapped */=0A> >> > +#=C2=A0 je 4f=0A> >> > +#=C2=A0 mov %ah, %dl= =C2=A0 /* Undo remap */=0A> >> > +=0A> >> > +4:mov %bp, %sp=0A> >> > +=C2= =A0 pop %bp=0A> >> > +=C2=A0 iret=0A> >> > +/* This label MUST be at the en= d of the copied block, since the installer code=0A> >> > +=C2=A0 reserves a= dditional space for mappings at runtime and copies them over it */=0A> >> >= +.align 2=0A> >> > +VARIABLE(grub_drivemap_int13_mapstart)=0A> >> > +/* Co= py stops here */=0A> >> > +.code32=0A> >> > +VARIABLE(grub_drivemap_int13_s= ize)=0A> >> > +=C2=A0 .word GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13= _size)=0A> >> > +=0A> >> > Index: conf/i386-pc.rmk=0A> >> > =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=0A> >> > --- conf/i386-pc.rmk=C2=A0=C2= =A0=C2=A0 (revisi=C3=B3n: 1766)=0A> >> > +++ conf/i386-pc.rmk=C2=A0=C2=A0= =C2=A0 (copia de trabajo)=0A> >> > @@ -158,7 +158,7 @@=0A> >> >=C2=A0 =C2= =A0=C2=A0=C2=A0 vbe.mod vbetest.mod vbeinfo.mod video.mod gfxterm.mod \=0A>= >> >=C2=A0 =C2=A0=C2=A0=C2=A0 videotest.mod play.mod bitmap.mod tga.mod cp= uid.mod serial.mod=C2=A0=C2=A0=C2=A0 \=0A> >> >=C2=A0 =C2=A0=C2=A0=C2=A0 at= a.mod vga.mod memdisk.mod jpeg.mod png.mod pci.mod lspci.mod \=0A> >> > -= =C2=A0=C2=A0=C2=A0 aout.mod _bsd.mod bsd.mod=0A> >> > +=C2=A0=C2=A0=C2=A0 a= out.mod _bsd.mod bsd.mod drivemap.mod=0A> >> >=C2=A0 =0A> >> >=C2=A0 # For = biosdisk.mod.=0A> >> >=C2=A0 biosdisk_mod_SOURCES =3D disk/i386/pc/biosdisk= .c=0A> >> > @@ -325,4 +325,11 @@=0A> >> >=C2=A0 bsd_mod_CFLAGS =3D $(COMMON= _CFLAGS)=0A> >> >=C2=A0 bsd_mod_LDFLAGS =3D $(COMMON_LDFLAGS)=0A> >> >=C2= =A0 =0A> >> > +# For drivemap.mod.=0A> >> > +drivemap_mod_SOURCES =3D comma= nds/i386/pc/drivemap.c \=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 commands/i386/pc/drivemap_int13h.S= =0A> >> > +drivemap_mod_ASFLAGS =3D $(COMMON_ASFLAGS)=0A> >> > +drivemap_mo= d_CFLAGS =3D $(COMMON_CFLAGS)=0A> >> > +drivemap_mod_LDFLAGS =3D $(COMMON_L= DFLAGS)=0A> >> > +=0A> >> >=C2=A0 include $(srcdir)/conf/common.mk=0A> >> >= Index: include/grub/loader.h=0A> >> > =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=0A> >> > --- include/grub/loader.h=C2=A0=C2=A0=C2=A0 (revis= i=C3=B3n: 1766)=0A> >> > +++ include/grub/loader.h=C2=A0=C2=A0=C2=A0 (copia= de trabajo)=0A> >> > @@ -37,6 +37,22 @@=0A> >> >=C2=A0 /* Unset current lo= ader, if any.=C2=A0 */=0A> >> >=C2=A0 void EXPORT_FUNC(grub_loader_unset) (= void);=0A> >> >=C2=A0 =0A> >> > +typedef struct hooklist_node *grub_preboot= _hookid;=0A> >> > +=0A> >> > +/* Register a function to be called before th= e boot hook.=C2=A0 Returns an id that=0A> >> > +=C2=A0 can be later used to= unregister the preboot (i.e. on module unload).=C2=A0 If=0A> >> > +=C2=A0 = abort_on_error is set, the boot sequence will abort if any of the registere= d=0A> >> > +=C2=A0 functions return anything else than GRUB_ERR_NONE.=0A> >= > > +=C2=A0 On error, the return value will compare equal to 0 and the erro= r information=0A> >> > +=C2=A0 will be available in errno and errmsg.=C2=A0= However, if the call is successful=0A> >> > +=C2=A0 those variables are _n= ot_ modified. */=0A> >> =0A> >> No need to mention errmsg, it's internal to= GRUB.=C2=A0 As for errno (which=0A> >> is grub_errno, actually) it does no= t need to be mentioned, otherwise=0A> >> we would have to do so everywhere.= =C2=A0 Please capitalize HOOK and=0A> >> ABORT_ON_ERROR in the comments abo= ve.=0A> > Done. "hook" removed because it referred to the loader module boo= t=0A> > function.=0A> >> =0A> >> > +grub_preboot_hookid EXPORT_FUNC(grub_lo= ader_register_preboot)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (grub_e= rr_t (*hook) (void), int abort_on_error);=0A> >> > +=0A> >> > +/* Unregiste= r a preboot hook by the id returned by loader_register_preboot.=0A> >> > += =C2=A0 This functions becomes a no-op if no such function is registered */= =0A> >> > +void EXPORT_FUNC(grub_loader_unregister_preboot) (grub_preboot_h= ookid id);=0A> >> > +=0A> >> >=C2=A0 /* Call the boot hook in current loade= r. This may or may not return,=0A> >> >=C2=A0 =C2=A0 depending on the setti= ng by grub_loader_set.=C2=A0 */=0A> >> =0A> >> Nitpick: "loader.=C2=A0 This= ..."=0A> > Are you a bot? =C2=AC=C2=AC Corrected=0A> =0A> It would make lik= e much simpler if I were ;-).=C2=A0 What makes you think=0A> so?=0AYour abi= lity to spot these kind of smallish things, like the "deltion" word and the= lack of a double space. You even write normal text with double spaces!=0A= =0A> =0A> >> >=C2=A0 grub_err_t EXPORT_FUNC(grub_loader_boot) (void);=0A> >= > > Index: kern/loader.c=0A> >> > =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=0A> >> > --- kern/loader.c=C2=A0=C2=A0=C2=A0 (revisi=C3=B3n: 1= 766)=0A> >> > +++ kern/loader.c=C2=A0=C2=A0=C2=A0 (copia de trabajo)=0A> >>= > @@ -61,11 +61,82 @@=0A> >> >=C2=A0 =C2=A0 grub_loader_loaded =3D 0;=0A> = >> >=C2=A0 }=0A> >> >=C2=A0 =0A> >> > +struct hooklist_node=0A> >> > +{=0A>= >> > +=C2=A0 grub_err_t (*hook) (void);=0A> >> > +=C2=A0 int abort_on_erro= r;=0A> >> > +=C2=A0 struct hooklist_node *next;=0A> >> > +};=0A> >> > +=0A>= >> > +static struct hooklist_node *preboot_hooks =3D 0;=0A> >> > +=0A> >> = > +grub_preboot_hookid=0A> >> > +grub_loader_register_preboot(grub_err_t (*= hook) (void), int abort_on_error)=0A> >> > +{=0A> >> > +=C2=A0 if (!hook)= =0A> >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_error (GRUB_= ERR_BAD_ARGUMENT, "preboot hook must not be NULL");=0A> >> > +=C2=A0 =C2=A0= =C2=A0 return 0;=0A> >> > +=C2=A0 =C2=A0 }=0A> >> > +=C2=A0 grub_preboot_h= ookid newentry =3D grub_malloc (sizeof (struct hooklist_node));=0A> >> =0A>= >> Mixed declarations/code.=0A> > Oops, sorry. I put most of my attention = on drivemap.c (and even then=0A> > many comments slipped through). Correcte= d.=0A> =0A> Please re-check them, I might have missed things this time...= =0A> =0A> >> > +=C2=A0 if (!newentry)=0A> >> > +=C2=A0 =C2=A0 {=0A> >> > += =C2=A0 =C2=A0 =C2=A0 grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot alloc a ho= okinfo structure");=0A> >> > +=C2=A0 =C2=A0 =C2=A0 return 0;=0A> >> > +=C2= =A0 =C2=A0 }=0A> >> > +=C2=A0 else=0A> >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2= =A0 =C2=A0 =C2=A0 newentry->hook =3D hook;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 n= ewentry->abort_on_error =3D abort_on_error;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 = newentry->next =3D preboot_hooks;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 preboot_ho= oks =3D newentry;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 return newentry;=0A> >> > = +=C2=A0 =C2=A0 }=0A> >> > +}=0A> >> > +=0A> >> > +void=0A> >> > +grub_loade= r_unregister_preboot(grub_preboot_hookid id)=0A> >> =0A> >> "preboot (grub"= =0A> > Corrected on both functions ;)=0A> >> =0A> >> > +{=0A> >> > +=C2=A0 = grub_preboot_hookid entry =3D 0;=0A> >> > +=C2=A0 grub_preboot_hookid searc= h =3D preboot_hooks;=0A> >> > +=C2=A0 grub_preboot_hookid previous =3D 0;= =0A> >> > +=0A> >> > +=C2=A0 if (0 =3D=3D id)=0A> >> > +=C2=A0 =C2=A0 retur= n;=0A> >> =0A> >> ...=0A> > ... xD=0A> >> =0A> >> > +=C2=A0 while (search)= =0A> >> > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (search =3D=3D= id)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 entry =3D search;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 break;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }=0A> >> > +=C2=A0 =C2=A0 = =C2=A0 previous =3D search;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 search =3D searc= h->next;=0A> >> > +=C2=A0 =C2=A0 }=0A> >> > +=C2=A0 if (entry) /* Found */= =0A> >> =0A> >> ...=0A> > Comment removed, was unnecessary.=0A> >> =0A> >> = > +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (previous)=0A> >> > += =C2=A0 =C2=A0 =C2=A0 =C2=A0 previous->next =3D entry->next;=0A> >> > +=C2= =A0 =C2=A0 =C2=A0 else preboot_hooks =3D entry->next; /* Entry was head of = list */=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_free (entry);=0A> >> > +=C2=A0 = =C2=A0 }=0A> >> > +}=0A> >> > +=0A> >> >=C2=A0 grub_err_t=0A> >> >=C2=A0 gr= ub_loader_boot (void)=0A> >> >=C2=A0 {=0A> >> >=C2=A0 =C2=A0 if (! grub_loa= der_loaded)=0A> >> >=C2=A0 =C2=A0 =C2=A0 return grub_error (GRUB_ERR_NO_KER= NEL, "no loaded kernel");=0A> >> > +=C2=A0 =0A> >> > +=C2=A0 grub_preboot_h= ookid entry =3D preboot_hooks;=0A> >> =0A> >> Mixed declarations/code.=0A> = > Moved the whole line up.=0A> >> =0A> >> > +=C2=A0 while (entry)=0A> >> > = +=C2=A0 =C2=A0 {=0A> >> > +=C2=A0 =C2=A0 =C2=A0 grub_err_t possible_error = =3D entry->hook();=0A> >> > +=C2=A0 =C2=A0 =C2=A0 if (possible_error !=3D G= RUB_ERR_NONE && entry->abort_on_error)=0A> >> > +=C2=A0 =C2=A0 =C2=A0 =C2= =A0 return possible_error;=0A> >> > +=C2=A0 =C2=A0 =C2=A0 entry =3D entry->= next;=0A> >> > +=C2=A0 =C2=A0 }=0A> >> >=C2=A0 =0A> >> >=C2=A0 =C2=A0 if (g= rub_loader_noreturn)=0A> >> >=C2=A0 =C2=A0 =C2=A0 grub_machine_fini ();=0A>= =0A> =0A> =0A> _______________________________________________=0A> Grub-de= vel mailing list=0A> Grub-devel@gnu.org=0A> http://lists.gnu.org/mailman/li= stinfo/grub-devel=0A=0AEl mar, 05-08-2008 a las 13:28 +0200, Marco Gerards = escribi=C3=B3:=0A> Hi,=0A> =0A> Javier Mart=C3=ADn w= rites:=0A> =0A> >> > There is, however, one point in which I keep my object= ion: comparisons=0A> >> > between a variable and a constant should be of th= e form CONSTANT =3D=3D=0A> >> > variable and not in the reverse order, sinc= e an erroneous but quite=0A> >> > possible change of =3D=3D for =3D results= in a compile-time error instead of a=0A> >> > _extremely_ difficult to tra= ce runtime bug. Such kind of bugs are quite=0A> >> > excruciating to find i= n userspace applications within an IDE, so I can't=0A> >> > even consider t= he pain to debug them in a bootloader.=0A> >> =0A> >> I understand your con= cern, nevertheless, can you please change it?=0A> > You understand my conce= rn, but seemingly do not understand that in order=0A> > to conform to the H= oly Coding Style you are asking me to write code that=0A> > can become bugg= y (and with a very hard to trace bug) with a simple=0A> > deltion! (point: = did you notice that last word _without_ a spelling=0A> > checker? Now try t= o do so in a multithousand-line program).=0A> =0A> BTW, your patch still co= ntains this, can you please change it before I=0A> go over it again?=0A> = =0A> I know people who claim that this code will become buggy because we=0A= > write it in C.=C2=A0 Should we start accepting patches to rewrite GRUB in= =0A> Haskell or whatever? :-)=0AWhat about Ada? The stock GCC has Ada suppo= rt ^^=0A> =0A> Really, as a maintainer I should set some standards and stic= k to it.=0A> Of course not everyone will like me and sometimes I have to ac= t like a=0A> jerk.=C2=A0 But I rather be a jerk, than committing code that = do not meet=0A> my expectations.=C2=A0 But please understand, this contribu= tion is highly=0A> appreciated.=C2=A0 However, we want to have something ma= intainable for the=0A> far future as well :-)=0AI understand these kind of = concerns, particularly seeing how GRUB Legacy=0Aended - tangled, unscalable= spaghetti code. You're not a jerk, just a=0Abit obsessive, but that's fine= when trying to handle herds of us=0Aall-important devs which think all we = do is The Right Thing (tm) and=0Aothers are heretics to The Truth.=0A> =0A>= --=0A> Marco=0A> =0A> =0A> =0A> __________________________________________= _____=0A> Grub-devel mailing list=0A> Grub-devel@gnu.org=0A> http://lists.g= nu.org/mailman/listinfo/grub-devel=0A=0AOk, so here is the new version of t= he patch, with a new drivemap.h=0Aheader and the =3D=3D arguments put in th= e Holy Ordering ^^. I hope I didn't=0Aforget anything this time. Here is th= e new CL entry:=0A=0A=EF=BB=BF=EF=BB=BF2008-08-XX=C2=A0 Javier Martin=C2=A0= =0A=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF* command= s/i386/pc/drivemap.c: New file.=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 * commands/i3= 86/pc/drivemap_int13h.S: New file.=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF*= conf/i386-pc.rmk (pkglib_MODULES): Added drivemap.mod.=0A=C2=A0 =C2=A0 =C2= =A0 =C2=A0 (drivemap_mod_SOURCES): New variable.=0A=C2=A0 =C2=A0 =C2=A0 =C2= =A0 (drivemap_mod_ASFLAGS): Likewise.=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB= =BF(drivemap_mod_CFLAGS): Likewise.=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF= (drivemap_mod_LDFLAGS): Likewise.=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF* = include/grub/loader.h (grub_loader_register_preboot): New=0A=C2=A0 =C2=A0 = =C2=A0 =C2=A0 function prototype.=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 (grub_loade= r_unregister_preboot): Likewise.=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 (grub_preboo= t_hookid): New typedef.=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 =EF=BB=BF* kern/loade= r.c =EF=BB=BF(grub_loader_register_preboot): New function.=0A=C2=A0 =C2=A0 = =C2=A0 =C2=A0 (grub_loader_unregister_preboot): Likewise.=0A=C2=A0 =C2=A0 = =C2=A0 =C2=A0 (preboot_hooks): New variable.=0A=C2=A0 =C2=A0 =C2=A0 =C2=A0 = (grub_loader_boot): Call the list of preboot-hooks before the=0A=C2=A0 =C2= =A0 =C2=A0 =C2=A0 actual loader.=0A=0ABy the way, is there anything I can d= o to make `svn up' updates less=0Atraumatic? I don't want to search each "C= " file for "<<<<<< mine" lines=0Aand correct them: is there any tool to do = this with a workflow not=0Aunlike that of Gentoo's `etc-update'?=0A=0AHabbi= t=0A=0A=0A=0A Add more friends to your messenger and enjoy! Go to http= ://in.messenger.yahoo.com/invite/ --0-1133575639-1218033794=:55348 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable

Hi,

=0A

 

=0A

Could you let all know how to use your= drivemap command.Is it similar to the map command in legacy grub.

=0A 

=0A

drivemap (hd0) (hd1)

=0A

drivemap (hd1) (hd0)

=0A<= P> 

=0A

with the grub.cfg to be

=0A

 

=0A

menuent= ry "Linux" {

=0A

linux ....

=0A

initrd ...

=0A

}

=0A

&n= bsp;

=0A

menuentry "Windows" {

=0A

      = ;  set root=3D(hd1,1)

=0A

        c= hainloader +1

=0A

}

=0A

 

=0A
Viswesh
=0A
= =0A
lordhabbit@gmail.com> writes:
>
> >> Anyway= , since "they" are more likely to maintain the code in
> >> th= e long run than you, in general, the question is whether
> >> = the code is more likely to become buggy by their hacking on
> >&g= t; it, if it follows project coding style or someone else's
> >&g= t; (your) "safer" coding style.  Likely it's safer if using a
> >> consist= ent programming style.  Although I personally don't
> >> = see that it's very helpful to have a style that makes things
> >&= gt; down to the order of "=3D=3D" arguments be consistent within the
&g= t; >> project; haphazard only slows reading the tiniest bit, and I > >> don't think it makes a different what order the arguments a= re...
> > =EF=BB=BF
> > Hmm... I was partially expecting = a flamefest to start. Pity ^^
> > Well, let's spill a little napal= m: the GNU style bracing is extremely
> > silly!! Why the hell are= the "if" and "else" blocks indented differently
> > in this examp= le?
> >  if (condition)
> >    return 0;> >  else
> >    {
> >   = ;   return -1;
> >    }
> > Nah, I'm not really bringing that issue, I was just joking, and in fact
> > I'= m reconsidering my objections to the operator=3D=3D arguments order rule,> > even though I still consider my style safer and more sensible. = If
> > someone else wants to express their opinion on that issue, = do it fast
> > before I completely submit to Master Marco's will := D
>
> Please don't be sarcastic, start flame wars or call name= s.  It will not
> help anyone.

Ok, sorry, picturing you a= s a whip-cracking dominatrix was really not proper ^^. I was just joking ab= out how fast can flamewars be started.

>
> --
> Marc= o
>
>
>
> ______________________________________= _________
> Grub-devel mailing list
> Grub-devel@gnu.org> http://lists.gnu.org/mailman/listinfo/grub-devel
= =EF=BB=BF
El mar, 05-08-2008 a las 13:23 +0200, Marco Gerards escribi=C3= =B3:
> Hi Javier,
>
> Javier Mart=C3=ADn <lordha= bbit@gmail.com> writes:
>
> > El lun, 04-08-2008 a l= as 22:51 +0200, Marco Gerards escribi=C3=B3:
> >> Javier Mart= =C3=ADn <lordhabbit@gmail.com> writes:
> >>
= > >> > After your latest replay, I "reevaluated" my stubbornnes= s WRT some of
> >> > your advices, and I've changed a few th= ings:
> >> >
> >> > - Variables are now decla= red (and, if possible, initialized) before
> >> > preconditi= on 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
> >&= gt; > after the "else" line:
> >> >      g= rub_uint32_t *ivtslot;
> >> >      grub_uint1= 6_t *bpa_freekb;
> >> >      grub_size_t tota= l_size;
> >> >      grub_uint16_t payload_siz= ekb;
> >> >      grub_uint8_t *handler_base;<= BR>> >> >      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 endl= essly but we will simply stick to the GNU Coding
> >> Standards as you might expect.
> >>
> >> I hope y= ou can appreciate that all code of GRUB has the same coding
> >>= ; style, if you like this style or not.
> > Big sigh. Function mod= ified 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<= BR>> > 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 o= f 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 GC= S.

Which is fine, but the order of the arguments to =3D=3D is specif= ied nowhere in the GCS: the order you defend only appears in examples less = than five times. Thus, your insistence is a matter of internal consistence = within GRUB and is only based on examples in the GCS. Additionally, your pr= evious suggestion to change checks line "entries =3D=3D 0 " to "!entries" a= re _against_ the examples of the GCS, even for pointers.

Then, given= that the _examples_ in the GCS are non-authoritative, I was advocating a s= imple change for better resilience against future changes. You want to keep= consistency with the current sources, and that I understand, so I will per= form the required trivial changes in my code to use the style used in other= GRUB code, but I still think it is worse and more error-prone.

>
>
> >> > - Only one declaration per line: even thou= gh 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
> >&g= t; > into such ambiguities.
> >> > - Comments moved as yo= u required, reworded as needed
> >> > - Extensive printf sho= wing quasi-help in the "show" mode trimmed down to
> >> > ju= st the first sentence.
> >> > - Internal helper functions no= w use the standard error handling, i.e.
> >> > return grub_e= rror (err, fmt, ...)
> >> > - Comment about the strange "voi= d" type instead of "void*" rephrased to
> >> > be clearer> >>
> >> Thanks a lot!
> >>
> &g= t;> > There is, however, one point in which I keep my objection: comparisons
> >> > between a variable and a cons= tant should be of the form CONSTANT =3D=3D
> >> > variable a= nd not in the reverse order, since an erroneous but quite
> >> = > possible change of =3D=3D for =3D results in a compile-time error inst= ead of a
> >> > _extremely_ difficult to trace runtime bug. = Such kind of bugs are quite
> >> > excruciating to find in u= serspace 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 th= at 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 th= at last word _without_ a spelling
> > checker? Now try to do so in a= multithousand-line program).
>
> Yes, I did notice it immedia= tely.
>
> The coding style is not holy in any way.  Every= one has its own coding
> style.  You must understand I do not wa= nt to fully discuss it every
> time someone sends in a patch, especia= lly since it will not change
> anyways.

By the way, I just not= iced that you write _normal_ text with two spaces after a dot, just like co= mments in the code! o_O
>
> > While tools like `svn diff' c= an help in these kind of problems, their
> > utility is greatly di= minished if the offending change is part of a
> > multi-line chang= e. Besides, sometimes checks like "if (a=3Db)", or more
> > freque= ntly "if (a=3Df())" 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: =EF=BB=BFmaybe I'm dead wrong = and an arrogant brat in
> > my attempt to tackle the Holy GNU Codi= ng Standards, but I'd like to ask
> > input from more people on th= is.
>
> I will refuse patches with "if (a =3D f())", if that m= akes you sleep
> better ;-)
In fact the GCS discourages that use, = but allows the same in loop checks, particularly "while" loops.
>&nbs= p;
> >> > 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
> >> &= gt; 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 GR= UB2ish, like:
> >>
> >> map --bios=3D(hd0) --os=3D= (hd1)
> > I agree. What about "grub" for the source drive instead = of "os", with
> > "bios" being the target drive? I mean:
> &= gt;     drivemap --grub=3D(hd1) --bios=3D(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 funct= ionality which GRUB is active.  But it
> will change it for the = OS that is loaded.  So I do not think --grub=3D
> is a good idea= because of this.  As for GRUB Legacy, it confused a lot
> of pe= ople, 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 fir= st I'll review this patch, then you will send in a new one?

Of cours= e. I meant that I prefer to smash all "syntactic" changes in the patch befo= re introducing a functionality change that could lead to new syntactic issu= es.
> 
> >> Or so, perhaps the long argument names = can be chosen in a more clever
> >> way :-)
> >> &g= t; The new version of the patch is attached, and here is my suggested CLog:=
> >> >
> >> > 2008-08-XX  Javier Martin=   <lordhabbit@gmail.com>
> >>
> >= > (newline)
> >>
> >> >      =   =EF=BB=BF* commands/i386/pc/drivemap.c : New file.
> >> >        * commands/i386/pc/drivemap_int13h.S : New= file.
> >> >        =EF=BB=BF* conf/i38= 6-pc.rmk (pkglib_MODULES) : Added drivemap.mod
> >> >  =       (drivemap_mod_SOURCES) : New variable
> >>= >        (drivemap_mod_ASFLAGS) : Likewise
> = >> >        =EF=BB=BF(drivemap_mod_CFLAGS) : L= ikewise
> >> >        =EF=BB=BF(drivemap= _mod_LDFLAGS) : Likewise
> >> >        = =EF=BB=BF* include/grub/loader.h (grub_loader_register_preboot) : New
&g= t; >> >        function prototype. Register a = new pre-boot handler
> >>
> >> No need how the cha= nge is used or why it was added.
> >>
> >> >&nb= sp;       (grub_loader_unregister_preboot) : Likewise. Unreg= ister handler
> >>
> >> Same here.
> >> > >> >        (grub_preboot_hookid) : New = typedef. Registered hook "handle"
> >>
> >> Same h= ere.
> >>
> >> >        =EF= =BB=BF* kern/loader.c =EF=BB=BF(grub_loader_register_preboot) : New functio= n.
> >> >        (grub_loader_unregister= _preboot) : Likewise.
> >> >        (pre= boot_hooks) : New variable. Linked list of preboot hooks
> >> <= BR>> >> Same here.
> >>
> >> >  &= nbsp;     (grub_loader_boot) : Call the list of preboot-hooks bef= ore the
> >> >        actual loader
&= gt; >>
> >> What's the `=EF=BB=BF'?
> > The wha= t? o_O
>
> I see some weird character in your text.  My f= ont shows it as a block
> before every `*'.
I see nothing: "What's the `'?"= . Maybe it's some kind of tab?
>
> >> Please do not add = a space before the ":"
> > Ok, everything corrected. New CL entry= :
> >
> > =EF=BB=BF2008-08-XX  Javier Martin  &= lt;lordhabbit@gmail.com>
> >
> >   =     =EF=BB=BF* commands/i386/pc/drivemap.c: New file.
> &g= t;        * commands/i386/pc/drivemap_int13h.S: New fil= e.
> >        =EF=BB=BF* conf/i386-pc.rmk (pkg= lib_MODULES): Added drivemap.mod
> >        (d= rivemap_mod_SOURCES): New variable
> >        = (drivemap_mod_ASFLAGS): Likewise
> >        = =EF=BB=BF(drivemap_mod_CFLAGS): Likewise
> >      &= nbsp; =EF=BB=BF(drivemap_mod_LDFLAGS): Likewise
> >     =   =EF=BB=BF* include/grub/loader.h (grub_loader_register_preboot): Ne= w
> >        function prototype.
> >&= nbsp;       (grub_loader_unregister_preboot): Likewise.
&= gt; >        (grub_preboot_hookid): New typedef.
= > >        =EF=BB=BF* kern/loader.c =EF=BB=BF(gru= b_loader_register_preboot): New function.
> >      =   (grub_loader_unregister_preboot): Likewise.
> >   = ;     (preboot_hooks): New variable.
> >    &n= bsp;   (grub_loader_boot): Call the list of preboot-hooks before the> >        actual loader
>
> Pleas= e add a `.' after "New variable" and "Likewise", same for the
> third= and the last sentence.  Sometimes you did it right :-).
>
D= one. New CL entry will go at the end of this neverending post.

>
> >>= Some comments can be found below.  Can you please fix the code mentio= n
> >> in the review and similar code?  I really want the = code to be just
> >> like any other GRUB code.  Please und= erstand that we have to maintain
> >> this in the future. = If everyone would use his own codingstyle, GRUB
> >> would bec= ome unmaintainable.
> >>
> >> > Index: commands= /i386/pc/drivemap.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
> >> > --- commands/i386/pc/drivemap.c&nb= sp;   (revisi=C3=B3n: 0)
> >> > +++ commands/i386/= pc/drivemap.c    (revisi=C3=B3n: 0)
> >> > @@= -0,0 +1,417 @@
> >> > +/* drivemap.c - command to manage th= e BIOS drive mappings.  */
> >> > +/*
> >> > + *  GRUB  --  GRand Unified Boot= loader
> >> > + *  Copyright (C) 2008  Free Softwa= re Foundation, Inc.
> >> > + *
> >> > + *&nbs= p; GRUB is free software: you can redistribute it and/or modify
> >= ;> > + *  it under the terms of the GNU General Public License a= s published by
> >> > + *  the Free Software Foundation= , either version 3 of the License, or
> >> > + *  (at y= our option) any later version.
> >> > + *
> >> &= gt; + *  GRUB is distributed in the hope that it will be useful,
&g= t; >> > + *  but WITHOUT ANY WARRANTY; without even the impli= ed warranty of
> >> > + *  MERCHANTABILITY or FITNESS F= OR A PARTICULAR PURPOSE.  See the
> >> > + *  GNU = General Public License for more details.
> >> > + *
> >> > + *  You should have received a copy of the G= NU General Public License
> >> > + *  along with GRUB.&= nbsp; If not, see <http://www.gnu.org/licenses/>.
> >> > + */
>= ; >> > +
> >> > +#include <grub/normal.h>
= > >> > +#include <grub/dl.h>
> >> > +#incl= ude <grub/mm.h>
> >> > +#include <grub/misc.h>> >> > +#include <grub/disk.h>
> >> > +#= include <grub/loader.h>
> >> > +#include <grub/mach= ine/loader.h>
> >> > +#include <grub/machine/biosdisk.= h>
> >> > +
> >> > +#define MODNAME "drive= map"
> >> > +
> >> > +static const struct gru= b_arg_option options[] =3D {
> >> > +  {"list", 'l', 0, "show the current mappings", 0, 0},
> >> > +  {"res= et", 'r', 0, "reset all mappings to the default values", 0, 0},
> >= ;> > +  {0, 0, 0, 0, 0, 0}
> >> > +};
> >= ;> > +
> >> > +/* Syms/vars/funcs exported from drivem= ap_int13h.S - start.  */
> >> > +
> >> >= +/* Realmode far ptr =3D 2 * 16b */
> >> > +extern grub_uin= t32_t grub_drivemap_int13_oldhandler;
> >> > +/* Size of the= section to be copied */
> >> > +extern grub_uint16_t grub_d= rivemap_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_symbo= l_t grub_drivemap_int13_mapstart;
> >> > +
> >> = > +void grub_drivemap_int13_handler (void);
> >>
> &g= t;> 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 hea= der
> > file at once. In particular, I think the grub_symbol_t typ= edef should go
> > into the "standard" GRUB headers somehow (but n= ot in symbol.h, which is
> > included from assembly files).
>= ;
> Please do so.
>
> Other people might want to commen= t on the symbol change.  They will
> most likely miss it if we k= eep discussing it here ;-).  Can you please
> send that in as a separate change to give other the opportunity to
> react on = it?
Ok, so in a first stage I will put the discussed block in a drivemap= .h file in include/, then when that's committed start a discussion about mo= ving grub_symbol_t from drivemap.h to another header file.
>
>= >> > +/* Syms/vars/funcs exported from drivemap_int13h.S - end.&n= bsp; */
> >> > +
> >> > +
> >> &g= t; +static grub_preboot_hookid insthandler_hook;
> >> > +> >> > +typedef struct drivemap_node
> >> > +{<= BR>> >> > +  grub_uint8_t newdrive;
> >> > = +  grub_uint8_t redirto;
> >> > +  struct drivemap= _node *next;
> >> > +} drivemap_node_t;
> >> >= ; +
> >> > +static drivemap_node_t *drivemap =3D 0;
> = >>
> >> No need to set this to zero.
> > Yes, you said so already, but I wanted to make the initial state very
&= gt; > explicit to a future developer, since that variable is checked aga= inst
> > 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 variabl= e in which you pointed out this
> > issue, because it is not check= ed against zero anywhere.
>
> Please do so anyways.
> > >> > +static grub_err_t install_int13_handler (void);
&g= t; >> > +
> >> > +/* Puts the specified mapping int= o the table, replacing an existing mapping
> >> > +  fo= r newdrive or adding a new one if required.  */
> >> > = +static grub_err_t
> >> > +drivemap_set (grub_uint8_t newdri= ve, grub_uint8_t redirto)
> >> > +
> >>
> >> Please do not add a newline here.
> > Oops, sor= ry. I forgot to remove it when moving the comment
>
> :-)
&= gt;
> >> > +  drivemap_node_t *mapping =3D 0;
> = >> > +  drivemap_node_t *search =3D drivemap;
> >>= ; > +  while (search)
> >> > +    {
>= ; >> > +      if (search->newdrive =3D=3D newdri= ve)
> >> > +        {
> >> &= gt; +          mapping =3D search;
> >>= ; > +          break;
> >> > +&n= bsp;       }
> >> > +      sea= rch =3D search->next;
> >> > +    }
> >= ;> > +
> >> > + 
> >> > +  /= * Check for pre-existing mappings to modify before creating a new one. = ; */
> >> > +  if (mapping)
> >> > + = ;   mapping->redirto =3D redirto;
> >> > +  els= e
> >> > +    {
> >> > +  &nb= sp;   mapping =3D grub_malloc (sizeof (drivemap_node_t));
> >= > > +      if (!mapping)
> >> > + =       return grub_error (GRUB_ERR_OUT_OF_MEMORY,
> &g= t;> > +                 =         "cannot allocate map entry, not enough memory"= );
> >> > +      mapping->newdrive =3D new= drive;
> >> > +      mapping->redirto =3D = redirto;
> >> > +      mapping->next =3D d= rivemap;
> >> > +      drivemap =3D mapping;<= BR>> >> > +    }
> >> > +  return GRUB_ERR_NONE;
> >> > +}
> >> > +
> &g= t;> > +/* Removes the mapping for newdrive from the table.  If t= here is no mapping,
> >> > +  then this function behave= s like a no-op on the map.  */
> >> > +static void
&= gt; >> > +drivemap_remove (grub_uint8_t newdrive)
> >>= > +{
> >> > +  drivemap_node_t *mapping =3D 0;
&= gt; >> > +  drivemap_node_t *search =3D drivemap;
> >= ;> > +  drivemap_node_t *previous =3D 0;
> >> > +=   while (search)
> >> > +    {
> >&g= t; > +      if (search->newdrive =3D=3D newdrive)
&= gt; >> > +        {
> >> > +&nb= sp;         mapping =3D search;
> >> > +=           break;
> >> > +  &nb= sp;     }
> >> > +      previous =3D s= earch;
> >> > +      search =3D search->ne= xt;
> >> > +    }
> >> > +  if= (mapping) /* Found.  */
> >>
> >> You forgot= one.
> > Corrected. Sorry.
> >>
> >> >= ; +    {
> >> > +      if (previous= )
> >> > +        previous->next =3D = mapping->next;
> >> > +      else /* Entry= was head of list.  */
> >> > +      &nb= sp; drivemap =3D mapping->next;
> >> > +    &nb= sp; grub_free (mapping);
> >> > +    }
> >= ;> > +}
> >> > +
> >> > +/* Given a dev= ice 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 =3D=3D *name)
> >> &g= t; +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name em= pty");
> >> > +  /* Skip the first ( in (hd0) - disk_op= en wants just the name.  */
> >> > +  if (*name = =3D=3D '(')
> >> > +    name++;
> >> &= gt; + 
> >> > +  disk =3D grub_disk_open (name);<= BR>> >> > +  if (!disk)
> >> > +  &nbs= p; return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"", nam= e);
> >> > +  else
> >> > +    {
> >> > +      const enum grub_disk_dev_id = id =3D disk->dev->id;
> >> > +      /* = The following assignment is only sound if the device is indeed a
> &g= t;> > +        biosdisk.  The caller must ch= eck the return value.  */
> >> > +      = if (disknum)
> >> > +        *disknum = =3D disk->id;
> >> > +      grub_disk_clos= e (disk);
> >> > +      if (GRUB_DISK_DEVICE_= BIOSDISK_ID =3D=3D id)
> >> > +        r= eturn GRUB_ERR_NONE;
> >> > +      else retur= n grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk", name);
> = >> > +    }
> >> > +}
> >> &g= t; +
> >> > +/* Given a BIOS disk number, returns its GRUB device name if it exists.
> >> > +  For nonexisting BI= OS dnums, this function returns ERR_UNKNOWN_DEVICE.  */
> >&g= t;
> >> This is GRUB_ERR_UNKNOWN_DEVICE
> > I know, I= consciously left the GRUB_ part out because 1) it would
> > requi= re the line to be split and 2) that prefix is all over the place.
> &= gt; Corrected, however.
> >>
> >> > +static gru= b_err_t
> >> > +revparse_biosdisk(const grub_uint8_t dnum, c= onst char **output)
> >> > +{
> >> > +  = int found =3D 0;
> >> > +  auto int find (const char *n= ame);
> >> > +  int find (const char *name)
> >= ;> > +  {
> >> > +    const grub_disk_t= disk =3D grub_disk_open (name);
> >> > +    if (!= disk)
> >> > +      return 0;
> >> > +    else
> >> > +    &nb= sp; {
> >> > +        if (disk->id = =3D=3D dnum && GRUB_DISK_DEVICE_BIOSDISK_ID =3D=3D disk->dev->= ;id)
> >> > +          {
> &g= t;> > +            found =3D 1;
>= >> > +            if (output)
&g= t; >> > +              *output = =3D name;
> >> > +          }
&g= t; >> > +        grub_disk_close (disk);
&g= t; >> > +        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);
> >>
&= gt; >> Please change this.
> > Em... to what? What is the pr= oblem? Do you want me to reverse the
> > comparison? i.e. if (!fou= nd) { return error; } else { return ok; }
>
> The return is on= the same line as the else.
>

Corrected.

>
>= >> > +/* Given a GRUB-like device name and a convenient location,= stores the related
> >> > +  BIOS disk number.  A= ccepts devices like \((f|h)dN\), with 0 <=3D N < 128.  */
>= ; >> > +static grub_err_t
> >> > +tryparse_diskstri= ng (const char *str, grub_uint8_t *output)
> >> > +{
>= >> > +  if (!str || 0 =3D=3D *str)
> >> > +&n= bsp;   goto fail;
> >> > +  /* Skip opening paren in order to allow both (hd0) and hd0.  */
> >> > +&nbs= p; if (*str =3D=3D '(')
> >> > +    str++;
>= >> > +  if ((str[0] =3D=3D 'f' || str[0] =3D=3D 'h') &&a= mp; str[1] =3D=3D 'd')
> >> > +    {
> >&= gt; > +      grub_uint8_t bios_num =3D (str[0] =3D=3D 'h'= )? 0x80 : 0x00;
> >> > +      grub_errno =3D = GRUB_ERR_NONE;
> >> > +      unsigned long dr= ivenum =3D grub_strtoul (str + 2, 0, 0);
> >> > +  &nbs= p;   if (grub_errno !=3D GRUB_ERR_NONE || drivenum > 127)
> &= gt;> > +        /* N not a number or out of range= */
> >> > +        goto fail;
> &= gt;>
> >> Can you put this between braces, now comment was = added.
> > Done.
> >>
> >> > +  &= nbsp;   else
> >> > +        {
> >>= > +          bios_num |=3D drivenum;
> &= gt;> > +          if (output)
> >&g= t; > +            *output =3D bios_num;> >> > +          return GRUB_ERR_NON= E;
> >> > +        }
> >> &g= t; +    }
> >> > +  else goto fail;
> &= gt;>
> >> ...
> > What's the problem here? The lac= k of braces? The goto (as used in the
> > ext2 code)?
>
= > goto is on the same line as the else.

Corrected, though I find = this change less logic than the last one (splitting the else retun grub_err= or(...)) line because this generates two _extremely_ short lines;
> <= BR>> >> > +fail:
> >> > +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" invalid: must"> >> > +               = ;     "be (f|h)dN, with 0 <=3D N < 128", str);
> >= > > +}
> >> > +
> >> > +static grub_err= _t
> >> > +grub_cmd_drivemap (struct grub_arg_list *state, i= nt argc, char **args)
> >> > +{
> >> > + = ; if (state[0].set)
> >> > +    {
> >>= > +      /* Show: list mappings.  */
> >&g= t; > +      if (!drivemap)
> >> > +  =       grub_printf ("No drives have been remapped");
> = >> > +      else
> >> > +  &nbs= p;     {
> >> > +         = ; grub_printf ("Showing only remapped drives.\n");
> >> > +          grub_printf ("Mapped\tGRUB\n");> >> > +          drivemap_node_t *c= urnode =3D drivemap;
> >> > +        &nb= sp; while (curnode)
> >> > +        &nbs= p;   {
> >> > +           = ;   const char *dname =3D 0;
> >> > +    &nbs= p;         grub_err_t err =3D revparse_biosdisk (curnod= e->redirto, &dname);
> >> > +      &nb= sp;       if (err !=3D GRUB_ERR_NONE)
> >> > = +                return grub_error = (err, "invalid mapping: non-existent disk"
> >> > +  &n= bsp;                     =                 "or not managed by the BIOS");
> >> > +          &nbs= p;   grub_printf("0x%02x\t%4s\n", curnode->newdrive, dname);
>= ; >> > +              curnode = =3D curnode->next;
> >> > +        &n= bsp;   }
> >> > +        }
> = >> > +    }
> >> > +  else if (state= [1].set)
> >> > +    {
> >> > +&nbs= p;     /* Reset: just delete all mappings, freeing their memory.&= nbsp; */
> >> > +      drivemap_node_t *curno= de =3D drivemap;
> >> > +      drivemap_node_= t *prevnode =3D 0;
> >> > +      while (curno= de)
> >> > +        {
> >> &= gt; +          prevnode =3D curnode;
> >> > +          curnode =3D curnode->= next;
> >> > +          grub_free (= prevnode);
> >> > +        }
> >= ;> > +      drivemap =3D 0;
> >> > +&nb= sp;   }
> >> > +  else
> >> > +&nbs= p;   {
> >> > +      /* Neither flag: pu= t mapping */
> >>
> >> ".  */
> > Do= ne
> >>
> >> > +      grub_uint8= _t mapfrom =3D 0;
> >> > +      grub_uint8_t = mapto =3D 0xFF;
> >> > +      grub_err_t err;=
> >> > +     
> >> > + =     if (argc !=3D 2)
> >> > +     =   return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
> >> > +
> >> > +    &= nbsp; err =3D parse_biosdisk (args[0], &mapfrom);
> >> >= +      if (err !=3D GRUB_ERR_NONE)
> >> > +&= nbsp;       return err;
> >> > +
> >= > > +      err =3D tryparse_diskstring (args[1], &= mapto);
> >> > +      if (err !=3D GRUB_ERR_N= ONE) /* Not a disk string. Maybe a raw num then?  */
> >> =
> >> Please move this up.
> > Done.
> >> =
> >> > +        {   
>= >> > +          grub_errno =3D GRUB_ERR_= NONE;
> >> > +          unsigned lo= ng num =3D grub_strtoul (args[1], 0, 0);
> >> > +  &nbs= p;       if (grub_errno !=3D GRUB_ERR_NONE || num > 0xFF)=   /* Not a raw num or too high.  */
> >> > +   = ;         return grub_error (grub_errno,
> >&g= t; > +                  &nb= sp;           "Target specifier must be of the for= m (fdN) or "
> >> > +          &nbs= p;                   "(hdN), w= ith 0 <=3D N < 128; or a plain dec/hex "
> >> > + = ;                     &nb= sp;       "number between 0 and 255");
> >> >= +          else mapto =3D (grub_uint8_t)num;
&= gt; >> > +        }
> >> > +&nb= sp;    
> >> > +      if (mapto = =3D=3D mapfrom)  /* Reset to default.  */
> >>
&g= t; >> Same here.
> > Done.
> >>
> >> = > +        {
> >> > +    &n= bsp;     grub_dprintf (MODNAME, "Removing the mapping for %s (%02= x)", args[0], mapfrom);
> >> > +        =   drivemap_remove (mapfrom);
> >> > +    &nbs= p;   }
> >> > +      else  /* Map.&= nbsp; */
> >>
> >> Please move the comment inside = the braces below.
> > Done, and reworded.
> >>
>= ; >> > +        {
> >> > + = ;         grub_dprintf (MODNAME, "Mapping %s (%02x) to = %02x\n", args[0], mapfrom, mapto);
> >> > +    &nb= sp;     return drivemap_set ((grub_uint8_t)mapto, mapfrom);
&g= t; >> > +        }
> >> > +    }
> >> > +
> >> > + = ; return GRUB_ERR_NONE;
> >> > +}
> >> > +> >> > +typedef struct __attribute__ ((packed)) int13map_node<= BR>> >> > +{
> >> > +  grub_uint8_t disknum= ;
> >> > +  grub_uint8_t mapto;
> >> > += } int13map_node_t;
> >> > +
> >> > +/* The mi= n amount of mem that must remain free after installing the handler.
>= >> > +  32 KiB is just above 0x7C00-0x7E00, where the bootse= ctor is loaded.  */
> >> > +#define MIN_FREE_MEM_KB 32<= BR>> >> > +#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 ) )
> >> > +
> >> > +/* Int= 13h handler installer - reserves conventional memory for the handler,
&g= t; >> > +  copies it over and sets the IVT entry for int13h.&= nbsp;
> >> > +  This code rests on the assumption that= GRUB does not activate any kind of
> >> > +  memory ma= pping apart from identity paging, since it accesses realmode
> >&g= t; > +  structures by their absolute addresses, like the IVT at 0 o= r the BDA at
> >> > +  0x400; and transforms a pmode po= inter into a rmode seg:off far ptr.  */
> >> > +static = grub_err_t
> >> > +install_int13_handler (void)
> >= > > +{
> >> > +  grub_size_t entries =3D 0;
&g= t; >> > +  drivemap_node_t *curentry =3D drivemap;
> &g= t;> > +  while (curentry)  /* Count entries to prepare a contiguous map block.  */
> >>
> >> ...
&= gt; > Comment moved up.
> >>
> >> > +  =   {
> >> > +      entries++;
> >= ;> > +      curentry =3D curentry->next;
> &g= t;> > +    }
> >> > +  if (0 =3D=3D ent= ries)
> >>
> >> I know this is what you prefer, bu= t can you change this nevertheless?
> > I refer to my objection ne= ar the top of the post.
>
> I know you object, but did you cha= nge it?
Done.
>
> >> > +      grub_= dprintf (MODNAME, "No drives marked as remapped, installation of"
> &= gt;> > +                 = ; "an int13h handler is not required.");
> >> > +  &nbs= p;   return GRUB_ERR_NONE;  /* No need to install the int13h handler.  */
> >> > +    }
> >> = > +  else
> >> > +    {
> >> &= gt; +      grub_dprintf (MODNAME, "Installing int13h handler= ...\n");
> >> > +      grub_uint32_t *ivtslot= =3D (grub_uint32_t*)0x0000004c;
> >> > +     = ;
> >> > +      /* Save the pointer to the o= ld int13h handler.  */
> >> > +      gru= b_drivemap_int13_oldhandler =3D *ivtslot;
> >> > +  &nb= sp;   grub_dprintf (MODNAME, "Old int13 handler at %04x:%04x\n",
&g= t; >> > +                &= nbsp; (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff,
> &= gt;> > +                 = ; grub_drivemap_int13_oldhandler & 0x0ffff);
> >> > +
> >> > +      /* Reserve a section of conv= entional memory as "BIOS memory" for handler:
> >> > + =       BDA offset 0x13 contains the top of such memory. = ; */
> >> > +      grub_uint16_t *bpa_freekb = =3D (grub_uint16_t*)0x00000413;
> >> > +     = grub_dprintf (MODNAME, "Top of conventional memory: %u KiB\n", *bpa_freekb= );
> >> > +      grub_size_t total_size =3D g= rub_drivemap_int13_size
> >> > +        =                     + (en= tries + 1) * sizeof(int13map_node_t);
> >> > +    =   grub_uint16_t payload_sizekb =3D (total_size >> 10) +
> = >> > +                &nbs= p;                   (((total_size % 1024) =3D=3D 0) ? 0 : 1);
> >> > +  &n= bsp;   if ((*bpa_freekb - payload_sizekb) < MIN_FREE_MEM_KB)
>= ; >> > +        return grub_error (GRUB_ERR_OU= T_OF_MEMORY, "refusing to install"
> >> > +    &nb= sp;                     "= int13 handler, not enough free memory after");
> >> > + = ;     grub_dprintf (MODNAME, "Payload is %u b long, reserving %u = Kb\n",
> >> > +         &= nbsp;          total_size, payload_sizek= b);
> >> > +      *bpa_freekb -=3D payload_si= zekb;
> >> > +
> >> > +      /= * Copy int13h handler chunk to reserved area.  */
> >> >= ; +      grub_uint8_t *handler_base =3D (grub_uint8_t*)(*bpa_freekb << 10);
> >> > +  &n= bsp;   grub_dprintf (MODNAME, "Copying int13 handler to: %p\n", handle= r_base);
> >> > +      grub_memcpy (handler_b= ase, &grub_drivemap_int13_handler_base,
> >> > +  &= nbsp;               grub_drivemap_int13_= size);
> >> > +
> >> > +      = /* Copy the mappings to the reserved area.  */
> >> > +=       curentry =3D drivemap;
> >> > +  &= nbsp;   grub_size_t i;
> >> > +      int= 13map_node_t *handler_map =3D (int13map_node_t*)
> >> > +&nb= sp;                     I= NT13H_TONEWADDR (&grub_drivemap_int13_mapstart);
> >> > = +      grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n", handler_map);
> >> > +      = for (i =3D 0; i < entries && curentry; i++, curentry =3D curentr= y->next)
> >> > +        {
> &g= t;> > +          handler_map[i].disknum =3D = curentry->newdrive;
> >> > +        &= nbsp; handler_map[i].mapto =3D curentry->redirto;
> >> > = +          grub_dprintf (MODNAME, "\t#%d: 0x%02x &= lt;- 0x%02x\n", i,
> >> > +      &n= bsp;               &= nbsp; handler_map[i].disknum, handler_map[i].mapto);
> >> > = +        }
> >> > +      = /* Signal end-of-map.  */
> >> > +      = handler_map[i].disknum =3D 0;
> >> > +      handler_map[i].mapto =3D 0;
> >> > +      gr= ub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x (end)\n", i,
> >&= gt; > +             &n= bsp;      handler_map[i].disknum, handler_map[i].mapto)= ;
> >> > +
> >> > +      /* In= stall our function as the int13h handler in the IVT.  */
> >&= gt; > +      grub_uint32_t ivtentry =3D ((grub_uint32_t)h= andler_base) << 12; /* Segment address.  */
> >> >= ; +      ivtentry |=3D (grub_uint16_t) INT13H_OFFSET(grub_dr= ivemap_int13_handler);
> >> > +      grub_dpr= intf (MODNAME, "New int13 handler IVT pointer: %04x:%04x\n",
> >&g= t; > +                  (iv= tentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);
> >> > +      *ivtslot =3D ivtentry;
> >>= ; > +     
> >> > +      r= eturn GRUB_ERR_NONE;
> >> > +    }
> >>= ; > +}
> >> > +
> >> > +GRUB_MOD_INIT (dri= vemap)
> >> > +{
> >> > +  (void) mod;&n= bsp;           /* Stop warning.&nbs= p; */
> >> > +  grub_register_command (MODNAME, grub_cm= d_drivemap,
> >> > +           = ;             GRUB_COMMAND_FLAG_BOTH,
>= >> > +            &n= bsp;                 MODNAME " -s |= -r | (hdX) newdrivenum",
> >> > +       = ;                 "Manage the BIOS drive mappings", options);
> >> > +  insthandler_= hook =3D grub_loader_register_preboot (&install_int13_handler, 1);
&= gt; >> > +}
> >> > +
> >> > +GRUB_MO= D_FINI (drivemap)
> >> > +{
> >> > +  gr= ub_loader_unregister_preboot (insthandler_hook);
> >> > +&nb= sp; insthandler_hook =3D 0;
> >> > +  grub_unregister_c= ommand (MODNAME);
> >> > +}
> >> > +
> = >> > Index: commands/i386/pc/drivemap_int13h.S
> >> &g= t; =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
> >> >= ; --- commands/i386/pc/drivemap_int13h.S    (revisi=C3=B3n: = 0)
> >> > +++ commands/i386/pc/drivemap_int13h.S  =   (revisi=C3=B3n: 0)
> >> > @@ -0,0 +1,118 @@
> &= gt;> > +/*
> >> > + *  GRUB  --  GRand Unified Bootloader
> >> > + *  Copyrigh= t (C) 1999,2000,2001,2002,2003,2005,2006,2007,2008 Free Software Foundation= , Inc.
> >> > + *
> >> > + *  GRUB is fr= ee software: you can redistribute it and/or modify
> >> > + = *  it under the terms of the GNU General Public License as published b= y
> >> > + *  the Free Software Foundation, either vers= ion 3 of the License, or
> >> > + *  (at your option) a= ny later version.
> >> > + *
> >> > + * = GRUB is distributed in the hope that it will be useful,
> >> &= gt; + *  but WITHOUT ANY WARRANTY; without even the implied warranty o= f
> >> > + *  MERCHANTABILITY or FITNESS FOR A PARTICUL= AR PURPOSE.  See the
> >> > + *  GNU General Publi= c License for more details.
> >> > + *
> >> > + *  You should have received a copy of the GNU General= Public License
> >> > + *  along with GRUB.  If n= ot, see <http:/= /www.gnu.org/licenses/>.
> >> > + */
> >>= > +
> >> > +
> >> > +/*
> >> = > + * Note: These functions defined in this file may be called from C.> >> > + *      Be careful of that you must no= t modify some registers. Quote
> >> > + *     = ; from gcc-2.95.2/gcc/config/i386/i386.h:
> >> > +
> &= gt;> > +  1 for registers not available across function calls.> >> > +  These must include the FIXED_REGISTERS and als= o any
> >> > +  registers that can be used without bein= g 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 wi= th the options -mrtd and -mregparm=3D3.
> >> > + *  &nb= sp;   So the first three arguments are passed in %eax, %edx, and %ecx,=
> >> > + *      respectively, and if a funct= ion 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>
&= gt; >> > +
> >> > +#define GRUB_DRIVEMAP_INT13H_OFF= SET(x) ((x) - grub_drivemap_int13_handler_base)
> >> > +
= > >> > +/* Copy starts here. When deployed, this label must be = segment-aligned */
> >> > +VARIABLE(grub_drivemap_int13_hand= ler_base)
> >> > +
> >> > +VARIABLE(grub_driv= emap_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 chan= ges to the registers as possible.  IP-relative
> >> > +  addressing like on amd64 would make life way easier here. */<= BR>> >> > +.code16
> >> > +FUNCTION(grub_drivema= p_int13_handler)
> >> > +  push %bp
> >> &g= t; +  mov %sp, %bp
> >> > +  push %ax  /* We'= ll need it later to determine the used BIOS function */
> >> &g= t; +
> >> > +  /* Map the drive number (always in DL?) = */
> >> > +  push %ax
> >> > +  pus= h %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 a= s-is */
> >> > +  cmp %dl, %al
> >> > +  jz 2f /* Found - drive remapped, modify DL */
> >> >= ; +  add $2, %si
> >> > +  jmp 1b /* Not found, bu= t more remaining, loop  */
> >> > +2:mov %ah, %dl
&g= t; >> > +3:pop %si
> >> > +  pop %bx
> &= gt;> > +  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 sens= ible?) */
> >> > +  lcall *%cs:GRUB_DRIVEMAP_INT13H_OFF= SET(grub_drivemap_int13_oldhandler)
> >> > +  pop %bp /* The pushed flags were removed by iret */
> >> > = +  /* Set the saved flags to what the int13h handler returned */
&g= t; >> > +  push %ax
> >> > +  pushf
&g= t; >> > +  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 =3D 0x08, but many BIOS
> >= > > +    refs say retDL =3D # of drives connected.  Ho= wever, the GRUB Legacy code
> >> > +    treats thi= s 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 */
> >> > +#&= nbsp; cmp %ah, %al  /* DRV=3DDST =3D> drive not remapped */
>= >> > +#  je 4f
> >> > +#  mov %ah, %dl&n= bsp; /* Undo remap */
> >> > +
> >> > +4:mov = %bp, %sp
> >> > +  pop %bp
> >> > + = ; iret
> >> > +/* This label MUST be at the end of the copie= d block, since the installer code
> >> > +  reserves ad= ditional space for mappings at runtime and copies them over it */
> &= gt;> > +.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
> >> > =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
> >> > --- = conf/i386-pc.rmk    (revisi=C3=B3n: 1766)
> >> &= gt; +++ conf/i386-pc.rmk    (copia de trabajo)
> >&= gt; > @@ -158,7 +158,7 @@
> >> >     = vbe.mod vbetest.mod vbeinfo.mod video.mod gfxterm.mod \
> >> &= gt;      videotest.mod play.mod bitmap.mod tga.mod cpui= d.mod serial.mod    \
> >> >   &nbs= p;  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 =3D dis= k/i386/pc/biosdisk.c
> >> > @@ -325,4 +325,11 @@
> >= ;> >  bsd_mod_CFLAGS =3D $(COMMON_CFLAGS)
> >> >&= nbsp; bsd_mod_LDFLAGS =3D $(COMMON_LDFLAGS)
> >> >  > >> > +# For drivemap.mod.
> >> > +drivemap_mo= d_SOURCES =3D commands/i386/pc/drivemap.c \
> >> > +  &= nbsp;                   comman= ds/i386/pc/drivemap_int13h.S
> >> > +drivemap_mod_ASFLAGS = =3D $(COMMON_ASFLAGS)
> >> > +drivemap_mod_CFLAGS =3D $(COMM= ON_CFLAGS)
> >> > +drivemap_mod_LDFLAGS =3D $(COMMON_LDFLAGS= )
> >> > +
> >> >  include $(srcdir)/con= f/common.mk
> >> > Index: include/grub/loader.h
> >= > > =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
> >> >= ; --- include/grub/loader.h    (revisi=C3=B3n: 1766)
>= >> > +++ include/grub/loader.h    (copia de trabaj= o)
> >> > @@ -37,6 +37,22 @@
> >> >  /* = Unset current loader, if any.  */
> >> >  void EXP= ORT_FUNC(grub_loader_unset) (void);
> >> > 
> &g= t;> > +typedef struct hooklist_node *grub_preboot_hookid;
> >= ;> > +
> >> > +/* Register a function to be called bef= ore the boot hook.  Returns an id that
> >> > +  c= an be later used to unregister the preboot (i.e. on module unload).  I= f
> >> > +  abort_on_error is set, the boot sequence wi= ll abort if any of the registered
> >> > +  functions r= eturn anything else than GRUB_ERR_NONE.
> >> > +  On error, the return value will compare equal to 0 and the er= ror information
> >> > +  will be available in errno an= d errmsg.  However, if the call is successful
> >> > +&= nbsp; 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 ment= ioned, 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), i= nt abort_on_error);
> >> > +
> >> > +/* Unregister a preboot hook by the id returned by loader_register_preboo= t.
> >> > +  This functions becomes a no-op if no such = function is registered */
> >> > +void EXPORT_FUNC(grub_load= er_unregister_preboot) (grub_preboot_hookid id);
> >> > +> >> >  /* Call the boot hook in current loader. This may= or may not return,
> >> >    depending on the set= ting by grub_loader_set.  */
> >>
> >> Nitpic= k: "loader.  This..."
> > Are you a bot? =C2=AC=C2=AC Correct= ed
>
> It would make like much simpler if I were ;-).  Wh= at makes you think
> so?
Your ability to spot these kind of smalli= sh things, like the "deltion" word and the lack of a double space. You even= write normal text with double spaces!

>
> >> >&n= bsp; grub_err_t EXPORT_FUNC(grub_loader_boot) (void);
> >> > 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
> >> > --- kern/loader.c  &n= bsp; (revisi=C3=B3n: 1766)
> >> > +++ kern/loader.c &nb= sp;  (copia de trabajo)
> >> > @@ -61,11 +61,82 @@
&= gt; >> >    grub_loader_loaded =3D 0;
> >> &= gt;  }
> >> > 
> >> > +struct hook= list_node
> >> > +{
> >> > +  grub_err_t= (*hook) (void);
> >> > +  int abort_on_error;
> = >> > +  struct hooklist_node *next;
> >> > +};=
> >> > +
> >> > +static struct hooklist_node= *preboot_hooks =3D 0;
> >> > +
> >> > +grub_= preboot_hookid
> >> > +grub_loader_register_preboot(grub_err= _t (*hook) (void), int abort_on_error)
> >> > +{
> >> > +  if (!hook)
> >> > +  =   {
> >> > +      grub_error (GRUB_ERR_B= AD_ARGUMENT, "preboot hook must not be NULL");
> >> > + = ;     return 0;
> >> > +    }
> &g= t;> > +  grub_preboot_hookid newentry =3D grub_malloc (sizeof (s= truct hooklist_node));
> >>
> >> Mixed declaration= s/code.
> > Oops, sorry. I put most of my attention on drivemap.c = (and even then
> > many comments slipped through). Corrected.
&= gt;
> Please re-check them, I might have missed things this time...<= BR>>
> >> > +  if (!newentry)
> >> >= +    {
> >> > +      grub_error (G= RUB_ERR_OUT_OF_MEMORY, "cannot alloc a hookinfo structure");
> >&g= t; > +      return 0;
> >> > +    }
> >> > +  else
> >> > +  =   {
> >> > +      newentry->hook =3D = hook;
> >> > +      newentry->abort_on_err= or =3D abort_on_error;
> >> > +      newentry= ->next =3D preboot_hooks;
> >> > +      pr= eboot_hooks =3D newentry;
> >> > +      retur= n newentry;
> >> > +    }
> >> > +}=
> >> > +
> >> > +void
> >> > = +grub_loader_unregister_preboot(grub_preboot_hookid id)
> >> > >> "preboot (grub"
> > Corrected on both functions ;)=
> >>
> >> > +{
> >> > +  g= rub_preboot_hookid entry =3D 0;
> >> > +  grub_preboot_= hookid search =3D preboot_hooks;
> >> > +  grub_preboot_hookid previous =3D 0;
> >> > +
> >&g= t; > +  if (0 =3D=3D id)
> >> > +    retur= n;
> >>
> >> ...
> > ... xD
> >&= gt;
> >> > +  while (search)
> >> > +&n= bsp;   {
> >> > +      if (search =3D=3D= id)
> >> > +        {
> >> = > +          entry =3D search;
> >>= > +          break;
> >> > +&nb= sp;       }
> >> > +      prev= ious =3D search;
> >> > +      search =3D sea= rch->next;
> >> > +    }
> >> > = +  if (entry) /* Found */
> >>
> >> ...
&g= t; > Comment removed, was unnecessary.
> >>
> >>= ; > +    {
> >> > +      if (previous)=
> >> > +        previous->next =3D e= ntry->next;
> >> > +      else preboot_hoo= ks =3D entry->next; /* Entry was head of list */
> >> > +=       grub_free (entry);
> >> > +   = ; }
> >> > +}
> >> > +
> >> >&= nbsp; grub_err_t
> >> >  grub_loader_boot (void)
>= ; >> >  {
> >> >    if (! grub_loade= r_loaded)
> >> >      return grub_error (GRUB= _ERR_NO_KERNEL, "no loaded kernel");
> >> > + 
>= >> > +  grub_preboot_hookid entry =3D preboot_hooks;
>= >>
> >> Mixed declarations/code.
> > Moved the= whole line up.
> >>
> >> > +  while (entry)
> >> > +    {
> >> > +&nbs= p;     grub_err_t possible_error =3D entry->hook();
> &g= t;> > +      if (possible_error !=3D GRUB_ERR_NONE &am= p;& entry->abort_on_error)
> >> > +    &nbs= p;   return possible_error;
> >> > +     = ; entry =3D entry->next;
> >> > +    }
> = >> > 
> >> >    if (grub_loader_nor= eturn)
> >> >      grub_machine_fini ();
&= gt;
>
>
> ____________________________________________= ___
> Grub-devel mailing list
> Grub-devel@gnu.org
>= http://lists.gnu.org/mailman/listinfo/grub-devel

El mar, 05-0= 8-2008 a las 13:28 +0200, Marco Gerards escribi=C3=B3:
> Hi,
>
&g= t; Javier Mart=C3=ADn <lordhabbit@gmail.com> writes:
&g= t;
> >> > There is, however, one point in which I keep my o= bjection: comparisons
> >> > between a variable and a consta= nt should be of the form CONSTANT =3D=3D
> >> > variable and= not in the reverse order, since an erroneous but quite
> >> &g= t; possible change of =3D=3D for =3D results in a compile-time error instea= d of a
> >> > _extremely_ difficult to trace runtime bug. Su= ch kind of bugs are quite
> >> > excruciating to find in use= rspace applications within an IDE, so I can't
> >> > even co= nsider the pain to debug them in a bootloader.
> >>
> &g= t;> I understand your concern, nevertheless, can you please change it?> > You understand my concern, but seemingly do not understand that in order
&g= t; > 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) wit= h a simple
> > deltion! (point: did you notice that last word _wit= hout_ a spelling
> > checker? Now try to do so in a multithousand-= line program).
>
> BTW, your patch still contains this, can yo= u please change it before I
> go over it again?
>
> I kn= ow people who claim that this code will become buggy because we
> wri= te it in C.  Should we start accepting patches to rewrite GRUB in
&= gt; Haskell or whatever? :-)
What about Ada? The stock GCC has Ada suppo= rt ^^
>
> Really, as a maintainer I should set some standards = and stick to it.
> Of course not everyone will like me and sometimes = I have to act like a
> jerk.  But I rather be a jerk, than committing code that do not meet
> my expectations.  But p= lease understand, this contribution is highly
> appreciated.  Ho= wever, we want to have something maintainable for the
> far future as= well :-)
I understand these kind of concerns, particularly seeing how G= RUB Legacy
ended - tangled, unscalable spaghetti code. You're not a jerk= , just a
bit obsessive, but that's fine when trying to handle herds of u= s
all-important devs which think all we do is The Right Thing (tm) andothers are heretics to The Truth.
>
> --
> Marco
&g= t;
>
>
> _____________________________________________= __
> Grub-devel mailing list
> Grub-devel@gnu.org
> = http://lists.gnu.org/mailman/listinfo/grub-devel

Ok, so here i= s the new version of the patch, with a new drivemap.h
header and the =3D= =3D arguments put in the Holy Ordering ^^. I hope I didn't
forget anythi= ng this time. Here is the new CL entry:

=EF=BB=BF=EF=BB=BF2008-08-XX=   Javier Martin  <lordhabbit@gmail.com>

&nb= sp;       =EF=BB=BF* commands/i386/pc/drivemap.c: New file.<= BR>        * commands/i386/pc/drivemap_int13h.S: New fi= le.
        =EF=BB=BF* conf/i386-pc.rmk (pkglib_MODU= LES): Added drivemap.mod.
        (drivemap_mod_SOUR= CES): New variable.
        (drivemap_mod_ASFLAGS): = Likewise.
        =EF=BB=BF(drivemap_mod_CFLAGS): Li= kewise.
        =EF=BB=BF(drivemap_mod_LDFLAGS): Lik= ewise.
        =EF=BB=BF* include/grub/loader.h (gru= b_loader_register_preboot): New
        function prototype.
        (g= rub_loader_unregister_preboot): Likewise.
        (g= rub_preboot_hookid): New typedef.
        =EF=BB=BF*= kern/loader.c =EF=BB=BF(grub_loader_register_preboot): New function.
&n= bsp;       (grub_loader_unregister_preboot): Likewise.
&n= bsp;       (preboot_hooks): New variable.
    &= nbsp;   (grub_loader_boot): Call the list of preboot-hooks before the<= BR>        actual loader.

By the way, is there a= nything I can do to make `svn up' updates less
traumatic? I don't want t= o search each "C" file for "<<<<<< mine" lines
and cor= rect them: is there any tool to do this with a workflow not
unlike that = of Gentoo's `etc-update'?

Habbit

=0A=0A=0A =
Connect with friends all over the world. Get Yahoo! India Messenger. --0-1133575639-1218033794=:55348--