Re: [PATCH] Drivemap module
Javier Martín [EMAIL PROTECTED] writes: Ok, making a mixup reply... Please don't. Unless you do not like it that your mails go unread. Having a small kernel is highly desireable for most users. If the kernel is too big, it won't fit and then either we have to use blocklists (which are unreliable), or we have to abort the install. Please, try to find a way that doesn't increase kernel size significantly. If the kernel interfaces are not extensible enough, you could try to readjust them for your needs. This approach works well most of the time (although I haven't studied this particular problem in detail). Like discussed before. Bring up such modifications like hooks up in a *separate* thread. I already said that not everyone reads this discussion. I will not accept a patch that changes the kernel if it is part of a bigger patch that not many people read. Please don't discuss this over with Robert and me, you know that it was pointed out that this has to be a patch in a separate thread. Furthermore, this is a way to get some feedback from Bean who wants something similar, IIRC. I know I will be regretting saying this, but it is _very_ rude to review some five versions of the patch, spotting mostly coding-style errors on each, and then, on version 8, tell me that you won't accept a patch that contains blah (with blah being essential for the patch to work). Quite the proverbial slap in the face to me. I did NOT say that your patch will not be accepted. In one of the earlier reviews (perhaps even the first) I mentioned that certain parts should be reviewed separately. It is a slap in the face that you imply I am a bad person when I repeat that you need to bring up some issues *separately*. I would consider it bad style from me towards other developers to change code they might have a strong opinion on. And you can say what you want, but if you are stubborn enough to ignore what I say in the reviews, you shouldn't be surprised if I didn't change my opinion in the next review and still ask you to change something. In fact, it is rude just to send in a new patch while you did not take the review seriously. There are a lot of projects that reject patches on beforehand if you didn't at least bother to get familiar with their coding styles. Furthermore, I reviewed your patch mainly because I care, not because I am the foremost expert in BIOS disks. It's for the best if other people can look at specific parts of your patch, especially if it changes the kernel. Perhaps you do not notice how many time I spent on reviewing patches on this list lately. Anyways, if it is not appreciated, I will leave the reviews to other people. But I simply will reject all code I do not like without actually bothering to explain why. I rather spend my time on coding instead of feeding trolls. -- Marco ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
El jue, 14-08-2008 a las 19:15 +0200, Marco Gerards escribió: Javier Martín [EMAIL PROTECTED] writes: Ok, making a mixup reply... Please don't. Unless you do not like it that your mails go unread. Having a small kernel is highly desireable for most users. If the kernel is too big, it won't fit and then either we have to use blocklists (which are unreliable), or we have to abort the install. Please, try to find a way that doesn't increase kernel size significantly. If the kernel interfaces are not extensible enough, you could try to readjust them for your needs. This approach works well most of the time (although I haven't studied this particular problem in detail). Like discussed before. Bring up such modifications like hooks up in a *separate* thread. I already said that not everyone reads this discussion. I will not accept a patch that changes the kernel if it is part of a bigger patch that not many people read. Please don't discuss this over with Robert and me, you know that it was pointed out that this has to be a patch in a separate thread. Furthermore, this is a way to get some feedback from Bean who wants something similar, IIRC. I know I will be regretting saying this, but it is _very_ rude to review some five versions of the patch, spotting mostly coding-style errors on each, and then, on version 8, tell me that you won't accept a patch that contains blah (with blah being essential for the patch to work). Quite the proverbial slap in the face to me. I did NOT say that your patch will not be accepted. In one of the earlier reviews (perhaps even the first) I mentioned that certain parts should be reviewed separately. It is a slap in the face that you imply I am a bad person when I repeat that you need to bring up some issues *separately*. I would consider it bad style from me towards other developers to change code they might have a strong opinion on. I don't imply you're a bad person or anything like that, sorry if it seemed so. By the way, I searched the whole thread of [PATCH] drivemap module (starting in july 4-20) and the only instances in which separate, separately, split or similar words are related to phrases like put the return on a separate line or split these declarations - even though I _do_ remember you telling me something like what you say. I have actually taken the time to re-read all three threads related to drivemap [0][1][2] and the main opposition (from Vesa) was due to what the boot command would do if one of the hooks with abort_on_error set failed and the abort_on_error bit itself. Nothing _fundamental_ about the hook system. The only other objection has surfaced recently (from Robert) and is about the size such system would add to kernel - which mostly depends on the implementation of the hook system (single variable vs. array vs. linked list, for example), not the interface. And you can say what you want, but if you are stubborn enough to ignore what I say in the reviews, you shouldn't be surprised if I didn't change my opinion in the next review and still ask you to change something. In fact, it is rude just to send in a new patch while you did not take the review seriously. There are a lot of projects that reject patches on beforehand if you didn't at least bother to get familiar with their coding styles. (kneels on the floor japanese-teen-style) That is true and completely my fault: I was stubbornly trying to convince you to change a big part of the coding style without thinking that conventions are there for a reason. I apologize for all the headaches I may have caused. Furthermore, I reviewed your patch mainly because I care, not because I am the foremost expert in BIOS disks. It's for the best if other people can look at specific parts of your patch, especially if it changes the kernel. True, but the list is public and we can't be blamed if others are not interested in parts of the patch. Besides, even though I've shown that the strong opinions of Vesa and Robert did not affect any fundamental part of the hook system, I _could_ split it from drivemap and put it up for discussion on a separate thread. Perhaps you do not notice how many time I spent on reviewing patches on this list lately. Anyways, if it is not appreciated, I will leave the reviews to other people. But I simply will reject all code I do not like without actually bothering to explain why. I rather spend my time on coding instead of feeding trolls. It _is_ appreciated: if it weren't for you, this patch would have been lost in the blue long ago, just because no-one would bother to look at it. Besides, if you don't feed the trolls (me?), I'll have to go back to Wikipedia, where my 42 sockpuppets have already been banned! ^^ Seriously, though, I don't mean to be trollish at all, nor to bludgeon others into submission - that's just politics... -Habbit [0] http
Re: [PATCH] Drivemap module
Javier Martín [EMAIL PROTECTED] writes:
In this reply-to-myself hoping to keep the thread continuity, I put
forth the new version 7 of the patch with the following changes:
- A new switch -s/--swap has been implemented, so that running
drivemap -s hd0 hd1 is equivalent to issuing the command twice with
the arguments normal, then reversed. There is one exception: if the
first mapping fails or the second drive does not exist, no mapping is
performed, whereas hand-performing the swap would have successfully
assigned BIOS disk #1 to hd0, then failed to assign bd#0 to non-existent
hd1.
- Raw BIOS disk number parsing has been removed: the syntax drivemap
hd1 0x80 is no longer legal. However, one can still map non-existent
BIOS disk numbers with drivemap hd0 hd42 for example and (more
controversially, maybe I'll add a check eventually) assign a floppy to
an HD and back.
Ah good :-)
The only file changed from the last patch (version 6) is drivemap.c:
the rest of it should be the same, so if anyone was reviewing it you can
seamlessly jump to version 7. In particular, the functional changes
are localized in the drivemap_cmd function proper, and there are
cosmetic changes elsewhere (spurious tabs removed, etc.).
You only forgot the changelog entry ;-)
-Habbit
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,433 @@
+/* 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/machine/drivemap.h
+#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},
+ {swap, 's', 0, perform both direct and reverse mappings, 0, 0},
+ {0, 0, 0, 0, 0, 0}
+};
+
+enum opt_idxs {
+ OPTIDX_LIST = 0,
+ OPTIDX_RESET,
+ OPTIDX_SWAP,
+};
+
+typedef struct drivemap_node
+{
+ grub_uint8_t newdrive;
+ grub_uint8_t redirto;
+ struct drivemap_node *next;
+} drivemap_node_t;
+
+static drivemap_node_t *drivemap;
+static grub_preboot_hookid insthandler_hook;
+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)
+{
+ 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)
+{
+ if (previous)
+previous-next = mapping-next;
+ else /* Entry was head of list. */
+drivemap = mapping-next;
+ grub_free
Re: [PATCH] Drivemap module
El mié, 13-08-2008 a las 12:13 +0200, Marco Gerards escribió:
Javier Martín [EMAIL PROTECTED] writes:
In this reply-to-myself hoping to keep the thread continuity, I put
forth the new version 7 of the patch with the following changes:
- A new switch -s/--swap has been implemented, so that running
drivemap -s hd0 hd1 is equivalent to issuing the command twice with
the arguments normal, then reversed. There is one exception: if the
first mapping fails or the second drive does not exist, no mapping is
performed, whereas hand-performing the swap would have successfully
assigned BIOS disk #1 to hd0, then failed to assign bd#0 to non-existent
hd1.
- Raw BIOS disk number parsing has been removed: the syntax drivemap
hd1 0x80 is no longer legal. However, one can still map non-existent
BIOS disk numbers with drivemap hd0 hd42 for example and (more
controversially, maybe I'll add a check eventually) assign a floppy to
an HD and back.
Ah good :-)
The only file changed from the last patch (version 6) is drivemap.c:
the rest of it should be the same, so if anyone was reviewing it you can
seamlessly jump to version 7. In particular, the functional changes
are localized in the drivemap_cmd function proper, and there are
cosmetic changes elsewhere (spurious tabs removed, etc.).
You only forgot the changelog entry ;-)
Oops, sorry... the only addition would be the new file drivemap.h, so
the final (I hope) version would be like this:
2008-08-13 Javier Martin [EMAIL PROTECTED]
* 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/i386/pc/drivemap.h: New file.
* 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.
WRT the code, all your concerns were taken care of (I keep forgetting
the . in comments, sorry), and about this snippet:
+static grub_err_t
+grub_cmd_drivemap (struct grub_arg_list *state, int argc, char **args)
+{
+ if (state[OPTIDX_LIST].set)
+{
+ /* Show: list mappings. */
+ if (!drivemap)
+grub_printf (No drives have been remapped);
+ else
+{
+ grub_printf (Showing only remapped drives.\n);
+ grub_printf (BIOS disk #num GRUB device\n);
BIOS disk #num?
Can you give an example?
The drivemap -l command lists all remapped drives. The format of the listing
has been reworked in order to be both understandable for end-users and
unambiguous for devs and power users. It is now something akin to:
BIOS disk #num GRUB device
FD #0 (0x00) (fd3)
(...)
FD #127 (0x7f) (hd15)
HD #0 (0x80) (hd2)
(...)
HD #127 (0xff) (fd0)
Of course, it only shows remapped drives, not the full 255 possibilities. Well,
here is version 8 of the patch... hope you like it ;)
-Habbit
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,434 @@
+/* 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/machine/drivemap.h
+#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},
+ {swap, 's', 0, perform both direct and reverse
Re: [PATCH] Drivemap module
Hi,
Marco asked me to review this. I haven't followed the earlier discussion,
so if I say or ask something that was discussed before, please bear with me
and just point me to that.
On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Martín wrote:
+
+#define MODNAME drivemap
+
[...]
+ grub_dprintf (MODNAME, Removing the mapping for %s (%02x),
args[0], mapfrom);
I don't think this MODNAME approach is a bad idea per se [1][2], but if we
are to do it, IMHO this should really be done globally for consistency, and
preferably separately from this patch.
[1] But I'd use a const char[] instead of a macro to save space. Maybe
significant space can be saved when doing this throurough the code!
[2] In fact, I think it's a nice idea.
+/* 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)
+{
Can this be made generic? Like install_int_handler (int n). We're probably
going to need interrupts for other things later on.
Or is this code suitable for i8086 mode interrupts only?
Anyway, if it's made generic, remember the handler itself becomes suitable for
all i386 ports, not just i386-pc (for directory placement).
+ drivemap_node_t *curentry = drivemap;
We use 'curr' as a handle for 'current' in lots of places throurough the code
(rgrep for curr[^e]). I think it's better to be consistent with that.
+/* Far pointer to the old handler. Stored as a CS:IP in the style of
real-mode
+ IVT entries (thus PI:SC in mem). */
+VARIABLE(grub_drivemap_int13_oldhandler)
+ .word 0xdead, 0xbeef
Is this a signature? Then a macro would be preferred, so that it can be shared
with whoever checks for it.
What is it used for, anyway? In general, I like to be careful when using
signatures because they introduce a non-deterministic factor (e.g. GRUB
might have a 1/64k possibility to missbehave).
+FUNCTION(grub_drivemap_int13_handler)
+ push %bp
+ mov %sp, %bp
+ push %ax /* We'll need it later to determine the used BIOS function. */
Please use size modifiers (pushw, movw, etc).
Index: include/grub/loader.h
===
--- include/grub/loader.h (revisión: 1802)
+++ include/grub/loader.h (copia de trabajo)
@@ -37,7 +37,23 @@
/* Unset current loader, if any. */
void EXPORT_FUNC(grub_loader_unset) (void);
-/* Call the boot hook in current loader. This may or may not return,
+typedef struct hooklist_node *grub_preboot_hookid;
+
+/* Register a function to be called before the loader boot function.
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 grub_errno. However, if the call is successful that
+ variable is _not_ modified. */
+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. */
grub_err_t EXPORT_FUNC(grub_loader_boot) (void);
This interface is added for all platforms. I didn't follow the discussion;
has it been considered that it will be useful elsewhere then i386-pc?
+/* 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;
I think this name is too generic for such an specific purpose.
Index: kern/loader.c
===
--- kern/loader.c (revisión: 1802)
+++ kern/loader.c (copia de trabajo)
@@ -61,11 +61,88 @@
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)
+{
[...]
This is a lot of code being added to kernel, and space in kernel is highly
valuable.
Would the same functionality work if put
Re: [PATCH] Drivemap module
On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Martín wrote: +static grub_err_t +revparse_biosdisk(const grub_uint8_t dnum, const char **output) Ah, and please separate function names from parenthesis ;-) -- Robert Millan The DRM opt-in fallacy: Your data belongs to us. We will decide when (and how) you may access your data; but nobody's threatening your freedom: we still allow you to remove your data and not access it at all. ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
El mié, 13-08-2008 a las 15:00 +0200, Robert Millan escribió:
Hi,
Marco asked me to review this.
So he finally got fed up of me... Understandable ^^
I haven't followed the earlier discussion,
so if I say or ask something that was discussed before, please bear with me
and just point me to that.
On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Martín wrote:
+
+#define MODNAME drivemap
+
[...]
+ grub_dprintf (MODNAME, Removing the mapping for %s (%02x),
args[0], mapfrom);
I don't think this MODNAME approach is a bad idea per se [1][2], but if we
are to do it, IMHO this should really be done globally for consistency, and
preferably separately from this patch.
[1] But I'd use a const char[] instead of a macro to save space. Maybe
significant space can be saved when doing this throurough the code!
[2] In fact, I think it's a nice idea.
Ok, so following your [1], what about replacing the define with... ?
static const char[] MODNAME = drivemap;
+/* 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)
+{
Can this be made generic? Like install_int_handler (int n). We're probably
going to need interrupts for other things later on.
Or is this code suitable for i8086 mode interrupts only?
Anyway, if it's made generic, remember the handler itself becomes suitable for
all i386 ports, not just i386-pc (for directory placement).
It _could_ be made generic, but the function as it is currently designed
installs a TSR-like assembly routine (more properly a bundle formed by a
routine and its data) in conventional memory that it has previously
reserved. Furthermore, it accesses the real-mode IVT at its standard
location of 0, which could be a weakness since from the 286 on even the
realmode IVT can be relocated with lidt.
Nevertheless, I don't think this functionality is so badly needed on its
own that it would be good to delay the implementation of drivemap to
wait for the re-engineering of this function.
+ drivemap_node_t *curentry = drivemap;
We use 'curr' as a handle for 'current' in lots of places throurough the code
(rgrep for curr[^e]). I think it's better to be consistent with that.
Done.
+/* Far pointer to the old handler. Stored as a CS:IP in the style of
real-mode
+ IVT entries (thus PI:SC in mem). */
+VARIABLE(grub_drivemap_int13_oldhandler)
+ .word 0xdead, 0xbeef
Is this a signature? Then a macro would be preferred, so that it can be
shared
with whoever checks for it.
What is it used for, anyway? In general, I like to be careful when using
signatures because they introduce a non-deterministic factor (e.g. GRUB
might have a 1/64k possibility to missbehave).
Sorry, it was a leftover from early development, in which I had to debug
the installing code to see whether the pointer to the old int13 was
installer: a pointer of beef:dead was a clue that it didn't work.
Removed and replaced with 32 bits of zeros.
+FUNCTION(grub_drivemap_int13_handler)
+ push %bp
+ mov %sp, %bp
+ push %ax /* We'll need it later to determine the used BIOS function. */
Please use size modifiers (pushw, movw, etc).
What for? the operands are clearly unambiguous. As you can see with the
xchgw %ax, -4(%bp) line, I only use them for disambiguation. Assembly
language is cluttered enough - and ATT syntax is twisted enough as it
is. In fact, given that the code is specific for i386, I'd like to
rewrite this code in GAS-Intel syntax so that memory references are not
insane: -4(%bp)? please... we can have a simpler [bp - 4].
Index: include/grub/loader.h
===
--- include/grub/loader.h (revisión: 1802)
+++ include/grub/loader.h (copia de trabajo)
@@ -37,7 +37,23 @@
/* Unset current loader, if any. */
void EXPORT_FUNC(grub_loader_unset) (void);
-/* Call the boot hook in current loader. This may or may not return,
+typedef struct hooklist_node *grub_preboot_hookid;
+
+/* Register a function to be called before the loader boot function.
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 grub_errno. However, if the call is successful
that
+ variable is _not_ modified. */
Re: [PATCH] Drivemap module
Hi,
Javier Martín [EMAIL PROTECTED] writes:
El mié, 13-08-2008 a las 15:00 +0200, Robert Millan escribió:
Hi,
Marco asked me to review this.
So he finally got fed up of me... Understandable ^^
No, but I am not as qualified regarding the BIOS as Robert is, except
for general remarks ;-)
I haven't followed the earlier discussion,
so if I say or ask something that was discussed before, please bear with me
and just point me to that.
On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Martín wrote:
+
+#define MODNAME drivemap
+
[...]
+ grub_dprintf (MODNAME, Removing the mapping for %s (%02x),
args[0], mapfrom);
I don't think this MODNAME approach is a bad idea per se [1][2], but if we
are to do it, IMHO this should really be done globally for consistency, and
preferably separately from this patch.
[1] But I'd use a const char[] instead of a macro to save space. Maybe
significant space can be saved when doing this throurough the code!
[2] In fact, I think it's a nice idea.
Ok, so following your [1], what about replacing the define with... ?
static const char[] MODNAME = drivemap;
static const char[] modname = drivemap;
We don't capatilize variables ;)
+/* 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)
+{
Can this be made generic? Like install_int_handler (int n). We're
probably
going to need interrupts for other things later on.
Or is this code suitable for i8086 mode interrupts only?
Anyway, if it's made generic, remember the handler itself becomes suitable
for
all i386 ports, not just i386-pc (for directory placement).
It _could_ be made generic, but the function as it is currently designed
installs a TSR-like assembly routine (more properly a bundle formed by a
routine and its data) in conventional memory that it has previously
reserved. Furthermore, it accesses the real-mode IVT at its standard
location of 0, which could be a weakness since from the 286 on even the
realmode IVT can be relocated with lidt.
Nevertheless, I don't think this functionality is so badly needed on its
own that it would be good to delay the implementation of drivemap to
wait for the re-engineering of this function.
This can go in and we can change it, I think.
+ drivemap_node_t *curentry = drivemap;
We use 'curr' as a handle for 'current' in lots of places throurough the code
(rgrep for curr[^e]). I think it's better to be consistent with that.
Done.
+/* Far pointer to the old handler. Stored as a CS:IP in the style of
real-mode
+ IVT entries (thus PI:SC in mem). */
+VARIABLE(grub_drivemap_int13_oldhandler)
+ .word 0xdead, 0xbeef
Is this a signature? Then a macro would be preferred, so that it can be
shared
with whoever checks for it.
What is it used for, anyway? In general, I like to be careful when using
signatures because they introduce a non-deterministic factor (e.g. GRUB
might have a 1/64k possibility to missbehave).
Sorry, it was a leftover from early development, in which I had to debug
the installing code to see whether the pointer to the old int13 was
installer: a pointer of beef:dead was a clue that it didn't work.
Removed and replaced with 32 bits of zeros.
+FUNCTION(grub_drivemap_int13_handler)
+ push %bp
+ mov %sp, %bp
+ push %ax /* We'll need it later to determine the used BIOS function.
*/
Please use size modifiers (pushw, movw, etc).
What for? the operands are clearly unambiguous. As you can see with the
xchgw %ax, -4(%bp) line, I only use them for disambiguation. Assembly
language is cluttered enough - and ATT syntax is twisted enough as it
is. In fact, given that the code is specific for i386, I'd like to
rewrite this code in GAS-Intel syntax so that memory references are not
insane: -4(%bp)? please... we can have a simpler [bp - 4].
I'll leave that to Robert :-)
Index: include/grub/loader.h
===
--- include/grub/loader.h (revisión: 1802)
+++ include/grub/loader.h (copia de trabajo)
@@ -37,7 +37,23 @@
/* Unset current loader, if any. */
void EXPORT_FUNC(grub_loader_unset) (void);
-/* Call the boot hook in current loader. This may or may not return,
+typedef struct hooklist_node *grub_preboot_hookid;
+
+/* Register a function to be called before the loader boot function.
Returns
+ an id that can be later used to unregister the preboot (i.e. on module
+ unload). If ABORT_ON_ERROR is set,
Re: [PATCH] Drivemap module
On Wed, Aug 13, 2008 at 04:28:24PM +0200, Javier Martín wrote: I don't think this MODNAME approach is a bad idea per se [1][2], but if we are to do it, IMHO this should really be done globally for consistency, and preferably separately from this patch. [1] But I'd use a const char[] instead of a macro to save space. Maybe significant space can be saved when doing this throurough the code! [2] In fact, I think it's a nice idea. Ok, so following your [1], what about replacing the define with... ? static const char[] MODNAME = drivemap; Yes, but I'd merge this change separately from your drivemap patch (either before or after, as you prefer), for the whole of the codebase. Doesn't make sense to do it in some places but not in others, IMHO. It _could_ be made generic, but the function as it is currently designed installs a TSR-like assembly routine (more properly a bundle formed by a routine and its data) in conventional memory that it has previously reserved. Furthermore, it accesses the real-mode IVT at its standard location of 0, which could be a weakness since from the 286 on even the realmode IVT can be relocated with lidt. Nevertheless, I don't think this functionality is so badly needed on its own that it would be good to delay the implementation of drivemap to wait for the re-engineering of this function. Fair enough. The addr=0 assumption sounds troubling, though. Why not use sidt instead? +/* Far pointer to the old handler. Stored as a CS:IP in the style of real-mode + IVT entries (thus PI:SC in mem). */ +VARIABLE(grub_drivemap_int13_oldhandler) + .word 0xdead, 0xbeef Is this a signature? Then a macro would be preferred, so that it can be shared with whoever checks for it. What is it used for, anyway? In general, I like to be careful when using signatures because they introduce a non-deterministic factor (e.g. GRUB might have a 1/64k possibility to missbehave). Sorry, it was a leftover from early development, in which I had to debug the installing code to see whether the pointer to the old int13 was installer: a pointer of beef:dead was a clue that it didn't work. Removed and replaced with 32 bits of zeros. Ok. +FUNCTION(grub_drivemap_int13_handler) + push %bp + mov %sp, %bp + push %ax /* We'll need it later to determine the used BIOS function. */ Please use size modifiers (pushw, movw, etc). What for? the operands are clearly unambiguous. For consistency (I'm so predictable). We do the same everywhere else. Also, I think some versions of binutils reject instructions that don't have size qualifiers. And it's more readable for people used to gas (I know, it's also less readable for people used to tasm or so). This interface is added for all platforms. I didn't follow the discussion; has it been considered that it will be useful elsewhere then i386-pc? Most likely not, and the discussion on this particular piece of the code died out long time (months) ago without reaching any decision. It's a way I've found for a fully out-kernel module like drivemap to set a just-before-boot hook in order to install its int13 handler: installing it earlier could cause havoc with the biosdisk driver, as drives in GRUB would suddenly reverse, duplicate, disappear, etc. This solution is the lightest and most scalable I've found that does not introduce drivemap-specific code in the kernel, because this infrastructure could be used by other modules. [...] This is a lot of code being added to kernel, and space in kernel is highly valuable. Would the same functionality work if put inside a module? For the reasons discussed above in the loader.h snippet, I don't think so: the only lighter solution would be to just put a drivemap_hook variable that would be called before boot, but as I mentioned before, this solution can be employed by other modules as well. Besides (and I realize this is not a great defense) it's not _that_ much code: just a simple linked-list implementation with add and delete operations, and the iteration of it on loader_boot. I did not check how many bytes does this patch add by itself, but I can run some simulations (I totally _had_ to say that ^^) if you want. Having a small kernel is highly desireable for most users. If the kernel is too big, it won't fit and then either we have to use blocklists (which are unreliable), or we have to abort the install. Please, try to find a way that doesn't increase kernel size significantly. If the kernel interfaces are not extensible enough, you could try to readjust them for your needs. This approach works well most of the time (although I haven't studied this particular problem in detail). +static grub_err_t +revparse_biosdisk(const grub_uint8_t dnum, const char **output) Ah, and please separate function names from parenthesis ;-) Done. Do you and/or Marco perform any
Re: [PATCH] Drivemap module
Robert Millan [EMAIL PROTECTED] writes: [...] This is a lot of code being added to kernel, and space in kernel is highly valuable. Would the same functionality work if put inside a module? For the reasons discussed above in the loader.h snippet, I don't think so: the only lighter solution would be to just put a drivemap_hook variable that would be called before boot, but as I mentioned before, this solution can be employed by other modules as well. Besides (and I realize this is not a great defense) it's not _that_ much code: just a simple linked-list implementation with add and delete operations, and the iteration of it on loader_boot. I did not check how many bytes does this patch add by itself, but I can run some simulations (I totally _had_ to say that ^^) if you want. Having a small kernel is highly desireable for most users. If the kernel is too big, it won't fit and then either we have to use blocklists (which are unreliable), or we have to abort the install. Please, try to find a way that doesn't increase kernel size significantly. If the kernel interfaces are not extensible enough, you could try to readjust them for your needs. This approach works well most of the time (although I haven't studied this particular problem in detail). Like discussed before. Bring up such modifications like hooks up in a *separate* thread. I already said that not everyone reads this discussion. I will not accept a patch that changes the kernel if it is part of a bigger patch that not many people read. Please don't discuss this over with Robert and me, you know that it was pointed out that this has to be a patch in a separate thread. Furthermore, this is a way to get some feedback from Bean who wants something similar, IIRC. -- Marco ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
Ok, making a mixup reply... El mié, 13-08-2008 a las 17:14 +0200, Robert Millan escribió: On Wed, Aug 13, 2008 at 04:28:24PM +0200, Javier Martín wrote: I don't think this MODNAME approach is a bad idea per se [1][2], but if we are to do it, IMHO this should really be done globally for consistency, and preferably separately from this patch. [1] But I'd use a const char[] instead of a macro to save space. Maybe significant space can be saved when doing this throurough the code! [2] In fact, I think it's a nice idea. Ok, so following your [1], what about replacing the define with... ? static const char[] MODNAME = drivemap; Yes, but I'd merge this change separately from your drivemap patch (either before or after, as you prefer), for the whole of the codebase. Doesn't make sense to do it in some places but not in others, IMHO. Urgh... It's already been implemented in drivemap, why not put it in with it, then change everything else? Or should I just keep the #define in the meantime? It _could_ be made generic, but the function as it is currently designed installs a TSR-like assembly routine (more properly a bundle formed by a routine and its data) in conventional memory that it has previously reserved. Furthermore, it accesses the real-mode IVT at its standard location of 0, which could be a weakness since from the 286 on even the realmode IVT can be relocated with lidt. Nevertheless, I don't think this functionality is so badly needed on its own that it would be good to delay the implementation of drivemap to wait for the re-engineering of this function. Fair enough. The addr=0 assumption sounds troubling, though. Why not use sidt instead? Well, so is the assumption that GRUB does not enable any kind of paging or memory remapping for that matter. WRT sidt, I think that could be better implemented as a function in kern/i386/pc called grub_machine_get_rmove_ivt() or SLT, because it requires dropping to rmode, executing sidt, going back to pmode and then returning the address, modified to be valid in pmode as required. This approach would cost a few bytes in kernel (I can hear you screaming already), but it would be extensible for the future interrupt installer you envisioned, and it would take care of the paging/mappings if there ever were any. Same for a function to get the address of the BDA or other machine-specific addresses. +/* Far pointer to the old handler. Stored as a CS:IP in the style of real-mode + IVT entries (thus PI:SC in mem). */ +VARIABLE(grub_drivemap_int13_oldhandler) + .word 0xdead, 0xbeef Is this a signature? Then a macro would be preferred, so that it can be shared with whoever checks for it. What is it used for, anyway? In general, I like to be careful when using signatures because they introduce a non-deterministic factor (e.g. GRUB might have a 1/64k possibility to missbehave). Sorry, it was a leftover from early development, in which I had to debug the installing code to see whether the pointer to the old int13 was installer: a pointer of beef:dead was a clue that it didn't work. Removed and replaced with 32 bits of zeros. Ok. +FUNCTION(grub_drivemap_int13_handler) + push %bp + mov %sp, %bp + push %ax /* We'll need it later to determine the used BIOS function. */ Please use size modifiers (pushw, movw, etc). What for? the operands are clearly unambiguous. For consistency (I'm so predictable). We do the same everywhere else. Also, I think some versions of binutils reject instructions that don't have size qualifiers. Version 0.1 (alpha 2) maybe? IIRC, it's been long since the size qualifiers inference is available in GAS. And it's more readable for people used to gas (I know, it's also less readable for people used to tasm or so). TASM? Don't even name the devil. I like GAS and its directives (even better and NASM and YASM), but many of the conventions of the ATT syntax are at best broken (i.e. src, dest looks good, but breaks on FP instructions), and memory references are a royal PITA. Given that the code is _not_ platform-portable and that a PPC dev will not understand it even in ATT syntax, why bother with it... However, I take it that your advertised predictability means that I should consider the int13h in intel syntax request denied. Nevertheless, I've attached the two versions of the asm file, so you can check which one is less of a mind boggler. Both assemble to _exactly_ the same machine code (checked with gcc + objdump). El mié, 13-08-2008 a las 17:57 +0200, Marco Gerards escribió: Robert Millan [EMAIL PROTECTED] writes: [...] This is a lot of code being added to kernel, and space in kernel is highly valuable. Would the same functionality work if put inside a module? For the reasons discussed above in the loader.h snippet, I don't think
Re: [PATCH] Drivemap module
In this reply-to-myself hoping to keep the thread continuity, I put
forth the new version 7 of the patch with the following changes:
- A new switch -s/--swap has been implemented, so that running
drivemap -s hd0 hd1 is equivalent to issuing the command twice with
the arguments normal, then reversed. There is one exception: if the
first mapping fails or the second drive does not exist, no mapping is
performed, whereas hand-performing the swap would have successfully
assigned BIOS disk #1 to hd0, then failed to assign bd#0 to non-existent
hd1.
- Raw BIOS disk number parsing has been removed: the syntax drivemap
hd1 0x80 is no longer legal. However, one can still map non-existent
BIOS disk numbers with drivemap hd0 hd42 for example and (more
controversially, maybe I'll add a check eventually) assign a floppy to
an HD and back.
The only file changed from the last patch (version 6) is drivemap.c:
the rest of it should be the same, so if anyone was reviewing it you can
seamlessly jump to version 7. In particular, the functional changes
are localized in the drivemap_cmd function proper, and there are
cosmetic changes elsewhere (spurious tabs removed, etc.).
-Habbit
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,433 @@
+/* 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/machine/drivemap.h
+#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},
+ {swap, 's', 0, perform both direct and reverse mappings, 0, 0},
+ {0, 0, 0, 0, 0, 0}
+};
+
+enum opt_idxs {
+ OPTIDX_LIST = 0,
+ OPTIDX_RESET,
+ OPTIDX_SWAP,
+};
+
+typedef struct drivemap_node
+{
+ grub_uint8_t newdrive;
+ grub_uint8_t redirto;
+ struct drivemap_node *next;
+} drivemap_node_t;
+
+static drivemap_node_t *drivemap;
+static grub_preboot_hookid insthandler_hook;
+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)
+{
+ 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)
+{
+ 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)
+{
Re: [PATCH] Drivemap module
- Original Message From: Felix Zielcke [EMAIL PROTECTED] To: The development of GRUB 2 grub-devel@gnu.org Sent: Friday, 8 August, 2008 6:50:12 PM Subject: Re: [PATCH] Drivemap module Am Mittwoch, den 06.08.2008, 19:31 +0200 schrieb Javier Martín: This will work on Windows, because no matter where it tries to boot from it will find the same disk: both the first and second BIOS disks point to hd1 now. ... This is the setup I have extensively tested in QEMU, while the Windows boot has been tested in both QEMU (with ReactOS) and some random computers including mine (with Windows XP Home and Windows XP Pro x64). Even though I don't need your drivemap at all (see one of my replies to Marco's ATA topic) I was so kind to try this out with my Vista x64 SP1 ;) I haven't remembered exactly the 2 menuentrys you wrote in your mail so I used a mixture of both :) drivemap (hd1) (hd0) set root=(hd0) chainloader (hd1,1)+1 This booted fine for me, GRUB loaded from IDE disk and Vista chainloaded from my SATA RAID 0. I really hope that you get this commited, then there's one feature less missing from grub-legacy ;) Hi, Nice to know that the patch is working..for others also.meanwhile I am not too much sure,why the same is not happening to me. The only difference in this case is that , my linux is in sata and my windows is in IDE. Viswesh ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel Unlimited freedom, unlimited storage. Get it now, on http://help.yahoo.com/l/in/yahoo/mail/yahoomail/tools/tools-08.html/___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
From: Javier Martín [EMAIL PROTECTED] To: The development of GRUB 2 grub-devel@gnu.org Sent: Wednesday, 6 August, 2008 11:01:19 PM Subject: Re: [PATCH] Drivemap module El mié, 06-08-2008 a las 07:43 -0700, Viswesh S escribió: Hi, Could you let all know how to use your drivemap command.Is it similar to the map command in legacy grub. Hi there. Information about the use of a command is better placed in the wiki than in this list, however this command is not merged in yet, and I reckon its help feature could be quite better. Currently, the safe GRUB2 commands for booting from (hd1,1) is: drivemap (hd0) (hd1) drivemap (hd1) (hd0) set root=(hd0) chainloader (hd1,1)+1 boot Maybe an explanation of what drivemap does would put a bit of light here: * The BIOS hard disks are numbered from 0 to 128 (actually from 0x80 to 0xFF because the lower numbers are reserved for floppy disks). These numbers are used to select the target drive when issuing I/O requests through the BIOS routines (INT 13h) * When the BIOS loads a boot sector and transfers control to it, the DL register contains the boot disk number, i.e. the disk from which the bootsector was loaded. This allows the bootsector to load its OS from the same disk it was run, instead of having to probe every single disk in the system. * The chainloader command works like the BIOS: it loads a bootsector into memory and jumps to it. In this case, the value in DL corresponds to the disk that was set as root drive to GRUB. If no root drive is set, the OS receives 0xFF, which should be recognized as an impossible drive. Some OSes will just trust this and fail (i.e. FreeDOS) while others will try to boot from the first hard disk (0x80). * The drivemap command acts as the old TSRs from the DOS times: when the boot command is issued, it installs its own handler for INT 13h requests, which performs the requested mappings and then call the old (usually BIOS) handler with the changed parameters. Thus, an OS accessing the drive through the BIOS routines will see the drives moved, duplicated or whatever the mappings provided. Again: drivemap does NOT modify the live drive mappings within GRUB; its changes only affect the booted OS. The strange root=(hd0) line that appears to contradict the chainloader line is there because drivemap has no communication with the particular loader. If you set root to (hd1,1) and then issue chainloader and boot, the OS will receive 0x81 in DL because hd1 was the second hard drive when chainloader was issued (remember that drivemap doesn't act until boot). Thus, the target OS will try to boot from what _it_ sees as the second hard disk, which will now be the old hd0 - wrong. If the target OS does not need to access the old hd0 or it only uses the BIOS routines for the boot process (i.e. it later uses ATA drivers and will redetect everything from scratch), you can leave the second drivemap line out and use a more compact script like this: drivemap (hd0) (hd1) set root=(hd1,1) chainloader +1 boot This will work on Windows, because no matter where it tries to boot from it will find the same disk: both the first and second BIOS disks point to hd1 now. However, if you use a DOS-like system which uses the BIOS routines exclusively (i.e. FreeDOS) then your hd0 disk would have disappeared to it and you'd have D: to be a mirror of C:. In order to have hd0 as D:, hd1 as C: and everything working, you need the first script I posted, which makes the complete swap and then makes FreeDOS load from the first hard drive (i.e. hd1). This is the setup I have extensively tested in QEMU, while the Windows boot has been tested in both QEMU (with ReactOS) and some random computers including mine (with Windows XP Home and Windows XP Pro x64). I'm looking forward to streamline the drivemap syntax, so that the two drivemap lines can be fused into one like drivemap --swap (hd0) (hd1). However, given that it's not a bug and that GRUB is still in heavy development (and thus syntax changes are acceptable), I'd prefer to have the patch integrated as it is - so we have the functionality - and then modify what's needed - so we have it pretty. -Habbit Hi, I have tried the same and different combinations,but still the machine is going for reset after I execute the boot command. As nothing gets displayed also,I am not sure where exactly this is going wrong also. What all might be the details in which I might have to look on, if I have to boot windows using grub2 only. Viswesh Connect with friends all over the world. Get Yahoo! India Messenger at http://in.messenger.yahoo.com/?wm=n/___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
El mié, 06-08-2008 a las 07:43 -0700, Viswesh S escribió: Hi, Could you let all know how to use your drivemap command.Is it similar to the map command in legacy grub. Hi there. Information about the use of a command is better placed in the wiki than in this list, however this command is not merged in yet, and I reckon its help feature could be quite better. Currently, the safe GRUB2 commands for booting from (hd1,1) is: drivemap (hd0) (hd1) drivemap (hd1) (hd0) set root=(hd0) chainloader (hd1,1)+1 boot Maybe an explanation of what drivemap does would put a bit of light here: * The BIOS hard disks are numbered from 0 to 128 (actually from 0x80 to 0xFF because the lower numbers are reserved for floppy disks). These numbers are used to select the target drive when issuing I/O requests through the BIOS routines (INT 13h) * When the BIOS loads a boot sector and transfers control to it, the DL register contains the boot disk number, i.e. the disk from which the bootsector was loaded. This allows the bootsector to load its OS from the same disk it was run, instead of having to probe every single disk in the system. * The chainloader command works like the BIOS: it loads a bootsector into memory and jumps to it. In this case, the value in DL corresponds to the disk that was set as root drive to GRUB. If no root drive is set, the OS receives 0xFF, which should be recognized as an impossible drive. Some OSes will just trust this and fail (i.e. FreeDOS) while others will try to boot from the first hard disk (0x80). * The drivemap command acts as the old TSRs from the DOS times: when the boot command is issued, it installs its own handler for INT 13h requests, which performs the requested mappings and then call the old (usually BIOS) handler with the changed parameters. Thus, an OS accessing the drive through the BIOS routines will see the drives moved, duplicated or whatever the mappings provided. Again: drivemap does NOT modify the live drive mappings within GRUB; its changes only affect the booted OS. The strange root=(hd0) line that appears to contradict the chainloader line is there because drivemap has no communication with the particular loader. If you set root to (hd1,1) and then issue chainloader and boot, the OS will receive 0x81 in DL because hd1 was the second hard drive when chainloader was issued (remember that drivemap doesn't act until boot). Thus, the target OS will try to boot from what _it_ sees as the second hard disk, which will now be the old hd0 - wrong. If the target OS does not need to access the old hd0 or it only uses the BIOS routines for the boot process (i.e. it later uses ATA drivers and will redetect everything from scratch), you can leave the second drivemap line out and use a more compact script like this: drivemap (hd0) (hd1) set root=(hd1,1) chainloader +1 boot This will work on Windows, because no matter where it tries to boot from it will find the same disk: both the first and second BIOS disks point to hd1 now. However, if you use a DOS-like system which uses the BIOS routines exclusively (i.e. FreeDOS) then your hd0 disk would have disappeared to it and you'd have D: to be a mirror of C:. In order to have hd0 as D:, hd1 as C: and everything working, you need the first script I posted, which makes the complete swap and then makes FreeDOS load from the first hard drive (i.e. hd1). This is the setup I have extensively tested in QEMU, while the Windows boot has been tested in both QEMU (with ReactOS) and some random computers including mine (with Windows XP Home and Windows XP Pro x64). I'm looking forward to streamline the drivemap syntax, so that the two drivemap lines can be fused into one like drivemap --swap (hd0) (hd1). However, given that it's not a bug and that GRUB is still in heavy development (and thus syntax changes are acceptable), I'd prefer to have the patch integrated as it is - so we have the functionality - and then modify what's needed - so we have it pretty. -Habbit signature.asc Description: Esta parte del mensaje está firmada digitalmente ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
Hi Javier, Javier Martín [EMAIL PROTECTED] writes: El lun, 04-08-2008 a las 22:51 +0200, Marco Gerards escribió: Javier Martín [EMAIL PROTECTED] 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. - 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. 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 ;-) 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)
Re: [PATCH] Drivemap module
Hi, Javier Martín [EMAIL PROTECTED] 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? :-) 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 :-) -- Marco ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
Hi,
Javier Martín [EMAIL PROTECTED] 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.
--
Marco
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
On Tue, 05 Aug 2008 01:10:25 +0200
Javier Martín [EMAIL PROTECTED] wrote:
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.
Respect and heed the gcc warnings!!
Note that gcc will warn you (you *are* using -Wall, at least, right?)
if you try to do
if (a = b)
but if you really mean it, you should surround the expression with an
extra pair of parentheses:
if ((a = b))
Use of the value of an assignment in an expression is common in code
like
if ((n = read (fd, buf, sz)) 0)
/* ... use the data in buf ... */
Here's an example showing gcc being helpful:
$ cat a.c
int f()
{
int a = 1;
int b;
if ((b = a))
return 1;
return 0;
}
$ cat b.c
int f()
{
int a = 1;
int b;
if (b = a)
return 1;
return 0;
}
$ gcc -Wall -c a.c
$ gcc -Wall -c b.c
b.c: In function ‘f’:
b.c:5: warning: suggest parentheses around assignment used as truth
$
New version of the patch follows. I must say that the discussion here
has become pretty interesting without degrading into flamewarring -
who whats to call Mr. Torvalds and Prof. Tanembaum? ^^
Well, it sounds to me like you are trying your best to set Marco ablaze,
but he is delightfully fire-retardant.
Regards,
Colin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
On Tue, 05 Aug 2008 13:23:11 +0200 Marco Gerards [EMAIL PROTECTED] wrote: Hi Javier, Javier Martín [EMAIL PROTECTED] writes: 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 ;-) Since that will generate a gcc warning, I hope that the author would at least do if ((a = f ())) to indicate to gcc that you mean what you said. Regards, Colin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
Javier Martín [EMAIL PROTECTED] 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. - 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? 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) 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 [EMAIL PROTECTED] (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 `'? Please do not add a space before the : 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
Re: [PATCH] Drivemap module
Javier Martín wrote: 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). well, maybe a bit off topic. But I can't imagine how, after code is written, I could accidentally delete an = character even when editing it. I prefer the (to me) intuitive meaning of (variable == value) in my own code. That particular problem has never bitten me. Although, in C++ coding style, a lot more local variables are const and therefore the error could be caught by the compiler anyway. It seems like an odd paranoia to choose. Say, take uint32_t. It's only a one-character deletion to become int32_t and then there is subtle breakage. htons and many other functions with similar names and suffixes. Etc.? It's half C language and culture, and half inevitable in programming, IMHO. point[2]: I half did notice the typo (only half because I've trained myself not to be too distracted by people's spelling), and I'm generally more precise when looking at code (maybe). ;-) 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... -Isaac ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
Hi there and thanks for playing The Game!
El lun, 04-08-2008 a las 20:50 -0400, Isaac Dupree escribió:
Javier Martín wrote:
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).
well, maybe a bit off topic.
But I can't imagine how, after code is written, I could
accidentally delete an = character even when editing it.
I prefer the (to me) intuitive meaning of (variable ==
value) in my own code. That particular problem has never
bitten me. Although, in C++ coding style, a lot more local
variables are const and therefore the error could be
caught by the compiler anyway. It seems like an odd
paranoia to choose. Say, take uint32_t. It's only a
one-character deletion to become int32_t and then there is
subtle breakage. htons and many other functions with
similar names and suffixes. Etc.? It's half C language and
culture, and half inevitable in programming, IMHO.
Sigh... Maybe I'm just a bit paranoid because I have been bitten by
similar problems (not in ifs, but mainly in loop condition checks) and
they were painstakingly difficult to hunt down even in a good IDE like
Netbeans: I sometimes considered the possibility of my computer having
lost all sense of logic because I saw it singlestep into the wrong
branch until I noticed the = instead of ==. True, the possibilities of
causing such a change are very low.
point[2]: I half did notice the typo (only half because
I've trained myself not to be too distracted by people's
spelling), and I'm generally more precise when looking at
code (maybe). ;-)
So I thought, but even my own code dodges me at times, not to speak of
others' code...
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
Habbit
signature.asc
Description: Esta parte del mensaje está firmada digitalmente
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
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;
- 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
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.
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.
The new version of the patch is attached, and here is my suggested CLog:
2008-08-XX Javier Martin [EMAIL PROTECTED]
* 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
(grub_loader_unregister_preboot) : Likewise. Unregister handler
(grub_preboot_hookid) : New typedef. Registered hook handle
* kern/loader.c (grub_loader_register_preboot) : New function.
(grub_loader_unregister_preboot) : Likewise.
(preboot_hooks) : New variable. Linked list of preboot hooks
(grub_loader_boot) : Call the list of preboot-hooks before the
actual loader
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
Re: [PATCH] Drivemap module
Javier Martín [EMAIL PROTECTED] writes: El dom, 20-07-2008 a las 21:40 +0200, Marco Gerards escribió: Did you use code from other people or projects? No, as far as I can control my own mind: the assembly int13h handler is loosely based on that of GRUB Legacy, but heavily rewritten. All other code was written from scratch, even the crappy linked lists all over the place. Okay :-) Sorry I kept you waiting... again... For newcomers, full info on the patch is available on the list archives - it was proposed on June and its discussion deferred for two or three weeks because the developers were busy. I have copied the changelog entry from your other e-mail: * commands/i386/pc/drivemap.c : New file, main part of the new drivemap module allowing BIOS drive remapping not unlike the legacy map command. This allows to boot OSes with boot-time dependencies on the particular ordering of BIOS drives or trusting their own to be 0x80, like Windows XP, with non-standard boot configurations. New file. would be sufficient * commands/i386/pc/drivemap_int13h.S : New file, INT 13h handler for the drivemap module. Installed as a TSR routine by drivemap.c, performs the actual redirection of BIOS drives. Same here. Hmm... isn't the ChangeLog too spartan? I thought it should be a bit informative - what about a single sentence per file? * commands/i386/pc/drivemap.c : New file - drivemap command and int13h installer * commands/i386/pc/drivemap_int13h.S : New file, resident real mode BIOS int13 handler No, please just stick to New file.. The Changelog is used for changes only. If you add more information, it will be noise and it will make my job harder. * conf/i386-pc.rmk : Added the new module Please say which variables you added in this file. You can find some examples on how to do this in the ChangeLog 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 And now we're being uselessly verbose. IMHO, ChangeLog should be more about semantic changes in the code and less about literal changes - we have `svn diff' for those. You are wrong. It is your opinion that this should go into a changelog entry and I can understand that. However, this is simply how we do it ;) * include/grub/loader.h : Added a just-before-boot callback infrastructure used by drivemap.mod to install the INT13 handler only when the boot command has been issued. Please describe changes, not effects. So which prototypes and macros did you add? * include/grub/loader.h (grub_loader_register_preboot) : New function (proto). Register a new pre-boot handler (grub_loader_unregister_preboot) : Likewise. Unregister handler (grub_preboot_hookid) : New typedef. Registered hook handle * kern/loader.c : Implement the preboot-hook described Which functions did you change and how? Please describe actual changes. * kern/loader.c (grub_loader_register_preboot) : New function. (grub_loader_unregister_preboot) : Likewise. (preboot_hooks) : New variable. Linked list of preboot hooks (grub_loader_boot) : Call the list of preboot-hooks before the actual loader The header is missing, please include it. Also newlines between the files make it easier to read. What header? The drivemap module itself has no .h files. The only header I touch is loader.h, and is both in the ChangeLog entry and the patch. With header I meant the first line of the changelog entry that includes your name and e-mail address. Here follows a review. Sorry I kept you waiting for this long, this feature and your work is really appreciated! Perhaps I can spot some more problems after you fixed it and supplied an updated changelog entry. There are quite some comments, but please do not let this demotivate you, it is mainly coding style related :-) Well, thanks to all the time I had free, I have nearly finished Final Fantasy XII, so the wait was not soo bad ^^ :-) (...) + +/* Uncomment the following line to enable debugging output */ +/* #define DRIVEMAP_DEBUG */ + +#ifdef DRIVEMAP_DEBUG +# define DBG_PRINTF(...) grub_printf(__VA_ARGS__) +#else +# define DBG_PRINTF(...) +#endif Please use the grub_dprintf infrastructure. Done. I didn't even know it existed :S :-) +/* realmode far ptr = 2 * 16b */ +extern grub_uint32_t EXPORT_VAR (grub_drivemap_int13_oldhandler); +/* Size of the section to be copied */ +extern grub_uint16_t EXPORT_VAR (grub_drivemap_int13_size); + +/* NOT a typo - just need the symbol's address with symbol */ +typedef void grub_symbol_t; +extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_handler_base); +extern grub_symbol_t
Re: [PATCH] Drivemap module
On Mon, Jul 21, 2008 at 02:55:41AM +0200, Javier Martín wrote: Hmm... isn't the ChangeLog too spartan? I tend to agree with that. I don't mind as much, as I get used myself, but it's really a PITA sometimes. -- Robert Millan GPLv2 I know my rights; I want my phone call! DRM What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
El lun, 21-07-2008 a las 02:55 +0200, Javier Martín escribió:
(...)
Phew! That was long, even after removing some parts. Well, this is the
new version of the patch. Cheers!
As always, I'm so stupid that I left the patch out... Sigh. Here it is.
Habbit
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,393 @@
+/* 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;
+
+/* NOT a typo - just need the symbol's address with symbol. */
+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);
+
+/* Syms/vars/funcs exported from drivemap_int13h.S - end. */
+
+
+static grub_preboot_hookid insthandler_hook = 0;
+
+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;
+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)
+
+{
+ drivemap_node_t *mapping = 0;
+ drivemap_node_t *search = drivemap;
+ while (search)
+{
+ if (search-newdrive == newdrive)
+{
+ mapping = search;
+ break;
+}
+ search = search-next;
+}
+
+
+ if (mapping) /* There was a mapping already in place, modify it. */
+mapping-redirto = redirto;
+ else /* Create a new mapping and add it to the head of the list. */
+{
+ 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. */
+{
+ if (previous)
+previous-next = mapping-next;
+ else /* Entry was head of list. */
+drivemap = mapping-next;
+ grub_free (mapping);
+}
+}
+
+static grub_err_t
+parse_biosdisk (const char *name, grub_uint8_t *disknum)
+{
+ if (!name) return GRUB_ERR_BAD_ARGUMENT;
+ if (*name == '(')
+name++; /* Skip the first ( in (hd0) - disk_open wants just the name. */
+ grub_disk_t disk = grub_disk_open (name);
+ if (!disk)
+return GRUB_ERR_UNKNOWN_DEVICE;
+ else
+{
+ enum grub_disk_dev_id id = disk-dev-id;
+ if (disknum)
+*disknum = disk-id; /* Only sound if it's a biosdisk - check later. */
+ grub_disk_close (disk);
+ return (GRUB_DISK_DEVICE_BIOSDISK_ID != id) ?
+ GRUB_ERR_BAD_DEVICE : GRUB_ERR_NONE;
+}
+}
+
+static grub_err_t
+revparse_biosdisk(const grub_uint8_t dnum, const char **output)
+{
+
Re: [PATCH] Drivemap module
Javier Martín [EMAIL PROTECTED] writes:
Just an updated version of the patch that adds support for device-like
names instead of raw BIOS disk numbers, i.e. this is now supported:
grub drivemap (hd0) (hd1)
In addition to the already supported:
grub drivemap (hd0) 0x81
The effect is the same: the second BIOS hard drive will map to (hd0)
through the installed int13h routine. The new syntax does not require
the target device name to exist (hd1 need not exist in my example), and
the parsing is very simple: it accepts names like (fdN) and (hdN) with
and without parenthesis, and with N ranging from 0 to 127, thus allowing
the full 0x00-0xFF range even though most BIOS-probing OSes don't bother
going any further than fd7 and hd7 respectively.
Did you use code from other people or projects?
For newcomers, full info on the patch is available on the list archives
- it was proposed on June and its discussion deferred for two or three
weeks because the developers were busy.
I have copied the changelog entry from your other e-mail:
* commands/i386/pc/drivemap.c : New file, main part of the new
drivemap module allowing BIOS drive remapping not unlike the
legacy map command. This allows to boot OSes with boot-time
dependencies on the particular ordering of BIOS drives or
trusting their own to be 0x80, like Windows XP, with
non-standard boot configurations.
New file. would be sufficient
* commands/i386/pc/drivemap_int13h.S : New file, INT 13h handler
for the drivemap module. Installed as a TSR routine by
drivemap.c, performs the actual redirection of BIOS drives.
Same here.
* conf/i386-pc.rmk : Added the new module
Please say which variables you added in this file. You can find some
examples on how to do this in the ChangeLog file.
* include/grub/loader.h : Added a just-before-boot callback
infrastructure used by drivemap.mod to install the INT13 handler
only when the boot command has been issued.
Please describe changes, not effects. So which prototypes and macros
did you add?
* kern/loader.c : Implement the preboot-hook described
Which functions did you change and how? Please describe actual changes.
The header is missing, please include it. Also newlines between the
files make it easier to read.
Here follows a review. Sorry I kept you waiting for this long, this
feature and your work is really appreciated! Perhaps I can spot some
more problems after you fixed it and supplied an updated changelog
entry. There are quite some comments, but please do not let this
demotivate you, it is mainly coding style related :-)
Index: commands/i386/pc/drivemap.c
===
RCS file: commands/i386/pc/drivemap.c
diff -N commands/i386/pc/drivemap.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ commands/i386/pc/drivemap.c 2 Jul 2008 01:12:36 -
@@ -0,0 +1,391 @@
+/* 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
+
+/* Uncomment the following line to enable debugging output */
+/* #define DRIVEMAP_DEBUG */
+
+#ifdef DRIVEMAP_DEBUG
+# define DBG_PRINTF(...) grub_printf(__VA_ARGS__)
+#else
+# define DBG_PRINTF(...)
+#endif
Please use the grub_dprintf infrastructure.
+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}
+};
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - start */
Useless comment
+/* realmode far ptr = 2 * 16b */
+extern grub_uint32_t EXPORT_VAR (grub_drivemap_int13_oldhandler);
+/* Size of the section to be copied */
+extern grub_uint16_t EXPORT_VAR (grub_drivemap_int13_size);
+
+/* NOT a typo - just need the symbol's address with symbol */
+typedef void grub_symbol_t;
+extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_handler_base);
+extern grub_symbol_t EXPORT_VAR
Re: [PATCH] Drivemap module
El dom, 20-07-2008 a las 21:40 +0200, Marco Gerards escribió: Did you use code from other people or projects? No, as far as I can control my own mind: the assembly int13h handler is loosely based on that of GRUB Legacy, but heavily rewritten. All other code was written from scratch, even the crappy linked lists all over the place. For newcomers, full info on the patch is available on the list archives - it was proposed on June and its discussion deferred for two or three weeks because the developers were busy. I have copied the changelog entry from your other e-mail: * commands/i386/pc/drivemap.c : New file, main part of the new drivemap module allowing BIOS drive remapping not unlike the legacy map command. This allows to boot OSes with boot-time dependencies on the particular ordering of BIOS drives or trusting their own to be 0x80, like Windows XP, with non-standard boot configurations. New file. would be sufficient * commands/i386/pc/drivemap_int13h.S : New file, INT 13h handler for the drivemap module. Installed as a TSR routine by drivemap.c, performs the actual redirection of BIOS drives. Same here. Hmm... isn't the ChangeLog too spartan? I thought it should be a bit informative - what about a single sentence per file? * commands/i386/pc/drivemap.c : New file - drivemap command and int13h installer * commands/i386/pc/drivemap_int13h.S : New file, resident real mode BIOS int13 handler * conf/i386-pc.rmk : Added the new module Please say which variables you added in this file. You can find some examples on how to do this in the ChangeLog 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 And now we're being uselessly verbose. IMHO, ChangeLog should be more about semantic changes in the code and less about literal changes - we have `svn diff' for those. * include/grub/loader.h : Added a just-before-boot callback infrastructure used by drivemap.mod to install the INT13 handler only when the boot command has been issued. Please describe changes, not effects. So which prototypes and macros did you add? * include/grub/loader.h (grub_loader_register_preboot) : New function (proto). Register a new pre-boot handler (grub_loader_unregister_preboot) : Likewise. Unregister handler (grub_preboot_hookid) : New typedef. Registered hook handle * kern/loader.c : Implement the preboot-hook described Which functions did you change and how? Please describe actual changes. * kern/loader.c (grub_loader_register_preboot) : New function. (grub_loader_unregister_preboot) : Likewise. (preboot_hooks) : New variable. Linked list of preboot hooks (grub_loader_boot) : Call the list of preboot-hooks before the actual loader The header is missing, please include it. Also newlines between the files make it easier to read. What header? The drivemap module itself has no .h files. The only header I touch is loader.h, and is both in the ChangeLog entry and the patch. Here follows a review. Sorry I kept you waiting for this long, this feature and your work is really appreciated! Perhaps I can spot some more problems after you fixed it and supplied an updated changelog entry. There are quite some comments, but please do not let this demotivate you, it is mainly coding style related :-) Well, thanks to all the time I had free, I have nearly finished Final Fantasy XII, so the wait was not soo bad ^^ (...) + +/* Uncomment the following line to enable debugging output */ +/* #define DRIVEMAP_DEBUG */ + +#ifdef DRIVEMAP_DEBUG +# define DBG_PRINTF(...) grub_printf(__VA_ARGS__) +#else +# define DBG_PRINTF(...) +#endif Please use the grub_dprintf infrastructure. Done. I didn't even know it existed :S +/* realmode far ptr = 2 * 16b */ +extern grub_uint32_t EXPORT_VAR (grub_drivemap_int13_oldhandler); +/* Size of the section to be copied */ +extern grub_uint16_t EXPORT_VAR (grub_drivemap_int13_size); + +/* NOT a typo - just need the symbol's address with symbol */ +typedef void grub_symbol_t; +extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_handler_base); +extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_mapstart); Please export stuff in header files, that's the normal practise in this file as well, right? What's not a typo? EXPORT_* macros removed; seemingly they are no longer needed because all code is in the same module (initially the assembly was in kernel). What's not a typo is the definition of grub_symbol_t as void instead of something more sound to a C programmer, like void *. I don't really know how to explain it in the source, but
Re: [PATCH] Drivemap module
El sáb, 05-07-2008 a las 13:04 +0200, Marco Gerards escribió: Javier Martín [EMAIL PROTECTED] writes: Just an updated version of the patch that adds support for device-like names instead of raw BIOS disk numbers, i.e. this is now supported: grub drivemap (hd0) (hd1) In addition to the already supported: grub drivemap (hd0) 0x81 The effect is the same: the second BIOS hard drive will map to (hd0) through the installed int13h routine. The new syntax does not require the target device name to exist (hd1 need not exist in my example), and the parsing is very simple: it accepts names like (fdN) and (hdN) with and without parenthesis, and with N ranging from 0 to 127, thus allowing the full 0x00-0xFF range even though most BIOS-probing OSes don't bother going any further than fd7 and hd7 respectively. Great! Can you please send in a changelog entry? -- Marco What about this: * commands/i386/pc/drivemap.c : New file, main part of the new drivemap module allowing BIOS drive remapping not unlike the legacy map command. This allows to boot OSes with boot-time dependencies on the particular ordering of BIOS drives or trusting their own to be 0x80, like Windows XP, with non-standard boot configurations. * commands/i386/pc/drivemap_int13h.S : New file, INT 13h handler for the drivemap module. Installed as a TSR routine by drivemap.c, performs the actual redirection of BIOS drives. * conf/i386-pc.rmk : Added the new module * include/grub/loader.h : Added a just-before-boot callback infrastructure used by drivemap.mod to install the INT13 handler only when the boot command has been issued. * kern/loader.c : Implement the preboot-hook described By the way, I sent the data to the assign mail as you instructed me to, and I've been told I'll receive some GNU copyright assignment documents to sign-and-return in a few days. signature.asc Description: Esta parte del mensaje está firmada digitalmente ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
Javier Martín [EMAIL PROTECTED] writes: Just an updated version of the patch that adds support for device-like names instead of raw BIOS disk numbers, i.e. this is now supported: grub drivemap (hd0) (hd1) In addition to the already supported: grub drivemap (hd0) 0x81 The effect is the same: the second BIOS hard drive will map to (hd0) through the installed int13h routine. The new syntax does not require the target device name to exist (hd1 need not exist in my example), and the parsing is very simple: it accepts names like (fdN) and (hdN) with and without parenthesis, and with N ranging from 0 to 127, thus allowing the full 0x00-0xFF range even though most BIOS-probing OSes don't bother going any further than fd7 and hd7 respectively. Great! Can you please send in a changelog entry? -- Marco ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Drivemap module
Just an updated version of the patch that adds support for device-like
names instead of raw BIOS disk numbers, i.e. this is now supported:
grub drivemap (hd0) (hd1)
In addition to the already supported:
grub drivemap (hd0) 0x81
The effect is the same: the second BIOS hard drive will map to (hd0)
through the installed int13h routine. The new syntax does not require
the target device name to exist (hd1 need not exist in my example), and
the parsing is very simple: it accepts names like (fdN) and (hdN) with
and without parenthesis, and with N ranging from 0 to 127, thus allowing
the full 0x00-0xFF range even though most BIOS-probing OSes don't bother
going any further than fd7 and hd7 respectively.
For newcomers, full info on the patch is available on the list archives
- it was proposed on June and its discussion deferred for two or three
weeks because the developers were busy.
Index: commands/i386/pc/drivemap.c
===
RCS file: commands/i386/pc/drivemap.c
diff -N commands/i386/pc/drivemap.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ commands/i386/pc/drivemap.c 2 Jul 2008 01:12:36 -
@@ -0,0 +1,391 @@
+/* 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
+
+/* Uncomment the following line to enable debugging output */
+/* #define DRIVEMAP_DEBUG */
+
+#ifdef DRIVEMAP_DEBUG
+# define DBG_PRINTF(...) grub_printf(__VA_ARGS__)
+#else
+# define DBG_PRINTF(...)
+#endif
+
+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}
+};
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - start */
+
+/* realmode far ptr = 2 * 16b */
+extern grub_uint32_t EXPORT_VAR (grub_drivemap_int13_oldhandler);
+/* Size of the section to be copied */
+extern grub_uint16_t EXPORT_VAR (grub_drivemap_int13_size);
+
+/* NOT a typo - just need the symbol's address with symbol */
+typedef void grub_symbol_t;
+extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_handler_base);
+extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_mapstart);
+
+void EXPORT_FUNC (grub_drivemap_int13_handler) (void);
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - end */
+
+static grub_preboot_hookid insthandler_hook = 0;
+
+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;
+static grub_err_t install_int13_handler (void);
+
+static grub_err_t
+drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
+ /* Puts the specified mapping into the table, replacing an existing mapping
+ * for newdrive or adding a new one if required. */
+{
+ drivemap_node_t *mapping = 0, *search = drivemap;
+ while (search)
+{
+ if (search-newdrive == newdrive)
+{
+ mapping = search;
+ break;
+}
+ search = search-next;
+}
+
+ if (mapping) /* There was a mapping already in place, modify it */
+mapping-redirto = redirto;
+ else /* Create a new mapping and add it to the head of the list */
+{
+ 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;
+}
+
+static void
+drivemap_remove (grub_uint8_t newdrive)
+ /* Removes the mapping for newdrive from the table. If there is no mapping,
+ * then this function behaves like a no-op on the map. */
+{
+ drivemap_node_t *mapping = 0, *search = drivemap, *previous = 0;
+ while (search)
+{
+ if (search-newdrive == newdrive)
+{
+ mapping = search;
+ break;
+}
+ previous = search;
+ search = search-next;
+}
+ if (mapping) /* Found */
+{
+ if (previous)
+
Re: [PATCH] Drivemap module
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 ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
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.
Index: commands/i386/pc/drivemap.c
===
RCS file: commands/i386/pc/drivemap.c
diff -N commands/i386/pc/drivemap.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ commands/i386/pc/drivemap.c 12 Jun 2008 22:25:01 -
@@ -0,0 +1,351 @@
+/* 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
+
+/* Uncomment the following line to enable debugging output */
+/* #define DRIVEMAP_DEBUG */
+
+#ifdef DRIVEMAP_DEBUG
+# define DBG_PRINTF(...) grub_printf(__VA_ARGS__)
+#else
+# define DBG_PRINTF(...)
+#endif
+
+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}
+ };
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - start */
+
+/* realmode far ptr = 2 * 16b */
+extern grub_uint32_t EXPORT_VAR(grub_drivemap_int13_oldhandler);
+/* Size of the section to be copied */
+extern grub_uint16_t EXPORT_VAR(grub_drivemap_int13_size);
+
+/* NOT a typo - just need the symbol's address with symbol */
+typedef void grub_symbol_t;
+extern grub_symbol_t EXPORT_VAR(grub_drivemap_int13_handler_base);
+extern grub_symbol_t EXPORT_VAR(grub_drivemap_int13_mapstart);
+
+void EXPORT_FUNC(grub_drivemap_int13_handler)(void);
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - end */
+
+static grub_preboot_hookid insthandler_hook = 0;
+
+typedef struct drivemap_node
+{
+ grub_uint8_t newdrive;
+ grub_uint8_t redirto;
+ struct drivemap_node *next;
+} drivemap_node_t;
+
+static drivemap_node_t *drivemap =
Re: [PATCH] Drivemap module
On Fri, 13 Jun 2008 00:43:31 +0200 Javier Martín [EMAIL PROTECTED] wrote: 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. I think Pavel meant the lines added by the patch, i.e., beginning with a plus in the patch, not in the original source. Here's an illustration to help find and visualize the invisible trailing whitespace: [EMAIL PROTECTED] ~ $ perl -ne '/^\+.* +$/ and y/ \t/_~/ and print' /home/cdb/drivemap.patch +__else_if_(mapto_==_mapfrom)__/*_Reset_to_default_*/__ +__ +__/*_Save_the_pointer_to_the_old_int13h_handler_*/ +__ +VARIABLE(grub_drivemap_int13_handler_base)__ +__ +__push_%bp__ +___(grub_err_t_(*hook)_(void),_int_abort_on_error);_ +__ I highly recommend running indent --no-tabs on your source code, as long as it doesn't cause too many spurious changes to pre-existing code. This should take care of most formatting inconsistencies and frees you a little but from concerning yourself with trivial details. Colin signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
On Thu, 2008-06-12 at 15:58 -0700, Colin D Bennett wrote: I think Pavel meant the lines added by the patch, i.e., beginning with a plus in the patch, not in the original source. Yes, I meant added lines. There is a lot of code that needs reformatting, but the patch shouldn't add more to that. I highly recommend running indent --no-tabs Why no tabs? Tabs are heavily used in the code. GNU indent is supposed to default to GNU Coding Standards, and it uses tabs. on your source code, as long as it doesn't cause too many spurious changes to pre-existing code. It will. This should take care of most formatting inconsistencies and frees you a little but from concerning yourself with trivial details. We can use a script to re-indent everything. Then new patches could be easily run through the same script. Otherwise, it's not going to be simple. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
On Thu, 12 Jun 2008 21:00:21 -0400 Pavel Roskin [EMAIL PROTECTED] wrote: On Thu, 2008-06-12 at 15:58 -0700, Colin D Bennett wrote: I highly recommend running indent --no-tabs Why no tabs? Tabs are heavily used in the code. GNU indent is supposed to default to GNU Coding Standards, and it uses tabs. The video module code uses only spaces for indentation. Since I've been working mostly on the video module, so far, I was misled to believe that spaces are the preferred GRUB style. I checked all the C files in GRUB2 CVS and found that you are right, tabs are heavily used, except in the code that I have been focused on. The following C files do not contains any tabs: ./kern/i386/efi/init.c ./commands/videotest.c ./util/powerpc/ieee1275/misc.c ./util/i386/pc/misc.c ./util/i386/get_disk_name.c ./util/ieee1275/get_disk_name.c ./util/lvm.c ./video/video.c ./video/i386/pc/vbeutil.c ./video/i386/pc/vbefill.c ./video/i386/pc/vbeblit.c ./video/i386/pc/vbe.c ./video/readers/tga.c ./video/bitmap.c ./disk/ieee1275/nand.c on your source code, as long as it doesn't cause too many spurious changes to pre-existing code. It will. This depends on the file. Running format (with the same choice of tabs+spaces or spaces-only for indentation) on a GRUB source file usually results in minimal changes, in my experience. This should take care of most formatting inconsistencies and frees you a little but from concerning yourself with trivial details. We can use a script to re-indent everything. Then new patches could be easily run through the same script. Otherwise, it's not going to be simple. Interesting idea! Anyway, my point is not to get into the religious war of spaces vs. tabs in indentation, since I am comfortable with both, although I think that mixing spaces and tabs together (using TAB characters as a substitute for 8 spaces) negates the benefits of both approaches. But that's all I'll say on that. I will indent code that I work on with the same style that it already has, so that my patches are clean and contain, in general, only changes related to actual features that the patch adds. Colin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
Hi Javier, Javier Martín [EMAIL PROTECTED] writes: Since the last posts in the reimplementing the legacy map command thread seemingly died silently, I'm sending the improved version of the patch here. If the death of the thread was intentional, please just tell me (either nicely or rudely) and I'll be off. It's not intentional. This feature is appreciated! Please do this scare you away... The problem is that many developers are really busy at the moment (unfortunately). I expect to have more time to review patches in about 2-3 weeks. -- Marco ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Drivemap module
Since the last posts in the reimplementing the legacy map command
thread seemingly died silently, I'm sending the improved version of the
patch here. If the death of the thread was intentional, please just tell
me (either nicely or rudely) and I'll be off.
Recapitulating the other thread, this is a new module for x86-pc that
provides functionality similar to the legacy map command, i.e. allows
BIOS drives to be remapped and have the second HD appear as 0x80 (the
first), which is necessary because some OSes refuse to boot otherwise,
ignoring the boot drive passed to their loaders in DL (Windows XP and
other NTs); or can only boot with limited functionality (FreeDOS). The
syntax of this new command is similar to the old map, though the
second argument must be the target BIOS drive number instead of a GRUB
device, though a hack-patch parsing them would be simple.
Regarding the objections raised by Vesa, I followed most of the advices,
removing chatter in the source and completely separating drivemap from
the kernel. However, I've left the abort_on_error part of the new
preboot-hook interface intact, mostly because I don't know how to
replace it. As drivemap is the only module using it, the problem should
be harmless for now, but a solution should be devised - not by me,
though, finals are here (literally, I had one today).
I hope I didn't leave anything out this time. This code has been tested
on the following situations:
* Emulators: Bochs QEMU, booting FreeDOS in (hd1) with another fat
partition in (hd0) - works wonderfully, and even allows access to both
drives if they are crossmapped and a faux root=(hd0) is set.
* Real hardware: an Athlon64 (939) CPU on a Gigabyte motherboard,
booting Windows XP Pro x64 in (hd1) with Ubuntu in (hd0) - works too,
with no hacks required at all (by the way, booting XP was my reason to
do this).
Well, here it is. Have a nice day!
Habbit
Index: commands/i386/pc/drivemap.c
===
RCS file: commands/i386/pc/drivemap.c
diff -N commands/i386/pc/drivemap.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ commands/i386/pc/drivemap.c 10 Jun 2008 20:02:19 -
@@ -0,0 +1,348 @@
+/* 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
+
+/* Uncomment the following line to enable debugging output */
+/* #define DRIVEMAP_DEBUG */
+
+#ifdef DRIVEMAP_DEBUG
+# define DBG_PRINTF(...) grub_printf(__VA_ARGS__)
+#else
+# define DBG_PRINTF(...)
+#endif
+
+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}
+ };
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - start */
+
+/* realmode far ptr = 2 * 16b */
+extern grub_uint32_t EXPORT_VAR(grub_drivemap_int13_oldhandler);
+/* Size of the section to be copied */
+extern grub_uint16_t EXPORT_VAR(grub_drivemap_int13_size);
+
+/* NOT a typo - just need the symbol's address with symbol */
+typedef void grub_symbol_t;
+extern grub_symbol_t EXPORT_VAR(grub_drivemap_int13_handler_base);
+extern grub_symbol_t EXPORT_VAR(grub_drivemap_int13_mapstart);
+
+void EXPORT_FUNC(grub_drivemap_int13_handler)(void);
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - end */
+
+static grub_preboot_hookid insthandler_hook = 0;
+
+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;
+static grub_err_t drivemap_install_int13_handler(void);
+
+static grub_err_t
+drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
+ /* Puts the specified mapping into the table, replacing an existing mapping
+ * for newdrive or adding a new one if required. */
+{
+ drivemap_node_t *mapping = 0, *search = drivemap;
+ while (search)
+{
+ if (search-newdrive == newdrive)
+{
+ mapping = search;
+ break;
+}
+ search = search-next;
+}
+
+ if (mapping) /* There was a mapping already in
Re: [PATCH] Drivemap module
Javier Martín wrote: Since the last posts in the reimplementing the legacy map command thread seemingly died silently, I'm sending the improved version of the patch here. If the death of the thread was intentional, please just tell me (either nicely or rudely) and I'll be off. I'll answer this now and look at patch when I have better time. Just keep in mind that while we appreciate new feature and bug fixes, we do not have all the time on earth to review them. Please be patient especially if you touch something that can be critical for stability and functionality of the software. We do not want to have feature on the system that no-one knows how it works, so we prefer to have understanding of the feature at hand and then that is gained then it is much easier to comment the patch and then diagnose if it has problems. So please wait a bit, and lets see what kind of comments it gets. ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drivemap module
El mar, 10-06-2008 a las 23:23 +0300, Vesa Jääskeläinen escribió: Javier Martín wrote: Since the last posts in the reimplementing the legacy map command thread seemingly died silently, I'm sending the improved version of the patch here. If the death of the thread was intentional, please just tell me (either nicely or rudely) and I'll be off. I'll answer this now and look at patch when I have better time. Just keep in mind that while we appreciate new feature and bug fixes, we do not have all the time on earth to review them. Please be patient especially if you touch something that can be critical for stability and functionality of the software. We do not want to have feature on the system that no-one knows how it works, so we prefer to have understanding of the feature at hand and then that is gained then it is much easier to comment the patch and then diagnose if it has problems. So please wait a bit, and lets see what kind of comments it gets. 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. ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
