Re: status grub2 port of grub-legasy map command

[Pavel Roskin <proski@gnu.org>]

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