Re: normal mode chainloader

[Marco Gerards]

address@hidden (Tomas Ebenlendr) writes:

> Hm, I forgot to append the new version. (It fixes only the GCS-compliancy,
> as you commented the patch).

Ok, here are the comments I still have, to be complete.  These
comments are only related to the GCS.

In my other email I told you what I thought of the boot.mod
dependency.  If that is worked out all you need is Okuji's blessing,
after that I will commit the patch (that reminds me about some other
patches I will commit real soon... :-/).

No need to go through this patch again.  Please do what you can do
while fixing this patch to the outcome of the discussing about the
boot.mod dependency, I will do the rest before committing.

>       * commands/boot.c (GRUB_MOD_INIT): Add symbol
>       grub_boot_dependency, used forsemantic dependency of
>       commands.
>       * conf/i386-pc.rmk: Add i386/pc/chainloader_normal.c as
>       chain.mod.

Please include (chain_mod_SOURCES) here, like you did with
(GRUB_MODE_INIT).  Same for adding chain.mod to pkgdata_MODULES.  You
should notice you renamed _chain.mod to chain.mod.

>       * include/grub/i386/pc/loader.h (grub_rescue_cmd_chainloader):
>       Delete prototype of rescue command chainloader.

Ok, although I prefer `Deleted prototype' (or short `Deleted').

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

Please fix the comment.  I would use:


/* To let be loader commands dependent on this command (don't let gcc
   optimize off this symbol).  */
grub_boot_dependency = 1;

(Notice the `.' and the two spaces)

> diff -rupN -x CVS grub2_x/include/grub/boot_cmd.h 
> grub2_work/include/grub/boot_cmd.h
> --- grub2_x/include/grub/boot_cmd.h   1970-01-01 01:00:00.000000000 +0100
> +++ grub2_work/include/grub/boot_cmd.h        2004-06-21 01:33:53.000000000 
> +0200
> @@ -0,0 +1,26 @@
> +/* This file is intended as command dependency */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2002  Free Software Foundation, Inc.

copyright crap: You have to update the copyright years.  In this case
add 2004.  AFAIK you should not use short notations or ranges, just
list the years like:

 *  Copyright (C) 2002, 2004  Free Software Foundation, Inc.

> +typedef enum
> +  {
> +    GRUB_CHAINLOADER_FORCE = 0x1
> +  }
> +grub_chainloader_flags_t;

I think this should be:

 } grub_chainloader_flags_t;

> +void EXPORT_FUNC(grub_chainloader_cmd) (const char * 
> file,grub_chainloader_flags_t flags);

A long line...

> +/* FIXME these commands should be divided to argument parsing and real work, 
> and moved to
> + * respective headers*/

Please fix this comment.

> -  if (signature != grub_le_to_cpu16 (0xaa55) && ! force)
> +  if (signature != grub_le_to_cpu16 (0xaa55) && ! (flags & 
> GRUB_CHAINLOADER_FORCE) )

A long lines.  And the last parenthesis should not be separated with a
space.  This would be better:

if (signature != grub_le_to_cpu16 (0xaa55) 
    && ! (flags & GRUB_CHAINLOADER_FORCE))

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

The formatting of the comments is not correct.

Thanks,
Marco