[Qemu-devel] [PATCH v3 0/4] bootdevcie: change the boot order validation logic

[Qemu-devel] [PATCH v3 0/4] bootdevcie: change the boot order validation logic

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

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 in patch 1.

Patch 2 check boot order argument validation
before vm running.

Patch 3 passing &error_abort instead of NULL.
Patch 4 update boot order in MachineState in qemu_boot_set
in order to support changing boot order on other machine type,
such as sPAPR.

v3 -> v2:
 - rework patch 2 using clearer variable name and coding order. (Maruks)
 - add patch 4 in this patch series because of revelance.

v2 -> v1:
 - add patch 2 suggested by Markus.
 - rework patch 3. (Maruks)
 - add R-by in patch 1.

Dinar Valeev (1):
  bootdevice: update boot_order in MachineState

Gonglei (3):
  bootdevice: remove the check about boot_set_handler
  bootdevice: check boot order argument validation before vm running
  bootdevice: add check in restore_boot_order()

 bootdevice.c | 16 ++++++++--------
 vl.c         | 23 +++++++++++++++--------
 2 files changed, 23 insertions(+), 16 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v3 1/4] bootdevice: remove the check about boot_set_handler

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

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 <arei.gonglei@huawei.com>
Reviewed-by: Alexander Graf <agraf@suse.de>
---
 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)
-- 
1.7.12.4

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

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> 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 <arei.gonglei@huawei.com>
> Reviewed-by: Alexander Graf <agraf@suse.de>
> ---
>  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.

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

[Gonglei]

On 2015/2/12 18:19, Markus Armbruster wrote:
> <arei.gonglei@huawei.com> writes:
> 
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> 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 <arei.gonglei@huawei.com>
>> Reviewed-by: Alexander Graf <agraf@suse.de>
>> ---
>>  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.
> 
Yes, indeed. Maybe I ignored the key point of your review comments, sorry
for that. :(

> 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.
> 
Yes, I agree with you.
> 
> 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.
> 
Yes, it is.
>   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.
> 
The main cause that I make the change here is making preparation for PATCH 4
(I will explain my original purpose about this patch in another thread).
But As your comments, it cause a regression for HMP command boot_set. So,
that's not a good idea after careful consideration.
> To avoid the regression, you could drop PATCH 1, and change PATCH 3 to
> something like
> 
It's ok.
> -    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.
> 

Regards,
-Gonglei

[Qemu-devel] [PATCH v3 2/4] bootdevice: check boot order argument validation before vm running

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

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 <armbru@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 vl.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/vl.c b/vl.c
index 983259b..24b4c38 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 *boot_once = NULL;
     DisplayState *ds;
     int cyls, heads, secs, translation;
     QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
@@ -4045,8 +4046,7 @@ int main(int argc, char **argv, char **envp)
     boot_order = machine_class->default_boot_order;
     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");
@@ -4059,16 +4059,13 @@ int main(int argc, char **argv, char **envp)
             boot_order = order;
         }
 
-        once = qemu_opt_get(opts, "once");
-        if (once) {
-            validate_bootdevices(once, &local_err);
+        boot_once = qemu_opt_get(opts, "once");
+        if (boot_once) {
+            validate_bootdevices(boot_once, &local_err);
             if (local_err) {
                 error_report("%s", error_get_pretty(local_err));
                 exit(1);
             }
-            normal_boot_order = g_strdup(boot_order);
-            boot_order = once;
-            qemu_register_reset(restore_boot_order, normal_boot_order);
         }
 
         boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
@@ -4246,6 +4243,16 @@ int main(int argc, char **argv, char **envp)
 
     net_check_clients();
 
+    if (boot_once) {
+        Error *local_err = NULL;
+        qemu_boot_set(boot_once, &local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            exit(1);
+        }
+        qemu_register_reset(restore_boot_order, g_strdup(boot_order));
+    }
+
     ds = init_displaystate();
 
     /* init local displays */
-- 
1.7.12.4

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

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> 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 <armbru@redhat.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  vl.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 983259b..24b4c38 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 *boot_once = NULL;
>      DisplayState *ds;
>      int cyls, heads, secs, translation;
>      QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
> @@ -4045,8 +4046,7 @@ int main(int argc, char **argv, char **envp)
>      boot_order = machine_class->default_boot_order;
>      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");
> @@ -4059,16 +4059,13 @@ int main(int argc, char **argv, char **envp)
           if (order) {
               validate_bootdevices(order, &local_err);
               if (local_err) {
                   error_report_err(local_err);
                   exit(1);
               }
>              boot_order = order;
>          }
>  
> -        once = qemu_opt_get(opts, "once");
> -        if (once) {
> -            validate_bootdevices(once, &local_err);
> +        boot_once = qemu_opt_get(opts, "once");
> +        if (boot_once) {
> +            validate_bootdevices(boot_once, &local_err);
>              if (local_err) {
>                  error_report("%s", error_get_pretty(local_err));
>                  exit(1);
>              }
> -            normal_boot_order = g_strdup(boot_order);
> -            boot_order = once;
> -            qemu_register_reset(restore_boot_order, normal_boot_order);
>          }
>  
>          boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);

Should work fine.  There's a slight asymmetry, though: parameter "order"
is fetched into a local variable, checked, and only then stored into its
global variable.  Parameter "once" goes straight to the global variable.
Matter of taste, but treating them the same would be nice.  Your choice.

> @@ -4246,6 +4243,16 @@ int main(int argc, char **argv, char **envp)
>  
>      net_check_clients();
>  
> +    if (boot_once) {
> +        Error *local_err = NULL;
> +        qemu_boot_set(boot_once, &local_err);
> +        if (local_err) {
> +            error_report("%s", error_get_pretty(local_err));
> +            exit(1);
> +        }
> +        qemu_register_reset(restore_boot_order, g_strdup(boot_order));
> +    }
> +
>      ds = init_displaystate();
>  
>      /* init local displays */

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

[Gonglei]

On 2015/2/12 18:19, Markus Armbruster wrote:
> <arei.gonglei@huawei.com> writes:
> 
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> 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 <armbru@redhat.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  vl.c | 23 +++++++++++++++--------
>>  1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 983259b..24b4c38 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 *boot_once = NULL;
>>      DisplayState *ds;
>>      int cyls, heads, secs, translation;
>>      QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
>> @@ -4045,8 +4046,7 @@ int main(int argc, char **argv, char **envp)
>>      boot_order = machine_class->default_boot_order;
>>      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");
>> @@ -4059,16 +4059,13 @@ int main(int argc, char **argv, char **envp)
>            if (order) {
>                validate_bootdevices(order, &local_err);
>                if (local_err) {
>                    error_report_err(local_err);
>                    exit(1);
>                }
>>              boot_order = order;
>>          }
>>  
>> -        once = qemu_opt_get(opts, "once");
>> -        if (once) {
>> -            validate_bootdevices(once, &local_err);
>> +        boot_once = qemu_opt_get(opts, "once");
>> +        if (boot_once) {
>> +            validate_bootdevices(boot_once, &local_err);
>>              if (local_err) {
>>                  error_report("%s", error_get_pretty(local_err));
>>                  exit(1);
>>              }
>> -            normal_boot_order = g_strdup(boot_order);
>> -            boot_order = once;
>> -            qemu_register_reset(restore_boot_order, normal_boot_order);
>>          }
>>  
>>          boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
> 
> Should work fine.  There's a slight asymmetry, though: parameter "order"
> is fetched into a local variable, checked, and only then stored into its
> global variable.  Parameter "once" goes straight to the global variable.
> Matter of taste, but treating them the same would be nice.  Your choice.
> 
Agree, I get your meaning. :)

Regards,
-Gonglei
>> @@ -4246,6 +4243,16 @@ int main(int argc, char **argv, char **envp)
>>  
>>      net_check_clients();
>>  
>> +    if (boot_once) {
>> +        Error *local_err = NULL;
>> +        qemu_boot_set(boot_once, &local_err);
>> +        if (local_err) {
>> +            error_report("%s", error_get_pretty(local_err));
>> +            exit(1);
>> +        }
>> +        qemu_register_reset(restore_boot_order, g_strdup(boot_order));
>> +    }
>> +
>>      ds = init_displaystate();
>>  
>>      /* init local displays */

[Qemu-devel] [PATCH v3 3/4] bootdevice: add check in restore_boot_order()

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

qemu_boot_set() can't fail in restore_boot_order(),
then simply assert it doesn't fail, by passing
&error_abort.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 bootdevice.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bootdevice.c b/bootdevice.c
index 52d3f9e..d3d4277 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -101,7 +101,7 @@ void restore_boot_order(void *opaque)
         return;
     }
 
-    qemu_boot_set(normal_boot_order, NULL);
+    qemu_boot_set(normal_boot_order, &error_abort);
 
     qemu_unregister_reset(restore_boot_order, normal_boot_order);
     g_free(normal_boot_order);
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 4/4] bootdevice: update boot_order in MachineState

[arei.gonglei]

From: Dinar Valeev <dvaleev@suse.com>

on sPAPR we need to update boot_order in MachineState in case it
got changed on reset.

Signed-off-by: Dinar Valeev <dvaleev@suse.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 bootdevice.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/bootdevice.c b/bootdevice.c
index d3d4277..ac586b1 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -26,6 +26,7 @@
 #include "qapi/visitor.h"
 #include "qemu/error-report.h"
 #include "hw/hw.h"
+#include "hw/boards.h"
 
 typedef struct FWBootEntry FWBootEntry;
 
@@ -50,6 +51,7 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
 void qemu_boot_set(const char *boot_order, Error **errp)
 {
     Error *local_err = NULL;
+    MachineState *machine = MACHINE(qdev_get_machine());
 
     validate_bootdevices(boot_order, &local_err);
     if (local_err) {
@@ -57,6 +59,8 @@ void qemu_boot_set(const char *boot_order, Error **errp)
         return;
     }
 
+    machine->boot_order = boot_order;
+
     if (boot_set_handler) {
         boot_set_handler(boot_set_opaque, boot_order, errp);
     }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 4/4] bootdevice: update boot_order in MachineState

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Dinar Valeev <dvaleev@suse.com>
>
> on sPAPR we need to update boot_order in MachineState in case it
> got changed on reset.
>
> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  bootdevice.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/bootdevice.c b/bootdevice.c
> index d3d4277..ac586b1 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -26,6 +26,7 @@
>  #include "qapi/visitor.h"
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
> +#include "hw/boards.h"
>  
>  typedef struct FWBootEntry FWBootEntry;
>  
> @@ -50,6 +51,7 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
>  void qemu_boot_set(const char *boot_order, Error **errp)
>  {
>      Error *local_err = NULL;
> +    MachineState *machine = MACHINE(qdev_get_machine());
>  
>      validate_bootdevices(boot_order, &local_err);
>      if (local_err) {
> @@ -57,6 +59,8 @@ void qemu_boot_set(const char *boot_order, Error **errp)
>          return;
>      }
>  
> +    machine->boot_order = boot_order;
> +
>      if (boot_set_handler) {
>          boot_set_handler(boot_set_opaque, boot_order, errp);
>      }

Looks harmless enough, but I'm afraid I don't quite understand why we
need this.  Can you explain it to me real slow?  :)

Re: [Qemu-devel] [PATCH v3 4/4] bootdevice: update boot_order in MachineState

[Gonglei]

On 2015/2/12 18:21, Markus Armbruster wrote:
> <arei.gonglei@huawei.com> writes:
> 
>> From: Dinar Valeev <dvaleev@suse.com>
>>
>> on sPAPR we need to update boot_order in MachineState in case it
>> got changed on reset.
>>
>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  bootdevice.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/bootdevice.c b/bootdevice.c
>> index d3d4277..ac586b1 100644
>> --- a/bootdevice.c
>> +++ b/bootdevice.c
>> @@ -26,6 +26,7 @@
>>  #include "qapi/visitor.h"
>>  #include "qemu/error-report.h"
>>  #include "hw/hw.h"
>> +#include "hw/boards.h"
>>  
>>  typedef struct FWBootEntry FWBootEntry;
>>  
>> @@ -50,6 +51,7 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
>>  void qemu_boot_set(const char *boot_order, Error **errp)
>>  {
>>      Error *local_err = NULL;
>> +    MachineState *machine = MACHINE(qdev_get_machine());
>>  
>>      validate_bootdevices(boot_order, &local_err);
>>      if (local_err) {
>> @@ -57,6 +59,8 @@ void qemu_boot_set(const char *boot_order, Error **errp)
>>          return;
>>      }
>>  
>> +    machine->boot_order = boot_order;
>> +
>>      if (boot_set_handler) {
>>          boot_set_handler(boot_set_opaque, boot_order, errp);
>>      }
> 
> Looks harmless enough, but I'm afraid I don't quite understand why we
> need this.  Can you explain it to me real slow?  :)
> 
Alex pointed that there is two reset logic for boot order, one is machine struct
(machine->order) and another is set_boot_handler by parameter. But the
qemu_boot_set() only support to use set_boot_handler to set boot order.
So, I decide to remove the check for boot_set_handler if NULL or not (PATCH 1),
and change the value of machine->boot_order in qemu_boot_set().

But I ignored this change cause a regression for HMP command set_boot. To avoid
the regression, we only support one way to set boot_order, that is set_boot_handler.
Fortunately the patches for supporting boot 'once' argument on sPAPR on the flight. We
have the opportunity to drop this change and avoid the regression.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v3 0/4] bootdevcie: change the boot order validation logic

[Gonglei]

On 2015/2/7 15:20, Gonglei (Arei) wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> 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 in patch 1.
> 
> Patch 2 check boot order argument validation
> before vm running.
> 
> Patch 3 passing &error_abort instead of NULL.
> Patch 4 update boot order in MachineState in qemu_boot_set
> in order to support changing boot order on other machine type,
> such as sPAPR.
> 
> v3 -> v2:
>  - rework patch 2 using clearer variable name and coding order. (Maruks)
>  - add patch 4 in this patch series because of relevance.
> 
> v2 -> v1:
>  - add patch 2 suggested by Markus.
>  - rework patch 3. (Maruks)
>  - add R-by in patch 1.
> 
> Dinar Valeev (1):
>   bootdevice: update boot_order in MachineState
> 
> Gonglei (3):
>   bootdevice: remove the check about boot_set_handler
>   bootdevice: check boot order argument validation before vm running
>   bootdevice: add check in restore_boot_order()
> 
>  bootdevice.c | 16 ++++++++--------
>  vl.c         | 23 +++++++++++++++--------
>  2 files changed, 23 insertions(+), 16 deletions(-)
> 
Any ideas, Markus?  Thanks.

With the Chinese new year during near, a seven day holiday has already
on the way. So I plan to send a pull request on this Sunday. :)

Regards,
-Gonglei