Re: normal mode chainloader

[Marco Gerards]

address@hidden (Tomas Ebenlendr) writes:

> Here is patch that implements command chainloader in normal mode. It may be
> done wrong way, but it is something to talk about. I successfully tested it
> on bochs. (In fact it is so simple, that it must work.). I added an artifical
> symbol to boot.mod to get dependency of normal mode loader commands on the
> boot command.

That is really nice!

Why is the dependency of the on boot.mod required?

> P.P.S. I saw somewhere (in archive) that I should probably subscribe
>       something for FSF. I don't understand this legal stuff, I
>       subscribe anything good for this piece of sw if needed.
>       (But I have no electronic signature yet :-(, I didn't need any.)

That is right.  We need copyright assignments for GRUB.  It is
required so the FSF can defend copyright violations and prevent other
legal problems (remember SCO? ;)).

> Notes on architecture dependent files:
>       I dislike how architecture dependent files (headers, conf/*.rmk)
>       are copied or cut&pasted between architectures. In fact my opinion
>       is that body of function may be architecture dependent, then it
>       should be in architecture dir. But the interface (the prototype of
>       the function) is architecture independent and so it shouldn't be
>       in architecture dir (now I'm talking about loader.h).

Well, that is correct.  It needs to be cleaned up a bit.  I will do
that when I will continue the work on the PPC port.

>       Files are same probably because powerpc is not too far from
>       ibm-pc. But I think they shouldn't be copied. They should be
>       somehow shared. (Ok, it is not easy to find `name' for common
>       code (directory), but I think that same files should be linked
>       and nearly same files should include common part.)

Most files will be changed.  The port is not complete yet.  I think we
can better discuss that later.

>       Folowing patch modifies only i386/pc architecture, but I think
>       that exactly same changes should be done in powerpc files.

I can do that.


>From this point the comments on your patch will follow.  The patch
looked quite good.  Most comments were GCS related, I hope you
understand that a consistent coding style is important to me.

Can you please read the GCS?  I don' like commenting much on a patch
that looks quite good. ;)

>       * commands/boot.c: Add symbol grub_boot_dependency, used for
>       semantic dependency of commands.

Please include the function (or whatever you modify).  In this case:

        * commands/boot.c (GRUB_MOD_INIT): Add symbol
        grub_boot_dependency, used for semantic dependency of
        commands.

>       * include/grub/i386/pc/loader.h: added FIXME, deleted prototype
>       of rescue command chainloader.

Please use correct punctuation.  Start your sentences with a
capital.  A better way of describing this would be:

        * include/grub/i386/pc/loader.h (prototype_name): Deleted
        prototype of rescue command chainloader.

>       * loader/i386/pc/chainloader_normal.c: New file. Defines the
>       command chainloader.

For comments and entries in the changelog start sentences with a
capital, end with a dot and add two spaces after the dot.  For
example:

/* This is a comment.  Blah blah blah.  Foo.  */

This is in the GCS.  The main reason we want this is because emacs
expects this when using auto-fill-mode.

> +/* common function for normal and rescue mode commands*/

See what I said about comments.

> +    grub_chainloader_cmd(args[0],flags);

Please use:
grub_chainloader_cmd (args[0], flags);

> +  grub_boot_dependency = 1; /* To be dependent on boot command
> +                            (don't let gcc optimize off this symbol) */

I think it is cleaner to move this comment to the line above this one.

> +  grub_register_command ("chainloader", chainloader_command, 
> GRUB_COMMAND_FLAG_BOTH,
> +                      "chainloader [options] FILE", "Prepare to boot another 
> boot loader", options);
> +}

Please don't use such long lines.  Many people use 80 character wide
terminals.

Thanks,
Marco