[Qemu-devel] [PATCH v2 0/2] fix strict boot regression

[Qemu-devel] [PATCH v2 0/2] fix strict boot regression

[Amos Kong]

parsing of "strict" is wrongly removed, we need it.
The first patch fixes a typo.

V2: remove bios.bin change, I just used it for testing (mjt)

Amos Kong (2):
  fix a typo of strict option
  add parsing of "strict" boot parameter

 vl.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

[Qemu-devel] [PATCH v2 1/2] fix a typo of strict option

[Amos Kong]

The type of "strict" should be bool.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index b0399de..12fe1f7 100644
--- a/vl.c
+++ b/vl.c
@@ -461,7 +461,7 @@ static QemuOptsList qemu_boot_opts = {
             .type = QEMU_OPT_STRING,
         }, {
             .name = "strict",
-            .type = QEMU_OPT_STRING,
+            .type = QEMU_OPT_BOOL,
         },
         { /*End of list */ }
     },
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 1/2] fix a typo of strict option

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> The type of "strict" should be bool.
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  vl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index b0399de..12fe1f7 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -461,7 +461,7 @@ static QemuOptsList qemu_boot_opts = {
>              .type = QEMU_OPT_STRING,
>          }, {
>              .name = "strict",
> -            .type = QEMU_OPT_STRING,
> +            .type = QEMU_OPT_BOOL,
>          },
>          { /*End of list */ }
>      },

No, this isn't just a typo fix, it's an interface change.  It's not
immediately obvious whether it's okay.

-boot parameter 'strict' appeared in commit c8a6ae8.  It was declared
QEMU_OPT_STRING, but the code using the value rejected anything but 'on'
and 'off'.  This shipped in 1.5.0.

Commit 6ef4716 accidentally dropped the code using the value of
'strict'.  Since then, any parameter value is accepted and silently
ignored.  Shipped in 1.6.0.

Your change restores the "reject anything but 'on' and 'off'" part (the
"use the value" part gets restored in the next patch).  Okay, because
the values erroneously accepted in the meantime didn't do anything.

Re: [Qemu-devel] [PATCH v2 1/2] fix a typo of strict option

[Amos Kong]

On Mon, Dec 09, 2013 at 10:16:38AM +0100, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> 
> > The type of "strict" should be bool.
> >
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  vl.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/vl.c b/vl.c
> > index b0399de..12fe1f7 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -461,7 +461,7 @@ static QemuOptsList qemu_boot_opts = {
> >              .type = QEMU_OPT_STRING,
> >          }, {
> >              .name = "strict",
> > -            .type = QEMU_OPT_STRING,
> > +            .type = QEMU_OPT_BOOL,
> >          },
> >          { /*End of list */ }
> >      },
> 
> No, this isn't just a typo fix, it's an interface change.  It's not
> immediately obvious whether it's okay.
 
You are correct, it's not a typo, menu's type was also changed from STRING to BOOL.
We can just process strict as menu.

> -boot parameter 'strict' appeared in commit c8a6ae8.  It was declared
> QEMU_OPT_STRING, but the code using the value rejected anything but 'on'
> and 'off'.  This shipped in 1.5.0.
> 
> Commit 6ef4716 accidentally dropped the code using the value of
> 'strict'.  Since then, any parameter value is accepted and silently
> ignored.  Shipped in 1.6.0.
> 
> Your change restores the "reject anything but 'on' and 'off'" part (the
> "use the value" part gets restored in the next patch).  Okay, because
> the values erroneously accepted in the meantime didn't do anything.

Thanks your explanation :)

-- 
			Amos.

[Qemu-devel] [PATCH v2 2/2] add parsing of "strict" boot parameter

[Amos Kong]

Commit 6ef4716cecdfa1b3794c1a33edba9840e1aa6b5f cleaned up the
parse code of boot parameter, parse code of "strict" wasn't
re-implemented, it caused that strict boot feature is always
disabled.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 vl.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 12fe1f7..ae6d0c6 100644
--- a/vl.c
+++ b/vl.c
@@ -4074,6 +4074,7 @@ int main(int argc, char **argv, char **envp)
         }
 
         boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
+        boot_strict = qemu_opt_get_bool(opts, "strict", false);
     }
 
     if (!kernel_cmdline) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 2/2] add parsing of "strict" boot parameter

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> Commit 6ef4716cecdfa1b3794c1a33edba9840e1aa6b5f cleaned up the
> parse code of boot parameter, parse code of "strict" wasn't
> re-implemented, it caused that strict boot feature is always
> disabled.
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  vl.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 12fe1f7..ae6d0c6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4074,6 +4074,7 @@ int main(int argc, char **argv, char **envp)
>          }
>  
>          boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
> +        boot_strict = qemu_opt_get_bool(opts, "strict", false);
>      }
>  
>      if (!kernel_cmdline) {

This restores the second half of what got dropped accidentally in commit
6ef4716.  Suggest to squash the two halves together, and rephrase the
commit message to more clearly mark it as bug fix.  Something like this:

    Fix -boot strict regressed in commit 6ef4716

    Commit 6ef4716 cleaned up parsing of -boot option argument, but
    accidentally dropped parameter strict.  It should have been updated
    exactly like parameter menu.  Do that.

I apologize for the screwup.  It took me 120 days to get my patch in,
and I botched the final rebase's conflict resolution.

Re: [Qemu-devel] [PATCH v2 0/2] fix strict boot regression

[Michael Tokarev]

09.12.2013 12:05, Amos Kong wrote:
> parsing of "strict" is wrongly removed, we need it.
> The first patch fixes a typo.
> 
> V2: remove bios.bin change, I just used it for testing (mjt)
> 
> Amos Kong (2):
>   fix a typo of strict option
>   add parsing of "strict" boot parameter
> 
>  vl.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

I think it is okay to go to qemu-trivial (both patches are fine to my eyes).
I queued them up for now, will roll back if there's any objections.  At any
rate,

Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>

Note that boot_strict can be made static since it isn't used anywhere else.
I'll queue this change too.

Thanks,

/mjt