2009/5/9 Javier Martín
<address@hidden>
El sáb, 09-05-2009 a las 11:17 +0200, Vladimir 'phcoder' Serbinenko
escribió:
> +/* Realmode far ptr (2 * 16b) to the previous INT13h handler. */
> +extern grub_uint32_t grub_drivemap_int13_oldhandler;
> I prefer it to be 2 variables because of the way they interract so
> nobody would do something like (char *)
> grub_drivemap_int13_oldhandler;
Two variables? I don't understand. You want the CS and IP parts of the
far pointer separated?
Yes. Especially that in .S you declared it as two words. But it's not an absolute must
Why would anyone try to use it like you said? The
comment explicits that it is a realmode pointer, and as such it is not
usable as a linear pmode pointer.
> +/* The type "void" is used for imported assembly labels, takes no
> storage and
> + serves just to take the address with &label. Do NOT put void*
> here. */
> +/* Start of the handler bundle. */
> +extern void grub_drivemap_int13_handler_base;
> The common practice is to use declarations like
> extern char grub_drivemap_int13_handler_base[];
> it makes pointer arithmetics easy
That variable is used only twice in the code: one in a call to memcpy
and another one inside the macro INT13_OFFSET. It would still be so even
if it were changed to a char array, with the added risk that a char
array can be modified with almost nothing to gain (as casts would still
be needed).
The casts are needed if you declare it as char foo[];
If someone modifies an array he does it on purpose
In fact, I'm declaring all the labels as "const void" so
no-one tries to overwrite the "master" data instead of the deployed
data.
> +typedef struct drivemap_node
> +{
> + grub_uint8_t newdrive;
> + grub_uint8_t redirto;
> + struct drivemap_node *next;
> +} drivemap_node_t;
> Here you could reuse Bean's list functions
After examining the API, I think such a change would be too invasive for
a "mature" patch right now. However, once the patch is in and drivemap
is working for everyone, I can work on modifying it to use
<grub/list.h>. The biggest hurdle I see is that there is no way to
automatically maintain "key" uniqueness like the current methods do, so
an iteration over the list would be required.
As a side note/rant, <grub/list.h> desperately needs documentation
comments for other developers to be able to actually use it without
wondering if they're going into the darkness. In particular I need to
know what grub_list_insert does without delving into "list.c" (for
example: "if all tests fail, will the item be inserted last or not at
all?"). This rant also applies to other grub include files.
> It's not necessary to try to open the disk actually because for some
> reason biosdisk module may be not loaded (e.g. grub may use ata) and
> loading it may cause nasty effects (e.g. conflict between ata and
> biosdisk)
> Same applies for revparse_biosdisk. And if user wants to map to
> unexistant disk it's ok. So just use tryparse_diskstring
Hmm... this is a profound design issue: you are arguing that drivemap
should not care about actual BIOS disks, but just their drive numbers,
and let you map 0x86 to 0x80 even if you have no (hd6). I do not agree,
since the main use for GRUB is a bootloader, not a test platform (which
would be the only reason to perform that kind of mappings which are sure
to cause havoc). Thus, by default drivemap only lets you perform
mappings that will work - any stress tester is free to modify the code.
Regarding opening the disk, the biosdisk interface does not assure in
any way (and again there is almost no API documentation) that hd0 will
be 0x80, though it's the sensible thing to expect. However, expecting
sensible things from an unstable API prone to redesigns (like the
command API, for example)
Mapping something to non-existant drive makes the drive non-existant. I think it's fair
It's almost guaranteed that numbering of disks won't change
usually leads to nasty surprises later on, so
unless 1) the biosdisk interface so specifies and 2) hdN will _always_
refer to biosdisk and not another possible handler, I think the check is
a Good Thing (tm).
It's not if it may hang when you use ata.mod
> + if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
> change this to something like
> if (! ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd'))
> return ...
> It makes code nicer by avoiding unnecessary long else
I think the "else" you were talking about was one line long, but it was
not needed anyways and so I've removed it.
> + grub_errno = GRUB_ERR_NONE;
> No need to set grub_errno explicitely unless you want to ignore an
> error
I may have got it wrong, but I think that functions like strtoul only
_set_ errno if there _has_ been an error, but leave it unchanged if
there hasn't. As I want to check if the conversion failed, I need to
have the variable to check in a known pre-state.
If grub_errno is set at the begining of the function then either scripting or your code has a bug
> + unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
> + if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
> I think it's enough to just check the range
See above.
> + {
> + /* N not a number or out of range. */
> + goto fail;
> Since at fail you have just a return, just put a return here with
> informative error message
The function was rearranged so only the first goto is required now.
+fail:
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" "
+ "invalid: must be (f|h)dN, with 0 <= N < 128", str);
I don't see a clear benefit of using goto here. Perhaps providing a
separate informative messages for 2 fail cases is a better option
> + }
> + else
> else is unnecessary, just continue with the code
I fear you don't give me enough context to find this one. If it was
inside tryparse_diskstring, it has been removed.
> + if (cmd->state[OPTIDX_LIST].set || 0 == argc)
> argc == 0 sounds better. Also moving list function to a separate
> function is a good idea
Ok, changed. I'd just like to remark that if someone slipped and changed
the line to "argc = 0" it would pass unnoticed by the compiler, while "0
= argc" would create a compile-time error.
It's what warnings are for
Regarding slicing the listing
functionality, I don't see the advantages.
It makes code more readable and avoids long else clause
> + push %bp
> + mov %sp, %bp
> You don't need to change bp. Just use sp. In this case it makes code
> easier
That would make it more difficult to address passed parameters like the
flags at [BP+6] because their location would depend on the current
status of SP (which might change depending on code above the relevant
lines).
I checked and can say that removing bp code makes the rest actually easier. If you don't see how tell me I'll modify it
I think that trying to shave off those 4 or so bytes would make
the code unnecessarily more complicated, which is usually a no-no when
assembly is involved. Besides, this prolog sequence and the
corresponding epilog before returning is practically a standard in x86.
> + lcall *%
> cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
> + pop %bp /* The pushed flags were removed by iret. */
> Use rather:
> pushf
> lcall *%
> cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
Why would we want to save our current flags? We want the old int13
handler to receive the flags it would have received if the drivemap
int13 handler were not present, that is, the original caller flags which
were stored above CS:IP in the stack when the "int $0x13" call was
issued. The comment means that "iret" pops not just CS:IP but also the
flags, so we can proceed directly to popping %bp.
> Also remember to restore original %dl after the call
I think there is no need to do so, because BIOS calls modify %dl anyway.
Then you can remove %bp and just make a long jump. This way int 13 handler recieves the original stack intact
> Could you also prepare a changelog entry for it?
Hmm... I don't really know how to write it, given that both files are
new and the patch does not modify any other GRUB function. Am I supposed
to also declare the modifications in the rmk files?
(written in mailer so identation is wrong)
<your contact line>
<short description>
* conf/i386-pc.rmk (pkglib_MODULES): add drivemap.mod
(drivemap_mod_SOURCES): new variable
(drivemap_mod_CFLAGS): likewise
(drivemap_mod_LDFLAGS): likewise
(drivemap_mod_ASFLAGS): likewise
* commands/i386/pc/drivemap.c: new file
* commands/i386/pc/drivemap_int13h.S: likewise
--
-- Lazy, Oblivious, Recurrent Disaster -- Habbit
_______________________________________________
Grub-devel mailing list
address@hidden
http://lists.gnu.org/mailman/listinfo/grub-devel