From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1M0pIu-0003Mp-2p for mharc-grub-devel@gnu.org; Sun, 03 May 2009 23:57:20 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M0pIs-0003L8-L6 for grub-devel@gnu.org; Sun, 03 May 2009 23:57:18 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M0pIo-0003Ja-Hl for grub-devel@gnu.org; Sun, 03 May 2009 23:57:18 -0400 Received: from [199.232.76.173] (port=53013 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M0pIo-0003JN-CV for grub-devel@gnu.org; Sun, 03 May 2009 23:57:14 -0400 Received: from c60.cesmail.net ([216.154.195.49]:28708) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.60) (envelope-from ) id 1M0pIn-0007ej-S7 for grub-devel@gnu.org; Sun, 03 May 2009 23:57:14 -0400 Received: from unknown (HELO smtprelay2.cesmail.net) ([192.168.1.112]) by c60.cesmail.net with ESMTP; 03 May 2009 23:57:12 -0400 Received: from [192.168.0.22] (static-72-92-88-10.phlapa.fios.verizon.net [72.92.88.10]) by smtprelay2.cesmail.net (Postfix) with ESMTPSA id EBEEB34C6D for ; Sun, 3 May 2009 23:55:56 -0400 (EDT) From: Pavel Roskin To: The development of GRUB 2 In-Reply-To: <1241393832.6586.35.camel@accesodirecto.casa> References: <49E3E0FB.1070907@verizon.net> <49E5A102.40701@verizon.net> <49E5A394.8050108@gmail.com> <49E5A91C.3000109@verizon.net> <49E5AA2F.1060305@gmail.com> <1240003243.6449.7.camel@localhost> <49E90CCD.90207@verizon.net> <49E91862.9090103@verizon.net> <1240021094.19677.29.camel@accesodirecto.casa> <1241308934.4290.79.camel@accesodirecto.casa> <1241384352.2442.133.camel@ct> <1241393832.6586.35.camel@accesodirecto.casa> Content-Type: text/plain; charset="ISO-8859-1" Date: Sun, 03 May 2009 23:57:11 -0400 Message-Id: <1241409431.26483.33.camel@mj> Mime-Version: 1.0 X-Mailer: Evolution 2.26.1 (2.26.1-2.fc11) Content-Transfer-Encoding: 8bit X-detected-operating-system: by monty-python.gnu.org: Genre and OS details not recognized. Subject: Re: status grub2 port of grub-legasy map command X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 May 2009 03:57:19 -0000 On Mon, 2009-05-04 at 01:37 +0200, Javier Martín wrote: > > The patch adds many trailing spaces. I suggest that you run GNU indent > > on drivemap.c. It will take care of most of the trailing spaces. > > Comments will still need to be fixed. > > > > Assembler files use different formatting in GRUB. Also, it's better to > > use meaningful names for the labels. Label 4 is unused. > Where can I find those assembly formatting conventions? From what I see > in your version of the patch, the gist is that asm instructions start at > the 8th column (not after a \t). This carries an unpleasant > FORTRAN-esque smell that I would rather avoid. I don't know if it's documented anywhere, but it's sufficient to looks at other *.S files in GRUB or another GNU project, such as GCC. > > Some comments are excessive or unnecessary. "These functions defined in > > this file may be called from C" - irrelevant, we have no such functions. > > Complaints that the processor is not in 64-bit mode are also useless. I > > don't understand what "bundle" means in the comments. > Sorry, I copied the initial comment from another .S file in GRUB2 as a > kind-of-boilerplate. In this context, "bundle" means the whole "int13 > handler" package that is deployed to the reserved memory address, > consisting of the old IVT pointer, the actual int13h handler code and > the entry map. It would be great if you change the comments to avoid the word "bundle" unless it's explained. > > What is "(void) mod;"? It doesn't prevent any warnings for me. > Once again, boilerplate code copied from hello.c long ago. I suspect > this stopped being required when the command framework was revamped. Correct. We are using gcc attributes to suppress warnings in GRUB_MOD_INIT. I've committed a patch that removes all that stuff. > > grub_symbol_t is already used in kern/dl.c with a different meaning. > > Why not just use void? > The reason to avoid using a plain "void" is that it might be a slightly > confusing sight, so a future coder might have an idea-of-the-moment and > change it to a "meaningful type" like void*. The presence of an explicit > type with a big comment is supposed to at least make people think twice > before changing it. You can leave the comment but use void. I don't think anyone (of the reasonable people allowed to touch GRUB code) would change the type if the code is working. > > Improved patch is attached. > Thanks. I will read it thoroughly tomorrow when I'm back from uni. I > think that drivemap.h could be scrapped, its contents incorporated into > drivemap.c so as to reduce clutter and bitrot potential, and would > reduce the impact of the declaration of grub_symbol_t even more. That's a good idea. Still, I would prefer that we don't use grub_symbol_t where void is be sufficient. -- Regards, Pavel Roskin