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

[Qemu-devel] [PATCH v2 0/3] 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.

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

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 | 12 ++++--------
 vl.c         | 13 +++++++++++--
 2 files changed, 15 insertions(+), 10 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v2 1/3] 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 v2 1/3] 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)

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 v2 1/3] bootdevice: remove the check about boot_set_handler

[Gonglei]

On 2015/2/6 16:56, 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)
> 
> 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.

Yes.

> 
> * 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.

Actually, I add this patch based on the previous conversation:
It duplicates the reset logic as well as information holder
locality (machine struct vs parameter).
http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg04172.html
and
http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg04292.html

Regards,
-Gonglei

> 
> 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.

[Qemu-devel] [PATCH v2 2/3] 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 | 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;
     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);
         }
 
@@ -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);
+        }
+    }
+
     ds = init_displaystate();
 
     /* init local displays */
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2 2/3] 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 | 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?

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

[Gonglei]

On 2015/2/6 17:06, 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 | 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.
> 

Agree.

>>      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?

Yes, it's cool. :)

Regards,
-Gonglei

[Qemu-devel] [PATCH v2 3/3] 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

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

[Gonglei]

On 2015/2/3 20:34, 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.
> 
> v2 -> v1:
>  - add patch 2 suggested by Markus.
>  - rework patch 3. (Markus)
>  - add R-by in patch 1.
> 

Markus, any thoughts? Thanks.

Regards,
-Gonglei

> 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 | 12 ++++--------
>  vl.c         | 13 +++++++++++--
>  2 files changed, 15 insertions(+), 10 deletions(-)
>