Re: [PATCH] Drivemap module

[Pavel Roskin <proski@gnu.org>]

On Tue, 2008-06-10 at 23:31 +0200, Javier Martín wrote:

> Ok, I'm sorry and don't mean to be intrusive, I just thought the last
> messages might have got lost between mail filters - it's happened to me.

Here's my very superficial review.

Please don't add any trailing whitespace.  Line in the patch that start
with a plus should not end with a space or a tab.

Please avoid camelCase names, such as bpaMemInKb and retVal.  Local
variables should generally be short, like "ret" and "bpa_mem".

Some strings are excessively long.  While there may be exception of the
80 column limit, I see a 118 character long line that can be trivially
wrapped.

The patch add a new warning:

commands/i386/pc/drivemap_int13h.S: Assembler messages:
commands/i386/pc/drivemap_int13h.S:124: Warning: .space repeat count is
zero, ignored

I'm not sure what you meant there.

I don't think using #undef is a good idea.  It's better to use macro
names that would never be reused accidentally and thus never need to be
undefined.

-- 
Regards,
Pavel Roskin