[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Drivemap module
|
From: |
Javier Martín |
|
Subject: |
Re: [PATCH] Drivemap module |
|
Date: |
Tue, 05 Aug 2008 18:39:41 +0200 |
Ok, I will reply to your last three messages on a single post:
Hi there!
El mar, 05-08-2008 a las 13:31 +0200, Marco Gerards escribió:
> Hi,
>
> Javier Martín <address@hidden> writes:
>
> >> Anyway, since "they" are more likely to maintain the code in
> >> the long run than you, in general, the question is whether
> >> the code is more likely to become buggy by their hacking on
> >> it, if it follows project coding style or someone else's
> >> (your) "safer" coding style. Likely it's safer if using a
> >> consistent programming style. Although I personally don't
> >> see that it's very helpful to have a style that makes things
> >> down to the order of "==" arguments be consistent within the
> >> project; haphazard only slows reading the tiniest bit, and I
> >> don't think it makes a different what order the arguments are...
> >
> > Hmm... I was partially expecting a flamefest to start. Pity ^^
> > Well, let's spill a little napalm: the GNU style bracing is extremely
> > silly!! Why the hell are the "if" and "else" blocks indented differently
> > in this example?
> > if (condition)
> > return 0;
> > else
> > {
> > return -1;
> > }
> > Nah, I'm not really bringing that issue, I was just joking, and in fact
> > I'm reconsidering my objections to the operator== arguments order rule,
> > even though I still consider my style safer and more sensible. If
> > someone else wants to express their opinion on that issue, do it fast
> > before I completely submit to Master Marco's will :D
>
> Please don't be sarcastic, start flame wars or call names. It will not
> help anyone.
Ok, sorry, picturing you as a whip-cracking dominatrix was really not proper
^^. I was just joking about how fast can flamewars be started.
>
> --
> Marco
>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
El mar, 05-08-2008 a las 13:23 +0200, Marco Gerards escribió:
> Hi Javier,
>
> Javier Martín <address@hidden> writes:
>
> > El lun, 04-08-2008 a las 22:51 +0200, Marco Gerards escribió:
> >> Javier Martín <address@hidden> writes:
> >>
> >> > After your latest replay, I "reevaluated" my stubbornness WRT some of
> >> > your advices, and I've changed a few things:
> >> >
> >> > - Variables are now declared (and, if possible, initialized) before
> >> > precondition checks, even simple ones. The install_int13_handler
> >> > function has not been modified, however, since I find it a bit
> >> > nonsensical to put bunch of declarations without an obvious meaning just
> >> > after the "else" line:
> >> > grub_uint32_t *ivtslot;
> >> > grub_uint16_t *bpa_freekb;
> >> > grub_size_t total_size;
> >> > grub_uint16_t payload_sizekb;
> >> > grub_uint8_t *handler_base;
> >> > int13map_node_t *handler_map;
> >> > grub_uint32_t ivtentry;
> >>
> >> Please understand me correctly. Code just has to be written according
> >> to our coding standards before it can and will be included. We can
> >> discuss things endlessly but we will simply stick to the GNU Coding
> >> Standards as you might expect.
> >>
> >> I hope you can appreciate that all code of GRUB has the same coding
> >> style, if you like this style or not.
> > Big sigh. Function modified to conform to your preciousss coding style.
> > I might look a bit childish or arrogant saying this, but what is the
> > point upholding a coding style that, no matter how consistent it is,
> > hinders readability. With so much local variables, they are hard to keep
> > track of no matter the coding style, but with the old ("mixed, heretic")
> > style there was not need for a comment before each declaration: they
> > were declared and used when needed instead of at the start of a block,
> > because that function is inherently linear, not block-structured.
>
> In your opinion it is not a good coding style. I just avoid
> discussions about it because such discussions just waste my time.
> Even if you are able to convince me, GRUB is a GNU project and will
> remain to use the GCS.
Which is fine, but the order of the arguments to == is specified nowhere in the
GCS: the order you defend only appears in examples less than five times. Thus,
your insistence is a matter of internal consistence within GRUB and is only
based on examples in the GCS. Additionally, your previous suggestion to change
checks line "entries == 0 " to "!entries" are _against_ the examples of the
GCS, even for pointers.
Then, given that the _examples_ in the GCS are non-authoritative, I was
advocating a simple change for better resilience against future changes. You
want to keep consistency with the current sources, and that I understand, so I
will perform the required trivial changes in my code to use the style used in
other GRUB code, but I still think it is worse and more error-prone.
>
>
> >> > - Only one declaration per line: even though C is a bit absurd in not
> >> > recognizing T* as a first class type and instead thinking of * as a
> >> > qualifier to the variable name; and even though my code does not incur
> >> > into such ambiguities.
> >> > - Comments moved as you required, reworded as needed
> >> > - Extensive printf showing quasi-help in the "show" mode trimmed down to
> >> > just the first sentence.
> >> > - Internal helper functions now use the standard error handling, i.e.
> >> > return grub_error (err, fmt, ...)
> >> > - Comment about the strange "void" type instead of "void*" rephrased to
> >> > be clearer
> >>
> >> Thanks a lot!
> >>
> >> > There is, however, one point in which I keep my objection: comparisons
> >> > between a variable and a constant should be of the form CONSTANT ==
> >> > variable and not in the reverse order, since an erroneous but quite
> >> > possible change of == for = results in a compile-time error instead of a
> >> > _extremely_ difficult to trace runtime bug. Such kind of bugs are quite
> >> > excruciating to find in userspace applications within an IDE, so I can't
> >> > even consider the pain to debug them in a bootloader.
> >>
> >> I understand your concern, nevertheless, can you please change it?
> > You understand my concern, but seemingly do not understand that in order
> > to conform to the Holy Coding Style you are asking me to write code that
> > can become buggy (and with a very hard to trace bug) with a simple
> > deltion! (point: did you notice that last word _without_ a spelling
> > checker? Now try to do so in a multithousand-line program).
>
> Yes, I did notice it immediately.
>
> The coding style is not holy in any way. Everyone has its own coding
> style. You must understand I do not want to fully discuss it every
> time someone sends in a patch, especially since it will not change
> anyways.
By the way, I just noticed that you write _normal_ text with two spaces after a
dot, just like comments in the code! o_O
>
> > While tools like `svn diff' can help in these kind of problems, their
> > utility is greatly diminished if the offending change is part of a
> > multi-line change. Besides, sometimes checks like "if (a=b)", or more
> > frequently "if (a=f())" are intentionally used in C, so the error might
> > become even more difficult to spot and correct. I ask for a deep
> > reflection on this issue: maybe I'm dead wrong and an arrogant brat in
> > my attempt to tackle the Holy GNU Coding Standards, but I'd like to ask
> > input from more people on this.
>
> I will refuse patches with "if (a = f())", if that makes you sleep
> better ;-)
In fact the GCS discourages that use, but allows the same in loop checks,
particularly "while" loops.
>
> >> > WRT accepting raw BIOS disk numbers, I agree with you in principle, but
> >> > I'm keeping the functionality for now, since I don't quite like the
> >> > "drivemap (hd0) (hd1)" syntax - which device is which?. I'd rather have
> >> > something akin to "drivemap (hd0) (bios:hd1)", but I want to hear the
> >> > opinions of people in this list.
> >>
> >> I personally do not care a lot if BIOS disk numbers are used.
> >> Although I do not see why it is useful.
> >>
> >> As for the syntax, I would prefer something more GRUB2ish, like:
> >>
> >> map --bios=(hd0) --os=(hd1)
> > I agree. What about "grub" for the source drive instead of "os", with
> > "bios" being the target drive? I mean:
> > drivemap --grub=(hd1) --bios=(hd0)
> > Would map 0x80 in the BIOS to grub's (hd1) drive which will most likely
> > be BIOS drive 0x81.
>
> It will not change the functionality which GRUB is active. But it
> will change it for the OS that is loaded. So I do not think --grub=
> is a good idea because of this. As for GRUB Legacy, it confused a lot
> of people, so making people explicitly say that it is changed for the
> OS, this confusion might go away :-)
>
> > However, I prefer not to change it right now. Maybe when there are no
> > other issues WRT to this patch we can set on to tackle this one.
>
> So first I'll review this patch, then you will send in a new one?
Of course. I meant that I prefer to smash all "syntactic" changes in the patch
before introducing a functionality change that could lead to new syntactic
issues.
>
> >> Or so, perhaps the long argument names can be chosen in a more clever
> >> way :-)
> >> > The new version of the patch is attached, and here is my suggested CLog:
> >> >
> >> > 2008-08-XX Javier Martin <address@hidden>
> >>
> >> (newline)
> >>
> >> > * commands/i386/pc/drivemap.c : New file.
> >> > * commands/i386/pc/drivemap_int13h.S : New file.
> >> > * conf/i386-pc.rmk (pkglib_MODULES) : Added drivemap.mod
> >> > (drivemap_mod_SOURCES) : New variable
> >> > (drivemap_mod_ASFLAGS) : Likewise
> >> > (drivemap_mod_CFLAGS) : Likewise
> >> > (drivemap_mod_LDFLAGS) : Likewise
> >> > * include/grub/loader.h (grub_loader_register_preboot) : New
> >> > function prototype. Register a new pre-boot handler
> >>
> >> No need how the change is used or why it was added.
> >>
> >> > (grub_loader_unregister_preboot) : Likewise. Unregister handler
> >>
> >> Same here.
> >>
> >> > (grub_preboot_hookid) : New typedef. Registered hook "handle"
> >>
> >> Same here.
> >>
> >> > * kern/loader.c (grub_loader_register_preboot) : New function.
> >> > (grub_loader_unregister_preboot) : Likewise.
> >> > (preboot_hooks) : New variable. Linked list of preboot hooks
> >>
> >> Same here.
> >>
> >> > (grub_loader_boot) : Call the list of preboot-hooks before the
> >> > actual loader
> >>
> >> What's the `'?
> > The what? o_O
>
> I see some weird character in your text. My font shows it as a block
> before every `*'.
I see nothing: "What's the `'?". Maybe it's some kind of tab?
>
> >> Please do not add a space before the ":"
> > Ok, everything corrected. New CL entry:
> >
> > 2008-08-XX Javier Martin <address@hidden>
> >
> > * commands/i386/pc/drivemap.c: New file.
> > * commands/i386/pc/drivemap_int13h.S: New file.
> > * conf/i386-pc.rmk (pkglib_MODULES): Added drivemap.mod
> > (drivemap_mod_SOURCES): New variable
> > (drivemap_mod_ASFLAGS): Likewise
> > (drivemap_mod_CFLAGS): Likewise
> > (drivemap_mod_LDFLAGS): Likewise
> > * include/grub/loader.h (grub_loader_register_preboot): New
> > function prototype.
> > (grub_loader_unregister_preboot): Likewise.
> > (grub_preboot_hookid): New typedef.
> > * kern/loader.c (grub_loader_register_preboot): New function.
> > (grub_loader_unregister_preboot): Likewise.
> > (preboot_hooks): New variable.
> > (grub_loader_boot): Call the list of preboot-hooks before the
> > actual loader
>
> Please add a `.' after "New variable" and "Likewise", same for the
> third and the last sentence. Sometimes you did it right :-).
>
Done. New CL entry will go at the end of this neverending post.
>
> >> Some comments can be found below. Can you please fix the code mention
> >> in the review and similar code? I really want the code to be just
> >> like any other GRUB code. Please understand that we have to maintain
> >> this in the future. If everyone would use his own codingstyle, GRUB
> >> would become unmaintainable.
> >>
> >> > Index: commands/i386/pc/drivemap.c
> >> > ===================================================================
> >> > --- commands/i386/pc/drivemap.c (revisión: 0)
> >> > +++ commands/i386/pc/drivemap.c (revisión: 0)
> >> > @@ -0,0 +1,417 @@
> >> > +/* drivemap.c - command to manage the BIOS drive mappings. */
> >> > +/*
> >> > + * GRUB -- GRand Unified Bootloader
> >> > + * Copyright (C) 2008 Free Software Foundation, Inc.
> >> > + *
> >> > + * GRUB is free software: you can redistribute it and/or modify
> >> > + * it under the terms of the GNU General Public License as published by
> >> > + * the Free Software Foundation, either version 3 of the License, or
> >> > + * (at your option) any later version.
> >> > + *
> >> > + * GRUB is distributed in the hope that it will be useful,
> >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> > + * GNU General Public License for more details.
> >> > + *
> >> > + * You should have received a copy of the GNU General Public License
> >> > + * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
> >> > + */
> >> > +
> >> > +#include <grub/normal.h>
> >> > +#include <grub/dl.h>
> >> > +#include <grub/mm.h>
> >> > +#include <grub/misc.h>
> >> > +#include <grub/disk.h>
> >> > +#include <grub/loader.h>
> >> > +#include <grub/machine/loader.h>
> >> > +#include <grub/machine/biosdisk.h>
> >> > +
> >> > +#define MODNAME "drivemap"
> >> > +
> >> > +static const struct grub_arg_option options[] = {
> >> > + {"list", 'l', 0, "show the current mappings", 0, 0},
> >> > + {"reset", 'r', 0, "reset all mappings to the default values", 0, 0},
> >> > + {0, 0, 0, 0, 0, 0}
> >> > +};
> >> > +
> >> > +/* Syms/vars/funcs exported from drivemap_int13h.S - start. */
> >> > +
> >> > +/* Realmode far ptr = 2 * 16b */
> >> > +extern grub_uint32_t grub_drivemap_int13_oldhandler;
> >> > +/* Size of the section to be copied */
> >> > +extern grub_uint16_t grub_drivemap_int13_size;
> >> > +
> >> > +/* This type is used for imported assembly labels, takes no storage and
> >> > is only
> >> > + used to take the symbol address with &label. Do NOT put void* here.
> >> > */
> >> > +typedef void grub_symbol_t;
> >> > +extern grub_symbol_t grub_drivemap_int13_handler_base;
> >> > +extern grub_symbol_t grub_drivemap_int13_mapstart;
> >> > +
> >> > +void grub_drivemap_int13_handler (void);
> >>
> >> The lines above belong in a header file.
> > True, but they are used in a single file in the whole project and thus I
> > see it pointless to extract an unneeded header, which would become one
> > more SVN object to track. However, if you insist I will split the header
> > file at once. In particular, I think the grub_symbol_t typedef should go
> > into the "standard" GRUB headers somehow (but not in symbol.h, which is
> > included from assembly files).
>
> Please do so.
>
> Other people might want to comment on the symbol change. They will
> most likely miss it if we keep discussing it here ;-). Can you please
> send that in as a separate change to give other the opportunity to
> react on it?
Ok, so in a first stage I will put the discussed block in a drivemap.h file in
include/, then when that's committed start a discussion about moving
grub_symbol_t from drivemap.h to another header file.
>
> >> > +/* Syms/vars/funcs exported from drivemap_int13h.S - end. */
> >> > +
> >> > +
> >> > +static grub_preboot_hookid insthandler_hook;
> >> > +
> >> > +typedef struct drivemap_node
> >> > +{
> >> > + grub_uint8_t newdrive;
> >> > + grub_uint8_t redirto;
> >> > + struct drivemap_node *next;
> >> > +} drivemap_node_t;
> >> > +
> >> > +static drivemap_node_t *drivemap = 0;
> >>
> >> No need to set this to zero.
> > Yes, you said so already, but I wanted to make the initial state very
> > explicit to a future developer, since that variable is checked against
> > zero in several points. Given that the added source size is four bytes
> > and the added binary size is _zero_, is all the fuss really necessary?
> > Notice that I changed the other variable in which you pointed out this
> > issue, because it is not checked against zero anywhere.
>
> Please do so anyways.
>
> >> > +static grub_err_t install_int13_handler (void);
> >> > +
> >> > +/* Puts the specified mapping into the table, replacing an existing
> >> > mapping
> >> > + for newdrive or adding a new one if required. */
> >> > +static grub_err_t
> >> > +drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
> >> > +
> >>
> >> Please do not add a newline here.
> > Oops, sorry. I forgot to remove it when moving the comment
>
> :-)
>
> >> > + drivemap_node_t *mapping = 0;
> >> > + drivemap_node_t *search = drivemap;
> >> > + while (search)
> >> > + {
> >> > + if (search->newdrive == newdrive)
> >> > + {
> >> > + mapping = search;
> >> > + break;
> >> > + }
> >> > + search = search->next;
> >> > + }
> >> > +
> >> > +
> >> > + /* Check for pre-existing mappings to modify before creating a new
> >> > one. */
> >> > + if (mapping)
> >> > + mapping->redirto = redirto;
> >> > + else
> >> > + {
> >> > + mapping = grub_malloc (sizeof (drivemap_node_t));
> >> > + if (!mapping)
> >> > + return grub_error (GRUB_ERR_OUT_OF_MEMORY,
> >> > + "cannot allocate map entry, not enough
> >> > memory");
> >> > + mapping->newdrive = newdrive;
> >> > + mapping->redirto = redirto;
> >> > + mapping->next = drivemap;
> >> > + drivemap = mapping;
> >> > + }
> >> > + return GRUB_ERR_NONE;
> >> > +}
> >> > +
> >> > +/* Removes the mapping for newdrive from the table. If there is no
> >> > mapping,
> >> > + then this function behaves like a no-op on the map. */
> >> > +static void
> >> > +drivemap_remove (grub_uint8_t newdrive)
> >> > +{
> >> > + drivemap_node_t *mapping = 0;
> >> > + drivemap_node_t *search = drivemap;
> >> > + drivemap_node_t *previous = 0;
> >> > + while (search)
> >> > + {
> >> > + if (search->newdrive == newdrive)
> >> > + {
> >> > + mapping = search;
> >> > + break;
> >> > + }
> >> > + previous = search;
> >> > + search = search->next;
> >> > + }
> >> > + if (mapping) /* Found. */
> >>
> >> You forgot one.
> > Corrected. Sorry.
> >>
> >> > + {
> >> > + if (previous)
> >> > + previous->next = mapping->next;
> >> > + else /* Entry was head of list. */
> >> > + drivemap = mapping->next;
> >> > + grub_free (mapping);
> >> > + }
> >> > +}
> >> > +
> >> > +/* Given a device name, resolves its BIOS disk number and stores it in
> >> > the
> >> > + passed location, which should only be trusted if ERR_NONE is
> >> > returned. */
> >> > +static grub_err_t
> >> > +parse_biosdisk (const char *name, grub_uint8_t *disknum)
> >> > +{
> >> > + grub_disk_t disk;
> >> > + if (!name || 0 == *name)
> >> > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");
> >> > + /* Skip the first ( in (hd0) - disk_open wants just the name. */
> >> > + if (*name == '(')
> >> > + name++;
> >> > +
> >> > + disk = grub_disk_open (name);
> >> > + if (!disk)
> >> > + return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device
> >> > \"%s\"", name);
> >> > + else
> >> > + {
> >> > + const enum grub_disk_dev_id id = disk->dev->id;
> >> > + /* The following assignment is only sound if the device is indeed
> >> > a
> >> > + biosdisk. The caller must check the return value. */
> >> > + if (disknum)
> >> > + *disknum = disk->id;
> >> > + grub_disk_close (disk);
> >> > + if (GRUB_DISK_DEVICE_BIOSDISK_ID == id)
> >> > + return GRUB_ERR_NONE;
> >> > + else return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS
> >> > disk", name);
> >> > + }
> >> > +}
> >> > +
> >> > +/* Given a BIOS disk number, returns its GRUB device name if it exists.
> >> > + For nonexisting BIOS dnums, this function returns
> >> > ERR_UNKNOWN_DEVICE. */
> >>
> >> This is GRUB_ERR_UNKNOWN_DEVICE
> > I know, I consciously left the GRUB_ part out because 1) it would
> > require the line to be split and 2) that prefix is all over the place.
> > Corrected, however.
> >>
> >> > +static grub_err_t
> >> > +revparse_biosdisk(const grub_uint8_t dnum, const char **output)
> >> > +{
> >> > + int found = 0;
> >> > + auto int find (const char *name);
> >> > + int find (const char *name)
> >> > + {
> >> > + const grub_disk_t disk = grub_disk_open (name);
> >> > + if (!disk)
> >> > + return 0;
> >> > + else
> >> > + {
> >> > + if (disk->id == dnum && GRUB_DISK_DEVICE_BIOSDISK_ID ==
> >> > disk->dev->id)
> >> > + {
> >> > + found = 1;
> >> > + if (output)
> >> > + *output = name;
> >> > + }
> >> > + grub_disk_close (disk);
> >> > + return found;
> >> > + }
> >> > + }
> >> > +
> >> > + grub_disk_dev_iterate (find);
> >> > + if (found)
> >> > + return GRUB_ERR_NONE;
> >> > + else return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %d not
> >> > found", dnum);
> >>
> >> Please change this.
> > Em... to what? What is the problem? Do you want me to reverse the
> > comparison? i.e. if (!found) { return error; } else { return ok; }
>
> The return is on the same line as the else.
>
Corrected.
>
> >> > +/* Given a GRUB-like device name and a convenient location, stores the
> >> > related
> >> > + BIOS disk number. Accepts devices like \((f|h)dN\), with 0 <= N <
> >> > 128. */
> >> > +static grub_err_t
> >> > +tryparse_diskstring (const char *str, grub_uint8_t *output)
> >> > +{
> >> > + if (!str || 0 == *str)
> >> > + goto fail;
> >> > + /* Skip opening paren in order to allow both (hd0) and hd0. */
> >> > + if (*str == '(')
> >> > + str++;
> >> > + if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
> >> > + {
> >> > + grub_uint8_t bios_num = (str[0] == 'h')? 0x80 : 0x00;
> >> > + grub_errno = GRUB_ERR_NONE;
> >> > + unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
> >> > + if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
> >> > + /* N not a number or out of range */
> >> > + goto fail;
> >>
> >> Can you put this between braces, now comment was added.
> > Done.
> >>
> >> > + else
> >> > + {
> >> > + bios_num |= drivenum;
> >> > + if (output)
> >> > + *output = bios_num;
> >> > + return GRUB_ERR_NONE;
> >> > + }
> >> > + }
> >> > + else goto fail;
> >>
> >> ...
> > What's the problem here? The lack of braces? The goto (as used in the
> > ext2 code)?
>
> goto is on the same line as the else.
Corrected, though I find this change less logic than the last one (splitting
the else retun grub_error(...)) line because this generates two _extremely_
short lines;
>
> >> > +fail:
> >> > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\"
> >> > invalid: must"
> >> > + "be (f|h)dN, with 0 <= N < 128", str);
> >> > +}
> >> > +
> >> > +static grub_err_t
> >> > +grub_cmd_drivemap (struct grub_arg_list *state, int argc, char **args)
> >> > +{
> >> > + if (state[0].set)
> >> > + {
> >> > + /* Show: list mappings. */
> >> > + if (!drivemap)
> >> > + grub_printf ("No drives have been remapped");
> >> > + else
> >> > + {
> >> > + grub_printf ("Showing only remapped drives.\n");
> >> > + grub_printf ("Mapped\tGRUB\n");
> >> > + drivemap_node_t *curnode = drivemap;
> >> > + while (curnode)
> >> > + {
> >> > + const char *dname = 0;
> >> > + grub_err_t err = revparse_biosdisk (curnode->redirto,
> >> > &dname);
> >> > + if (err != GRUB_ERR_NONE)
> >> > + return grub_error (err, "invalid mapping: non-existent
> >> > disk"
> >> > + "or not managed by the BIOS");
> >> > + grub_printf("0x%02x\t%4s\n", curnode->newdrive, dname);
> >> > + curnode = curnode->next;
> >> > + }
> >> > + }
> >> > + }
> >> > + else if (state[1].set)
> >> > + {
> >> > + /* Reset: just delete all mappings, freeing their memory. */
> >> > + drivemap_node_t *curnode = drivemap;
> >> > + drivemap_node_t *prevnode = 0;
> >> > + while (curnode)
> >> > + {
> >> > + prevnode = curnode;
> >> > + curnode = curnode->next;
> >> > + grub_free (prevnode);
> >> > + }
> >> > + drivemap = 0;
> >> > + }
> >> > + else
> >> > + {
> >> > + /* Neither flag: put mapping */
> >>
> >> ". */
> > Done
> >>
> >> > + grub_uint8_t mapfrom = 0;
> >> > + grub_uint8_t mapto = 0xFF;
> >> > + grub_err_t err;
> >> > +
> >> > + if (argc != 2)
> >> > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments
> >> > required");
> >> > +
> >> > + err = parse_biosdisk (args[0], &mapfrom);
> >> > + if (err != GRUB_ERR_NONE)
> >> > + return err;
> >> > +
> >> > + err = tryparse_diskstring (args[1], &mapto);
> >> > + if (err != GRUB_ERR_NONE) /* Not a disk string. Maybe a raw num
> >> > then? */
> >>
> >> Please move this up.
> > Done.
> >>
> >> > + {
> >> > + grub_errno = GRUB_ERR_NONE;
> >> > + unsigned long num = grub_strtoul (args[1], 0, 0);
> >> > + if (grub_errno != GRUB_ERR_NONE || num > 0xFF) /* Not a raw
> >> > num or too high. */
> >> > + return grub_error (grub_errno,
> >> > + "Target specifier must be of the form
> >> > (fdN) or "
> >> > + "(hdN), with 0 <= N < 128; or a plain
> >> > dec/hex "
> >> > + "number between 0 and 255");
> >> > + else mapto = (grub_uint8_t)num;
> >> > + }
> >> > +
> >> > + if (mapto == mapfrom) /* Reset to default. */
> >>
> >> Same here.
> > Done.
> >>
> >> > + {
> >> > + grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)",
> >> > args[0], mapfrom);
> >> > + drivemap_remove (mapfrom);
> >> > + }
> >> > + else /* Map. */
> >>
> >> Please move the comment inside the braces below.
> > Done, and reworded.
> >>
> >> > + {
> >> > + grub_dprintf (MODNAME, "Mapping %s (%02x) to %02x\n",
> >> > args[0], mapfrom, mapto);
> >> > + return drivemap_set ((grub_uint8_t)mapto, mapfrom);
> >> > + }
> >> > + }
> >> > +
> >> > + return GRUB_ERR_NONE;
> >> > +}
> >> > +
> >> > +typedef struct __attribute__ ((packed)) int13map_node
> >> > +{
> >> > + grub_uint8_t disknum;
> >> > + grub_uint8_t mapto;
> >> > +} int13map_node_t;
> >> > +
> >> > +/* The min amount of mem that must remain free after installing the
> >> > handler.
> >> > + 32 KiB is just above 0x7C00-0x7E00, where the bootsector is loaded.
> >> > */
> >> > +#define MIN_FREE_MEM_KB 32
> >> > +#define INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) -
> >> > ((grub_uint8_t*)&grub_drivemap_int13_handler_base) )
> >> > +#define INT13H_REBASE(x) ( (void*) (((grub_uint8_t*)handler_base) +
> >> > (x)) )
> >> > +#define INT13H_TONEWADDR(x) INT13H_REBASE( INT13H_OFFSET( x ) )
> >> > +
> >> > +/* Int13h handler installer - reserves conventional memory for the
> >> > handler,
> >> > + copies it over and sets the IVT entry for int13h.
> >> > + This code rests on the assumption that GRUB does not activate any
> >> > kind of
> >> > + memory mapping apart from identity paging, since it accesses realmode
> >> > + structures by their absolute addresses, like the IVT at 0 or the BDA
> >> > at
> >> > + 0x400; and transforms a pmode pointer into a rmode seg:off far ptr.
> >> > */
> >> > +static grub_err_t
> >> > +install_int13_handler (void)
> >> > +{
> >> > + grub_size_t entries = 0;
> >> > + drivemap_node_t *curentry = drivemap;
> >> > + while (curentry) /* Count entries to prepare a contiguous map block.
> >> > */
> >>
> >> ...
> > Comment moved up.
> >>
> >> > + {
> >> > + entries++;
> >> > + curentry = curentry->next;
> >> > + }
> >> > + if (0 == entries)
> >>
> >> I know this is what you prefer, but can you change this nevertheless?
> > I refer to my objection near the top of the post.
>
> I know you object, but did you change it?
Done.
>
> >> > + grub_dprintf (MODNAME, "No drives marked as remapped,
> >> > installation of"
> >> > + "an int13h handler is not required.");
> >> > + return GRUB_ERR_NONE; /* No need to install the int13h handler.
> >> > */
> >> > + }
> >> > + else
> >> > + {
> >> > + grub_dprintf (MODNAME, "Installing int13h handler...\n");
> >> > + grub_uint32_t *ivtslot = (grub_uint32_t*)0x0000004c;
> >> > +
> >> > + /* Save the pointer to the old int13h handler. */
> >> > + grub_drivemap_int13_oldhandler = *ivtslot;
> >> > + grub_dprintf (MODNAME, "Old int13 handler at %04x:%04x\n",
> >> > + (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff,
> >> > + grub_drivemap_int13_oldhandler & 0x0ffff);
> >> > +
> >> > + /* Reserve a section of conventional memory as "BIOS memory" for
> >> > handler:
> >> > + BDA offset 0x13 contains the top of such memory. */
> >> > + grub_uint16_t *bpa_freekb = (grub_uint16_t*)0x00000413;
> >> > + grub_dprintf (MODNAME, "Top of conventional memory: %u KiB\n",
> >> > *bpa_freekb);
> >> > + grub_size_t total_size = grub_drivemap_int13_size
> >> > + + (entries + 1) * sizeof(int13map_node_t);
> >> > + grub_uint16_t payload_sizekb = (total_size >> 10) +
> >> > + (((total_size % 1024) == 0) ? 0 :
> >> > 1);
> >> > + if ((*bpa_freekb - payload_sizekb) < MIN_FREE_MEM_KB)
> >> > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "refusing to install"
> >> > + "int13 handler, not enough free memory
> >> > after");
> >> > + grub_dprintf (MODNAME, "Payload is %u b long, reserving %u Kb\n",
> >> > + total_size, payload_sizekb);
> >> > + *bpa_freekb -= payload_sizekb;
> >> > +
> >> > + /* Copy int13h handler chunk to reserved area. */
> >> > + grub_uint8_t *handler_base = (grub_uint8_t*)(*bpa_freekb << 10);
> >> > + grub_dprintf (MODNAME, "Copying int13 handler to: %p\n",
> >> > handler_base);
> >> > + grub_memcpy (handler_base, &grub_drivemap_int13_handler_base,
> >> > + grub_drivemap_int13_size);
> >> > +
> >> > + /* Copy the mappings to the reserved area. */
> >> > + curentry = drivemap;
> >> > + grub_size_t i;
> >> > + int13map_node_t *handler_map = (int13map_node_t*)
> >> > + INT13H_TONEWADDR (&grub_drivemap_int13_mapstart);
> >> > + grub_dprintf (MODNAME, "Target map at %p, copying mappings...\n",
> >> > handler_map);
> >> > + for (i = 0; i < entries && curentry; i++, curentry =
> >> > curentry->next)
> >> > + {
> >> > + handler_map[i].disknum = curentry->newdrive;
> >> > + handler_map[i].mapto = curentry->redirto;
> >> > + grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x\n", i,
> >> > + handler_map[i].disknum,
> >> > handler_map[i].mapto);
> >> > + }
> >> > + /* Signal end-of-map. */
> >> > + handler_map[i].disknum = 0;
> >> > + handler_map[i].mapto = 0;
> >> > + grub_dprintf (MODNAME, "\t#%d: 0x%02x <- 0x%02x (end)\n", i,
> >> > + handler_map[i].disknum,
> >> > handler_map[i].mapto);
> >> > +
> >> > + /* Install our function as the int13h handler in the IVT. */
> >> > + grub_uint32_t ivtentry = ((grub_uint32_t)handler_base) << 12; /*
> >> > Segment address. */
> >> > + ivtentry |= (grub_uint16_t)
> >> > INT13H_OFFSET(grub_drivemap_int13_handler);
> >> > + grub_dprintf (MODNAME, "New int13 handler IVT pointer:
> >> > %04x:%04x\n",
> >> > + (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);
> >> > + *ivtslot = ivtentry;
> >> > +
> >> > + return GRUB_ERR_NONE;
> >> > + }
> >> > +}
> >> > +
> >> > +GRUB_MOD_INIT (drivemap)
> >> > +{
> >> > + (void) mod; /* Stop warning. */
> >> > + grub_register_command (MODNAME, grub_cmd_drivemap,
> >> > + GRUB_COMMAND_FLAG_BOTH,
> >> > + MODNAME " -s | -r | (hdX)
> >> > newdrivenum",
> >> > + "Manage the BIOS drive mappings", options);
> >> > + insthandler_hook = grub_loader_register_preboot
> >> > (&install_int13_handler, 1);
> >> > +}
> >> > +
> >> > +GRUB_MOD_FINI (drivemap)
> >> > +{
> >> > + grub_loader_unregister_preboot (insthandler_hook);
> >> > + insthandler_hook = 0;
> >> > + grub_unregister_command (MODNAME);
> >> > +}
> >> > +
> >> > Index: commands/i386/pc/drivemap_int13h.S
> >> > ===================================================================
> >> > --- commands/i386/pc/drivemap_int13h.S (revisión: 0)
> >> > +++ commands/i386/pc/drivemap_int13h.S (revisión: 0)
> >> > @@ -0,0 +1,118 @@
> >> > +/*
> >> > + * GRUB -- GRand Unified Bootloader
> >> > + * Copyright (C) 1999,2000,2001,2002,2003,2005,2006,2007,2008 Free
> >> > Software Foundation, Inc.
> >> > + *
> >> > + * GRUB is free software: you can redistribute it and/or modify
> >> > + * it under the terms of the GNU General Public License as published by
> >> > + * the Free Software Foundation, either version 3 of the License, or
> >> > + * (at your option) any later version.
> >> > + *
> >> > + * GRUB is distributed in the hope that it will be useful,
> >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> > + * GNU General Public License for more details.
> >> > + *
> >> > + * You should have received a copy of the GNU General Public License
> >> > + * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
> >> > + */
> >> > +
> >> > +
> >> > +/*
> >> > + * Note: These functions defined in this file may be called from C.
> >> > + * Be careful of that you must not modify some registers. Quote
> >> > + * from gcc-2.95.2/gcc/config/i386/i386.h:
> >> > +
> >> > + 1 for registers not available across function calls.
> >> > + These must include the FIXED_REGISTERS and also any
> >> > + registers that can be used without being saved.
> >> > + The latter must include the registers where values are returned
> >> > + and the register where structure-value addresses are passed.
> >> > + Aside from that, you can include as many other registers as you like.
> >> > +
> >> > + ax,dx,cx,bx,si,di,bp,sp,st,st1,st2,st3,st4,st5,st6,st7,arg
> >> > +{ 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 }
> >> > + */
> >> > +
> >> > +/*
> >> > + * Note: GRUB is compiled with the options -mrtd and -mregparm=3.
> >> > + * So the first three arguments are passed in %eax, %edx, and
> >> > %ecx,
> >> > + * respectively, and if a function has a fixed number of arguments
> >> > + * and the number if greater than three, the function must return
> >> > + * with "ret $N" where N is ((the number of arguments) - 3) * 4.
> >> > + */
> >> > +
> >> > +#include <grub/symbol.h>
> >> > +
> >> > +#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) -
> >> > grub_drivemap_int13_handler_base)
> >> > +
> >> > +/* Copy starts here. When deployed, this label must be segment-aligned
> >> > */
> >> > +VARIABLE(grub_drivemap_int13_handler_base)
> >> > +
> >> > +VARIABLE(grub_drivemap_int13_oldhandler)
> >> > + .word 0xdead, 0xbeef
> >> > +/* Drivemap module - INT 13h handler - BIOS HD map */
> >> > +/* We need to use relative addressing, and with CS to top it all, since
> >> > we
> >> > + must make as few changes to the registers as possible. IP-relative
> >> > + addressing like on amd64 would make life way easier here. */
> >> > +.code16
> >> > +FUNCTION(grub_drivemap_int13_handler)
> >> > + push %bp
> >> > + mov %sp, %bp
> >> > + push %ax /* We'll need it later to determine the used BIOS function
> >> > */
> >> > +
> >> > + /* Map the drive number (always in DL?) */
> >> > + push %ax
> >> > + push %bx
> >> > + push %si
> >> > + mov $GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx
> >> > + xor %si, %si
> >> > +1:movw %cs:(%bx,%si), %ax
> >> > + cmp %ah, %al
> >> > + jz 3f /* DRV=DST => map end - drive not remapped, leave DL as-is */
> >> > + cmp %dl, %al
> >> > + jz 2f /* Found - drive remapped, modify DL */
> >> > + add $2, %si
> >> > + jmp 1b /* Not found, but more remaining, loop */
> >> > +2:mov %ah, %dl
> >> > +3:pop %si
> >> > + pop %bx
> >> > + xchgw %ax, -4(%bp) /* Recover the old AX and save the map entry for
> >> > later */
> >> > +
> >> > + push %bp
> >> > + /* Simulate interrupt call: push flags and do a far call in order to
> >> > set
> >> > + the stack the way the old handler expects it so that its iret
> >> > works */
> >> > + push 6(%bp)
> >> > + movw (%bp), %bp /* Restore the caller BP (is this needed and/or
> >> > sensible?) */
> >> > + lcall *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
> >> > + pop %bp /* The pushed flags were removed by iret */
> >> > + /* Set the saved flags to what the int13h handler returned */
> >> > + push %ax
> >> > + pushf
> >> > + pop %ax
> >> > + movw %ax, 6(%bp)
> >> > + pop %ax
> >> > +
> >> > + /* Reverse map any returned drive number if the data returned
> >> > includes it.
> >> > + The only func that does this seems to be origAH = 0x08, but many
> >> > BIOS
> >> > + refs say retDL = # of drives connected. However, the GRUB Legacy
> >> > code
> >> > + treats this as the _drive number_ and "undoes" the remapping.
> >> > Thus,
> >> > + this section has been disabled for testing if it's required */
> >> > +# cmpb $0x08, -1(%bp) /* Caller's AH */
> >> > +# jne 4f
> >> > +# xchgw %ax, -4(%bp) /* Map entry used */
> >> > +# cmp %ah, %al /* DRV=DST => drive not remapped */
> >> > +# je 4f
> >> > +# mov %ah, %dl /* Undo remap */
> >> > +
> >> > +4:mov %bp, %sp
> >> > + pop %bp
> >> > + iret
> >> > +/* This label MUST be at the end of the copied block, since the
> >> > installer code
> >> > + reserves additional space for mappings at runtime and copies them
> >> > over it */
> >> > +.align 2
> >> > +VARIABLE(grub_drivemap_int13_mapstart)
> >> > +/* Copy stops here */
> >> > +.code32
> >> > +VARIABLE(grub_drivemap_int13_size)
> >> > + .word GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_size)
> >> > +
> >> > Index: conf/i386-pc.rmk
> >> > ===================================================================
> >> > --- conf/i386-pc.rmk (revisión: 1766)
> >> > +++ conf/i386-pc.rmk (copia de trabajo)
> >> > @@ -158,7 +158,7 @@
> >> > vbe.mod vbetest.mod vbeinfo.mod video.mod gfxterm.mod \
> >> > videotest.mod play.mod bitmap.mod tga.mod cpuid.mod serial.mod
> >> > \
> >> > ata.mod vga.mod memdisk.mod jpeg.mod png.mod pci.mod lspci.mod \
> >> > - aout.mod _bsd.mod bsd.mod
> >> > + aout.mod _bsd.mod bsd.mod drivemap.mod
> >> >
> >> > # For biosdisk.mod.
> >> > biosdisk_mod_SOURCES = disk/i386/pc/biosdisk.c
> >> > @@ -325,4 +325,11 @@
> >> > bsd_mod_CFLAGS = $(COMMON_CFLAGS)
> >> > bsd_mod_LDFLAGS = $(COMMON_LDFLAGS)
> >> >
> >> > +# For drivemap.mod.
> >> > +drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \
> >> > + commands/i386/pc/drivemap_int13h.S
> >> > +drivemap_mod_ASFLAGS = $(COMMON_ASFLAGS)
> >> > +drivemap_mod_CFLAGS = $(COMMON_CFLAGS)
> >> > +drivemap_mod_LDFLAGS = $(COMMON_LDFLAGS)
> >> > +
> >> > include $(srcdir)/conf/common.mk
> >> > Index: include/grub/loader.h
> >> > ===================================================================
> >> > --- include/grub/loader.h (revisión: 1766)
> >> > +++ include/grub/loader.h (copia de trabajo)
> >> > @@ -37,6 +37,22 @@
> >> > /* Unset current loader, if any. */
> >> > void EXPORT_FUNC(grub_loader_unset) (void);
> >> >
> >> > +typedef struct hooklist_node *grub_preboot_hookid;
> >> > +
> >> > +/* Register a function to be called before the boot hook. Returns an
> >> > id that
> >> > + can be later used to unregister the preboot (i.e. on module unload).
> >> > If
> >> > + abort_on_error is set, the boot sequence will abort if any of the
> >> > registered
> >> > + functions return anything else than GRUB_ERR_NONE.
> >> > + On error, the return value will compare equal to 0 and the error
> >> > information
> >> > + will be available in errno and errmsg. However, if the call is
> >> > successful
> >> > + those variables are _not_ modified. */
> >>
> >> No need to mention errmsg, it's internal to GRUB. As for errno (which
> >> is grub_errno, actually) it does not need to be mentioned, otherwise
> >> we would have to do so everywhere. Please capitalize HOOK and
> >> ABORT_ON_ERROR in the comments above.
> > Done. "hook" removed because it referred to the loader module boot
> > function.
> >>
> >> > +grub_preboot_hookid EXPORT_FUNC(grub_loader_register_preboot)
> >> > + (grub_err_t (*hook) (void), int abort_on_error);
> >> > +
> >> > +/* Unregister a preboot hook by the id returned by
> >> > loader_register_preboot.
> >> > + This functions becomes a no-op if no such function is registered */
> >> > +void EXPORT_FUNC(grub_loader_unregister_preboot) (grub_preboot_hookid
> >> > id);
> >> > +
> >> > /* Call the boot hook in current loader. This may or may not return,
> >> > depending on the setting by grub_loader_set. */
> >>
> >> Nitpick: "loader. This..."
> > Are you a bot? ¬¬ Corrected
>
> It would make like much simpler if I were ;-). What makes you think
> so?
Your ability to spot these kind of smallish things, like the "deltion" word and
the lack of a double space. You even write normal text with double spaces!
>
> >> > grub_err_t EXPORT_FUNC(grub_loader_boot) (void);
> >> > Index: kern/loader.c
> >> > ===================================================================
> >> > --- kern/loader.c (revisión: 1766)
> >> > +++ kern/loader.c (copia de trabajo)
> >> > @@ -61,11 +61,82 @@
> >> > grub_loader_loaded = 0;
> >> > }
> >> >
> >> > +struct hooklist_node
> >> > +{
> >> > + grub_err_t (*hook) (void);
> >> > + int abort_on_error;
> >> > + struct hooklist_node *next;
> >> > +};
> >> > +
> >> > +static struct hooklist_node *preboot_hooks = 0;
> >> > +
> >> > +grub_preboot_hookid
> >> > +grub_loader_register_preboot(grub_err_t (*hook) (void), int
> >> > abort_on_error)
> >> > +{
> >> > + if (!hook)
> >> > + {
> >> > + grub_error (GRUB_ERR_BAD_ARGUMENT, "preboot hook must not be
> >> > NULL");
> >> > + return 0;
> >> > + }
> >> > + grub_preboot_hookid newentry = grub_malloc (sizeof (struct
> >> > hooklist_node));
> >>
> >> Mixed declarations/code.
> > Oops, sorry. I put most of my attention on drivemap.c (and even then
> > many comments slipped through). Corrected.
>
> Please re-check them, I might have missed things this time...
>
> >> > + if (!newentry)
> >> > + {
> >> > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot alloc a hookinfo
> >> > structure");
> >> > + return 0;
> >> > + }
> >> > + else
> >> > + {
> >> > + newentry->hook = hook;
> >> > + newentry->abort_on_error = abort_on_error;
> >> > + newentry->next = preboot_hooks;
> >> > + preboot_hooks = newentry;
> >> > + return newentry;
> >> > + }
> >> > +}
> >> > +
> >> > +void
> >> > +grub_loader_unregister_preboot(grub_preboot_hookid id)
> >>
> >> "preboot (grub"
> > Corrected on both functions ;)
> >>
> >> > +{
> >> > + grub_preboot_hookid entry = 0;
> >> > + grub_preboot_hookid search = preboot_hooks;
> >> > + grub_preboot_hookid previous = 0;
> >> > +
> >> > + if (0 == id)
> >> > + return;
> >>
> >> ...
> > ... xD
> >>
> >> > + while (search)
> >> > + {
> >> > + if (search == id)
> >> > + {
> >> > + entry = search;
> >> > + break;
> >> > + }
> >> > + previous = search;
> >> > + search = search->next;
> >> > + }
> >> > + if (entry) /* Found */
> >>
> >> ...
> > Comment removed, was unnecessary.
> >>
> >> > + {
> >> > + if (previous)
> >> > + previous->next = entry->next;
> >> > + else preboot_hooks = entry->next; /* Entry was head of list */
> >> > + grub_free (entry);
> >> > + }
> >> > +}
> >> > +
> >> > grub_err_t
> >> > grub_loader_boot (void)
> >> > {
> >> > if (! grub_loader_loaded)
> >> > return grub_error (GRUB_ERR_NO_KERNEL, "no loaded kernel");
> >> > +
> >> > + grub_preboot_hookid entry = preboot_hooks;
> >>
> >> Mixed declarations/code.
> > Moved the whole line up.
> >>
> >> > + while (entry)
> >> > + {
> >> > + grub_err_t possible_error = entry->hook();
> >> > + if (possible_error != GRUB_ERR_NONE && entry->abort_on_error)
> >> > + return possible_error;
> >> > + entry = entry->next;
> >> > + }
> >> >
> >> > if (grub_loader_noreturn)
> >> > grub_machine_fini ();
>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
El mar, 05-08-2008 a las 13:28 +0200, Marco Gerards escribió:
> Hi,
>
> Javier Martín <address@hidden> writes:
>
> >> > There is, however, one point in which I keep my objection: comparisons
> >> > between a variable and a constant should be of the form CONSTANT ==
> >> > variable and not in the reverse order, since an erroneous but quite
> >> > possible change of == for = results in a compile-time error instead of a
> >> > _extremely_ difficult to trace runtime bug. Such kind of bugs are quite
> >> > excruciating to find in userspace applications within an IDE, so I can't
> >> > even consider the pain to debug them in a bootloader.
> >>
> >> I understand your concern, nevertheless, can you please change it?
> > You understand my concern, but seemingly do not understand that in order
> > to conform to the Holy Coding Style you are asking me to write code that
> > can become buggy (and with a very hard to trace bug) with a simple
> > deltion! (point: did you notice that last word _without_ a spelling
> > checker? Now try to do so in a multithousand-line program).
>
> BTW, your patch still contains this, can you please change it before I
> go over it again?
>
> I know people who claim that this code will become buggy because we
> write it in C. Should we start accepting patches to rewrite GRUB in
> Haskell or whatever? :-)
What about Ada? The stock GCC has Ada support ^^
>
> Really, as a maintainer I should set some standards and stick to it.
> Of course not everyone will like me and sometimes I have to act like a
> jerk. But I rather be a jerk, than committing code that do not meet
> my expectations. But please understand, this contribution is highly
> appreciated. However, we want to have something maintainable for the
> far future as well :-)
I understand these kind of concerns, particularly seeing how GRUB Legacy
ended - tangled, unscalable spaghetti code. You're not a jerk, just a
bit obsessive, but that's fine when trying to handle herds of us
all-important devs which think all we do is The Right Thing (tm) and
others are heretics to The Truth.
>
> --
> Marco
>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
Ok, so here is the new version of the patch, with a new drivemap.h
header and the == arguments put in the Holy Ordering ^^. I hope I didn't
forget anything this time. Here is the new CL entry:
2008-08-XX Javier Martin <address@hidden>
* commands/i386/pc/drivemap.c: New file.
* commands/i386/pc/drivemap_int13h.S: New file.
* conf/i386-pc.rmk (pkglib_MODULES): Added drivemap.mod.
(drivemap_mod_SOURCES): New variable.
(drivemap_mod_ASFLAGS): Likewise.
(drivemap_mod_CFLAGS): Likewise.
(drivemap_mod_LDFLAGS): Likewise.
* include/grub/loader.h (grub_loader_register_preboot): New
function prototype.
(grub_loader_unregister_preboot): Likewise.
(grub_preboot_hookid): New typedef.
* kern/loader.c (grub_loader_register_preboot): New function.
(grub_loader_unregister_preboot): Likewise.
(preboot_hooks): New variable.
(grub_loader_boot): Call the list of preboot-hooks before the
actual loader.
By the way, is there anything I can do to make `svn up' updates less
traumatic? I don't want to search each "C" file for "<<<<<< mine" lines
and correct them: is there any tool to do this with a workflow not
unlike that of Gentoo's `etc-update'?
Habbit
drivemap.patch.6
Description: Text Data
signature.asc
Description: Esta parte del mensaje está firmada digitalmente
- Re: [PATCH] Drivemap module, Javier Martín, 2008/08/03
- Re: [PATCH] Drivemap module, Marco Gerards, 2008/08/05
- Re: [PATCH] Drivemap module,
Javier Martín <=
- Re: [PATCH] Drivemap module, Javier Martín, 2008/08/09
- Re: [PATCH] Drivemap module, Marco Gerards, 2008/08/13
- Re: [PATCH] Drivemap module, Javier Martín, 2008/08/13
- Re: [PATCH] Drivemap module, Robert Millan, 2008/08/13
- Re: [PATCH] Drivemap module, Javier Martín, 2008/08/13
- Re: [PATCH] Drivemap module, Marco Gerards, 2008/08/13
- Re: [PATCH] Drivemap module, Robert Millan, 2008/08/13
- Re: [PATCH] Drivemap module, Marco Gerards, 2008/08/13
- Re: [PATCH] Drivemap module, Javier Martín, 2008/08/13
- Re: [PATCH] Drivemap module, Marco Gerards, 2008/08/14