grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: status grub2 port of grub-legasy map command


From: Javier Martín
Subject: Re: status grub2 port of grub-legasy map command
Date: Mon, 04 May 2009 01:37:12 +0200

El dom, 03-05-2009 a las 16:59 -0400, Pavel Roskin escribió:
> On Sun, 2009-05-03 at 02:02 +0200, Javier Martín wrote:
> > I am glad to inform that, with the new version of the mmap patch,
> > drivemap now boots all my hd1 installs of:
> > - Windows XP (Pro x64)
> > - ReactOS
> > - FreeDOS
> 
> I confirm that 32-bit Windows XP is working too.
> 
> > I would suggest, however, that the return type of
> > grub_mmap_malign_and_register be changed to void* from char*, just like
> > the return type from malloc, because it's the meaningful data type to
> > indicate a pointer to generic memory _and_ it automatically casts to any
> > pointer type the caller uses (which is the reason it's used in malloc).
> 
> Done.
> 
> A few comments regarding the code.
> 
> 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.

> 
> 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.

> 
> Why do we have duplication between grub_drivemap_int13_mapstart and
> grub_drivemap_int13_size?
It was initially added so that install_int13_handler would make no
assumptions about the structure of the bundle it was copying. However,
given that it already assumes that the map space is the last thing in
the bundle, your change is fine.

> 
> 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.

> 
> 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.

> 
> Please use "void" in the argument list if the function takes no
> arguments, as in uninstall_int13_handler().
> 
> "two arguments required" may be misleading.  In some cases, only one
> argument is required, such as "-l".  Let's make drivemap without
> arguments show the map.
Good idea.

> 
> 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.

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

Attachment: signature.asc
Description: Esto es una parte de mensaje firmado digitalmente


reply via email to

[Prev in Thread] Current Thread [Next in Thread]