Re: [PATCH] Preboot support

["Yoshinori K. Okuji" <okuji@enbug.org>]

On Monday 13 April 2009 02:19:07 phcoder wrote:
> What about this one?

- ChangeLog, loader.h and loader.c are not consistent. For example, loader.h 
declares grub_loader_unregister_preboot_hook, but loader.c defines 
grub_loader_remove_preboot.

- I don't understand how preboot_func and preboot_rest_func are different. At 
least, not obvious. Can you elaborate on them?

- This part:

+
+  for (cur = preboots_head; cur; cur = cur->next)
+    if (err = cur->preboot_func (grub_loader_noreturn))
+      {
+       for (cur = cur->prev; cur; cur = cur->prev)
+         cur->preboot_rest_func ();
+       return err;
+      }

You should not set ERR inside the "if" clause. This is against the GNU Coding 
Standards. The main reason is that it is not friendly to debuggers (as you 
may not "step" between the assignment and the conditional jump).

Regards,
Okuji