Re: [Qemu-devel] [PATCH v3 1/4] 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)

You didn't address my review of v2 (appended for your convenience).  You
replied to it, pointing to previous conversation, but I'm afraid don't
understand how that conversation applies to changing behavior of HMP
command boot_set.

If changing boot_set to silently do nothing instead of failing loudly
when the target doesn't support changing the boot order is what you
want, then you have to document it *prominently* in the commit message.

My advice is not to change boot_set's behavior that way, because when
the user's command makes no sense, ignoring it silently instead of
telling him about the problem is not nice.  My review comment describes
one way to do that.  There are others.


Review of v2:

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.