Re: [PATCH] 1/5 Multiple fallback entries

[Vesa Jääskeläinen]

Colin D Bennett wrote:
> This patch adds support for multiple fallback entries in grub.cfg.
> 
> I'll prepare the ChangeLog entry when the patch is approved to be
> committed.
> 
> The patch is against GRUB trunk revision 1964.

What do you think if all text menu related would be moved to own file,
like menu_text.c or so ? I think it will cleanup code nicely...

> === modified file 'include/grub/normal.h'
> --- include/grub/normal.h     2009-01-31 09:15:43 +0000
> +++ include/grub/normal.h     2009-01-31 17:47:55 +0000
> @@ -98,10 +98,24 @@
>  
>  extern struct grub_menu_viewer grub_normal_terminal_menu_viewer;
>  
> +typedef struct grub_menu_execute_callback
> +{
> +  void (*notify_booting) (void *userdata, grub_menu_entry_t entry);
> +  void (*notify_fallback) (void *userdata, grub_menu_entry_t entry);
> +  void (*notify_failure) (void *userdata);

Usually pointer to userdata is last argument in list. Please use same
term for it as in grub_menu_execute_with_fallback (you are free to pick
new name if you see that it fits better).

> +} 
> +*grub_menu_execute_callback_t;
> +
>  void grub_enter_normal_mode (const char *config);
>  void grub_normal_execute (const char *config, int nested);
> +void grub_menu_execute_with_fallback (grub_menu_t menu,
> +                                      grub_menu_entry_t entry,
> +                                      grub_menu_execute_callback_t callback,
> +                                      void *callback_data);
>  void grub_menu_entry_run (grub_menu_entry_t entry);
>  void grub_menu_execute_entry(grub_menu_entry_t entry);
> +int grub_menu_get_timeout (void);
> +void grub_menu_set_timeout (int timeout);
>  void grub_cmdline_run (int nested);
>  int grub_cmdline_get (const char *prompt, char cmdline[], unsigned max_len,
>                     int echo_char, int readline);
> 
> === modified file 'normal/menu.c'
> --- normal/menu.c     2009-01-31 09:15:43 +0000
> +++ normal/menu.c     2009-01-31 17:47:55 +0000
> @@ -244,8 +244,8 @@
>  
>  /* Return the current timeout. If the variable "timeout" is not set or
>     invalid, return -1.  */
> -static int
> -get_timeout (void)
> +int
> +grub_menu_get_timeout (void)
>  {
>    char *val;
>    int timeout;
> @@ -272,8 +272,8 @@
>  }
>  
>  /* Set current timeout in the variable "timeout".  */
> -static void
> -set_timeout (int timeout)
> +void
> +grub_menu_set_timeout (int timeout)
>  {
>    /* Ignore TIMEOUT if it is zero, because it will be unset really soon.  */
>    if (timeout > 0)
> @@ -311,6 +311,44 @@
>    return entry;
>  }
>  
> +/* Get the first entry number from the variable NAME, which is a
> +   space-separated list of nonnegative integers.  The entry number which
> +   is returned is stripped from the value of NAME.  If no entry number can
> +   be found, returns -1.  */

We use term environment variable...

> +static int
> +get_and_remove_first_entry_number (const char *name)
> +{
> +  char *val;
> +  char *tail;
> +  int entry;
> +
> +  val = grub_env_get (name);
> +  if (! val)
> +    return -1;
> +
> +  grub_error_push ();
> +
> +  entry = (int) grub_strtoul (val, &tail, 0);

...

> +
> +/* Callbacks for grub_menu_execute_with_fallback() for the text menu.  */
> +

If you move these to new file, please replace this comment with new
comments for every function.

> +static void
> +notify_booting (void *userdata __attribute__((unused)),
> +             grub_menu_entry_t entry)
> +{
> +  grub_printf ("  Booting \'%s\'\n\n", entry->title);
> +}
> +
> +static void
> +notify_fallback (void *userdata __attribute__((unused)),
> +              grub_menu_entry_t entry)
> +{
> +  grub_printf ("\n  Falling back to \'%s\'\n\n", entry->title);
> +  grub_millisleep (2000);

I think this delay should be generic part of the implementation and not
specific to text menu. Or alternatively it needs to be documented that
fallback callback function is supposed to wait for 2 secs...