Hello, I had two clear oppositions which weren't resolved. I don't
believe that merge patches screwing up the pendin oppositions is a
good practice. The opposition about declaration is based on another
handlers how it is used accross grub. Opposition about calling
biosdisk is technically relevant. Or perhaps I should start committing
anything without discussion?
Here is a fix patch. Could I commit it even if there will be oppositions?
On Thu, May 14, 2009 at 3:51 AM, Pavel Roskin <[email protected]> wrote:
> On Sat, 2009-05-09 at 17:42 +0200, Javier Martín wrote:
>
>> OK, I have a good feeling about this version of the patch. Most
>> importantly, it still works!!
>
> I have committed your patch after a cleanup. My changes were following:
>
> grub_drivemap_int13_handler_base and grub_drivemap_int13_handler have
> been merged.
>
> grub_drivemap_int13_oldhandler points directly to the ljmp argument
> (other GRUB sources do it too, jump look for 0xea).
>
> %si is not used anymore.
>
> Tabs are used consistently in drivemap_int13h.S
>
> Many variables have been renamed.
>
> Constants have been brought to the top level and marked as such.
>
> Logic in uninstall_int13_handler() has been fixed.
>
Which logic fix?
Other than variable rename it seems to be identical to Javier Martín's patch
> Some messages have been clarified. In particularly, biosdisk is called
> osdisk now, as it's what the OS sees.
> I was thinking of "payload" or
> "loadee" as more generic terms, but it can be confusing to the users.
>
> My check for drivemap with no arguments was wrong, fixed now.
>
> Added missing line ends in all outputs.
>
> Removed INT13H_REBASE and INT13H_TONEWADDR, as they were used only once.
>
> Comments have been improved.
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> [email protected]
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
--
Regards
Vladimir 'phcoder' Serbinenko
Index: commands/i386/pc/drivemap.c
===================================================================
--- commands/i386/pc/drivemap.c (revision 2213)
+++ commands/i386/pc/drivemap.c (working copy)
@@ -55,10 +55,10 @@
/* The assembly function to replace the old INT13h handler. It does not follow
any C callspecs and returns with IRET. */
-extern const void grub_drivemap_handler;
+extern const char grub_drivemap_handler[];
/* Start of the drive mappings area (space reserved at runtime). */
-extern const void grub_drivemap_mapstart;
+extern const char grub_drivemap_mapstart[];
typedef struct drivemap_node
{
@@ -74,7 +74,7 @@
} int13map_node_t;
#define INT13H_OFFSET(x) \
- (((grub_uint8_t *)(x)) - ((grub_uint8_t *)&grub_drivemap_handler))
+ ((x) - (grub_drivemap_handler))
static drivemap_node_t *map_head;
static void *drivemap_hook;
@@ -144,31 +144,6 @@
}
}
-/* Given a device name, resolves its BIOS disk number and stores it in the
- passed location, which should only be trusted if ERR_NONE is returned. */
-static grub_err_t
-parse_biosdisk (const char *name, grub_uint8_t *disknum)
-{
- grub_disk_t disk;
- /* Skip the first ( in (hd0) - disk_open wants just the name. */
- if (*name == '(')
- name++;
-
- disk = grub_disk_open (name);
- if (! disk)
- return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"",
- name);
- const enum grub_disk_dev_id id = disk->dev->id;
- /* The following assignment is only sound if the device is indeed a
- biosdisk. The caller must check the return value. */
- if (disknum)
- *disknum = disk->id;
- grub_disk_close (disk);
- if (id != GRUB_DISK_DEVICE_BIOSDISK_ID)
- return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk", name);
- return GRUB_ERR_NONE;
-}
-
/* Given a BIOS disk number, returns its GRUB device name if it exists.
If the call succeeds, the resulting device string must be freed.
For nonexisting BIOS disk numbers, this function returns
@@ -287,17 +262,14 @@
if (argc != 2)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
- err = parse_biosdisk (args[0], &mapfrom);
+ err = tryparse_diskstring (args[0], &mapfrom);
if (err != GRUB_ERR_NONE)
return err;
/* When swapping we require both devices to be BIOS disks, but when
performing direct mappings we only require the 2nd argument to look
like a BIOS disk in order to resolve it into a BIOS disk number. */
- if (cmd->state[OPTIDX_SWAP].set)
- err = parse_biosdisk (args[1], &mapto);
- else
- err = tryparse_diskstring (args[1], &mapto);
+ err = tryparse_diskstring (args[1], &mapto);
if (err != GRUB_ERR_NONE)
return err;
@@ -364,7 +336,7 @@
/* Find a rmode-segment-aligned zone in conventional memory big
enough to hold the handler and its data. */
- total_size = INT13H_OFFSET (&grub_drivemap_mapstart)
+ total_size = INT13H_OFFSET (grub_drivemap_mapstart)
+ (entries + 1) * sizeof (int13map_node_t);
grub_dprintf (MODNAME, "Payload is %u bytes long\n", total_size);
handler_base = grub_mmap_malign_and_register (16, total_size,
@@ -378,13 +350,13 @@
/* Copy int13h handler bundle to reserved area. */
grub_dprintf (MODNAME, "Reserved memory at %p, copying handler\n",
handler_base);
- grub_memcpy (handler_base, &grub_drivemap_handler,
- INT13H_OFFSET (&grub_drivemap_mapstart));
+ grub_memcpy (handler_base, grub_drivemap_handler,
+ INT13H_OFFSET (grub_drivemap_mapstart));
/* Copy the mappings to the reserved area. */
curentry = map_head;
handler_map = (int13map_node_t *) (handler_base +
- INT13H_OFFSET (&grub_drivemap_mapstart));
+ INT13H_OFFSET (grub_drivemap_mapstart));
grub_dprintf (MODNAME, "Target map at %p, copying mappings\n", handler_map);
for (i = 0; i < entries; ++i, curentry = curentry->next)
{
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
