Re: [PATCH] Drivemap module

[Javier Martín]

Hi there, I checked your comment, reread the GNU coding guidelines and
fixed some things (stated below the relevant parts of your message).
Here is the new version of the patch.

El jue, 12-06-2008 a las 16:31 -0400, Pavel Roskin escribió:
> 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.
I could not find the line you were referring to. I could only find one
line starting with a plus (in install_int13_handler), but it does not
end with a space/tab. However, I did find one trailing tab after a
comment, and removed it.
> 
> 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.
Both corrected. Sorry, I work on a wide screen and, tough I tried to
uphold the 80-char line rule, I'm not used to it. I think there was a
switch in gedit to graphically display the 80-char limit, but I can't
find it now...

OOC: do you know if there is a syntax highlighting mode for gas/x86
assembly code in gedit? Right now the program thinks the code is C, and
the only opcode it highlights is "int". Even a gas-only highlighting
(i.e. only the directives, not the opcodes) would be useful.
> 
> 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.
Removed. That was a leftover from the early stages when the "mappings"
area of the int13h handler had a maximum size of 8 entries. Then, I
decided that the space for the map was to be dynamically allocated, so I
moved it to the end of the block and set the .space directive to zero.
> 
> 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.
Removed. Those are leftovers too, from the time the patch modified the
loader.h and loader.S files, which are public and #included, so I didn't
want to contaminate the namespace. Since Vesa's review, I moved the
relevant code to its own files inside the module, and thus it's no
longer public.

I also removed four lines from some old debug code that would have made
the int13h handler crash if the "unmap" section had been uncommented.

drivemap.patch
Description: Text Data