Re: [Qemu-devel] [PATCH v2 2/3] bootdevice: check boot order argument validation before vm running

[Markus Armbruster]

<address@hidden> writes:

> From: Gonglei <address@hidden>
>
> Either 'once' option or 'order' option can take effect for -boot at
> the same time, that is say initial startup processing can check only
> one. And pc.c's set_boot_dev() fails when its boot order argument
> is invalid. This patch provide a solution fix this problem:
>
>  1. If "once" is given, register reset handler to restore boot order.
>
>  2. Pass the normal boot order to machine creation.  Should fail when
>    the normal boot order is invalid.
>
>  3. If "once" is given, set it with qemu_boot_set().  Fails when the
>    once boot order is invalid.
>
>  4. Start the machine.
>
>  5. On reset, the reset handler calls qemu_boot_set() to restore boot
>    order.  Should never fail.
>
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  vl.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 983259b..0d90d98 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2734,6 +2734,7 @@ int main(int argc, char **argv, char **envp)
>      const char *initrd_filename;
>      const char *kernel_filename, *kernel_cmdline;
>      const char *boot_order;
> +    const char *once = NULL;

Consider renaming once to boot_once.  In its much larger scope, the boot
context isn't obvious anymore, so a more telling name would be in order.

>      DisplayState *ds;
>      int cyls, heads, secs, translation;
>      QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
> @@ -4046,7 +4047,7 @@ int main(int argc, char **argv, char **envp)
>      opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
>      if (opts) {
>          char *normal_boot_order;
> -        const char *order, *once;
> +        const char *order;
>          Error *local_err = NULL;
>  
>          order = qemu_opt_get(opts, "order");
> @@ -4067,7 +4068,6 @@ int main(int argc, char **argv, char **envp)
>                  exit(1);
>              }
>              normal_boot_order = g_strdup(boot_order);
> -            boot_order = once;
>              qemu_register_reset(restore_boot_order, normal_boot_order);
>          }
>  

normal_boot_order can be eliminated now.

        }
        qemu_register_reset(restore_boot_order, strdup(order));
    }

Even better, move qemu_register_reset()...

> @@ -4246,6 +4246,15 @@ int main(int argc, char **argv, char **envp)
>  
>      net_check_clients();
>  
> +    if (once) {
> +        Error *local_err = NULL;
> +        qemu_boot_set(once, &local_err);
> +        if (local_err) {
> +            error_report("%s", error_get_pretty(local_err));
> +            exit(1);
> +        }

... here:

           qemu_register_reset(restore_boot_order, strdup(boot_order));

> +    }
> +
>      ds = init_displaystate();
>  
>      /* init local displays */

Clearer, don't you think?