Re: [Qemu-devel] [PATCH v2 1/3] bootdevice: remove the check about boot_set_handler

[Markus Armbruster]

<address@hidden> writes:

> From: Gonglei <address@hidden>
>
> The reset logic can be done by both machine reset and
> boot handler. So we shouldn't return error when the boot
> handler callback don't be set.
>
> Signed-off-by: Gonglei <address@hidden>
> Reviewed-by: Alexander Graf <address@hidden>
> ---
>  bootdevice.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/bootdevice.c b/bootdevice.c
> index 5914417..52d3f9e 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -51,19 +51,15 @@ void qemu_boot_set(const char *boot_order, Error **errp)
>  {
>      Error *local_err = NULL;
>  
> -    if (!boot_set_handler) {
> -        error_setg(errp, "no function defined to set boot device list for"
> -                         " this architecture");
> -        return;
> -    }
> -
>      validate_bootdevices(boot_order, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    boot_set_handler(boot_set_opaque, boot_order, errp);
> +    if (boot_set_handler) {
> +        boot_set_handler(boot_set_opaque, boot_order, errp);
> +    }
>  }
>  
>  void validate_bootdevices(const char *devices, Error **errp)

Two callers:

* HMP command boot_set

  Before your patch: command fails when the target doesn't support
  changing the boot order.

  After your patch: command silently does nothing.  I'm afraid that's a
  regression.

  Aside: looks like there's no QMP command.

* restore_boot_order()

  No change yet, because restore_boot_order() ignores errors.  But PATCH
  3 will make it abort on error.  I guess that's why you make the change
  here.

To avoid the regression, you could drop PATCH 1, and change PATCH 3 to
something like

-    qemu_boot_set(normal_boot_order, NULL);
+    if (boot_set_handler) {
+        qemu_boot_set(normal_boot_order, &error_abort);
+    }

There are other ways, but this looks like the simplest one.