Hello
2009/5/9 Javier Martín <lordhab...@gmail.com>
> 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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>
--
Regards
Vladimir 'phcoder' Serbinenko
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
