[Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Daniel P. Berrangé]

This short series fixes a flaw identified by coverity which broke the
handling of multiboot modules.

Daniel P. Berrangé (3):
  i386: fix regression parsing multiboot initrd modules
  i386: only parse the initrd_filename once for multiboot modules
  opts: remove redundant check for NULL parameter

 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules

[Daniel P. Berrangé]

The logic for parsing the multiboot initrd modules was messed up in

  commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Mon Apr 16 12:17:43 2018 +0100

    opts: don't silently truncate long option values

Causing the length to be undercounter, and the number of modules over
counted. It also passes NULL to get_opt_value() which was not robust
at accepting a NULL value.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/i386/multiboot.c | 3 +--
 util/qemu-option.c  | 4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 7a2953e26f..8e26545814 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -292,8 +292,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     cmdline_len += strlen(kernel_cmdline) + 1;
     if (initrd_filename) {
         const char *r = get_opt_value(initrd_filename, NULL);
-        cmdline_len += strlen(r) + 1;
-        mbs.mb_mods_avail = 1;
+        cmdline_len += strlen(initrd_filename) + 1;
         while (1) {
             mbs.mb_mods_avail++;
             r = get_opt_value(r, NULL);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 58d1c23893..8a68bc2314 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -75,7 +75,9 @@ const char *get_opt_value(const char *p, char **value)
     size_t capacity = 0, length;
     const char *offset;
 
-    *value = NULL;
+    if (value) {
+        *value = NULL;
+    }
     while (1) {
         offset = strchr(p, ',');
         if (!offset) {
-- 
2.17.0

Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules

[Peter Maydell]

On 14 May 2018 at 18:19, Daniel P. Berrangé <berrange@redhat.com> wrote:
> The logic for parsing the multiboot initrd modules was messed up in
>
>   commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
>   Author: Daniel P. Berrangé <berrange@redhat.com>
>   Date:   Mon Apr 16 12:17:43 2018 +0100
>
>     opts: don't silently truncate long option values
>
> Causing the length to be undercounter, and the number of modules over
> counted. It also passes NULL to get_opt_value() which was not robust
> at accepting a NULL value.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/i386/multiboot.c | 3 +--
>  util/qemu-option.c  | 4 +++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 7a2953e26f..8e26545814 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -292,8 +292,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>      cmdline_len += strlen(kernel_cmdline) + 1;
>      if (initrd_filename) {
>          const char *r = get_opt_value(initrd_filename, NULL);
> -        cmdline_len += strlen(r) + 1;
> -        mbs.mb_mods_avail = 1;
> +        cmdline_len += strlen(initrd_filename) + 1;
>          while (1) {
>              mbs.mb_mods_avail++;
>              r = get_opt_value(r, NULL);
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 58d1c23893..8a68bc2314 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -75,7 +75,9 @@ const char *get_opt_value(const char *p, char **value)
>      size_t capacity = 0, length;
>      const char *offset;
>
> -    *value = NULL;
> +    if (value) {
> +        *value = NULL;
> +    }
>      while (1) {
>          offset = strchr(p, ',');
>          if (!offset) {

Don't we delete this check again in patch 3? If we're
going to fix this by making multiboot.c not pass in NULL pointers,
is there a reason not to simply do that?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules

[Daniel P. Berrangé]

On Fri, May 18, 2018 at 06:54:48PM +0100, Peter Maydell wrote:
> On 14 May 2018 at 18:19, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > The logic for parsing the multiboot initrd modules was messed up in
> >
> >   commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
> >   Author: Daniel P. Berrangé <berrange@redhat.com>
> >   Date:   Mon Apr 16 12:17:43 2018 +0100
> >
> >     opts: don't silently truncate long option values
> >
> > Causing the length to be undercounter, and the number of modules over
> > counted. It also passes NULL to get_opt_value() which was not robust
> > at accepting a NULL value.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  hw/i386/multiboot.c | 3 +--
> >  util/qemu-option.c  | 4 +++-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> > index 7a2953e26f..8e26545814 100644
> > --- a/hw/i386/multiboot.c
> > +++ b/hw/i386/multiboot.c
> > @@ -292,8 +292,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> >      cmdline_len += strlen(kernel_cmdline) + 1;
> >      if (initrd_filename) {
> >          const char *r = get_opt_value(initrd_filename, NULL);
> > -        cmdline_len += strlen(r) + 1;
> > -        mbs.mb_mods_avail = 1;
> > +        cmdline_len += strlen(initrd_filename) + 1;
> >          while (1) {
> >              mbs.mb_mods_avail++;
> >              r = get_opt_value(r, NULL);
> > diff --git a/util/qemu-option.c b/util/qemu-option.c
> > index 58d1c23893..8a68bc2314 100644
> > --- a/util/qemu-option.c
> > +++ b/util/qemu-option.c
> > @@ -75,7 +75,9 @@ const char *get_opt_value(const char *p, char **value)
> >      size_t capacity = 0, length;
> >      const char *offset;
> >
> > -    *value = NULL;
> > +    if (value) {
> > +        *value = NULL;
> > +    }
> >      while (1) {
> >          offset = strchr(p, ',');
> >          if (!offset) {
> 
> Don't we delete this check again in patch 3? If we're
> going to fix this by making multiboot.c not pass in NULL pointers,
> is there a reason not to simply do that?

That is correct, however, I wanted to do a really simple fix that addressed
the crasher regression, so that aspect of the fix wasn't obscured by the
bigger refactoring I do in 2 & 3.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules

[Eduardo Habkost]

On Mon, May 14, 2018 at 06:19:11PM +0100, Daniel P. Berrangé wrote:
> The logic for parsing the multiboot initrd modules was messed up in
> 
>   commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
>   Author: Daniel P. Berrangé <berrange@redhat.com>
>   Date:   Mon Apr 16 12:17:43 2018 +0100
> 
>     opts: don't silently truncate long option values
> 
> Causing the length to be undercounter, and the number of modules over
> counted. It also passes NULL to get_opt_value() which was not robust
> at accepting a NULL value.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Queueing on x86-next.  Thanks.

-- 
Eduardo

[Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules

[Daniel P. Berrangé]

The multiboot code parses the initrd_filename twice, first to count how
many entries there are, and second to process each entry. This changes
the first loop to store the parse module names in a list, and the second
loop can now use these names. This avoids having to pass NULL to the
get_opt_value() method which means it can safely assume a non-NULL param.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/i386/multiboot.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 8e26545814..d519e206c5 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -161,6 +161,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     uint8_t bootinfo[MBI_SIZE];
     uint8_t *mb_bootinfo_data;
     uint32_t cmdline_len;
+    GList *mods = NULL;
 
     /* Ok, let's see if it is a multiboot image.
        The header is 12x32bit long, so the latest entry may be 8192 - 48. */
@@ -291,15 +292,16 @@ int load_multiboot(FWCfgState *fw_cfg,
     cmdline_len = strlen(kernel_filename) + 1;
     cmdline_len += strlen(kernel_cmdline) + 1;
     if (initrd_filename) {
-        const char *r = get_opt_value(initrd_filename, NULL);
+        const char *r = initrd_filename;
         cmdline_len += strlen(initrd_filename) + 1;
-        while (1) {
+        while (*r) {
+            char *value;
+            r = get_opt_value(r, &value);
             mbs.mb_mods_avail++;
-            r = get_opt_value(r, NULL);
-            if (!*r) {
-                break;
+            mods = g_list_append(mods, value);
+            if (*r) {
+                r++;
             }
-            r++;
         }
     }
 
@@ -314,20 +316,16 @@ int load_multiboot(FWCfgState *fw_cfg,
     mbs.offset_cmdlines   = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
     mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
 
-    if (initrd_filename) {
-        const char *next_initrd;
-        char not_last;
-        char *one_file = NULL;
-
+    if (mods) {
+        GList *tmpl = mods;
         mbs.offset_mods = mbs.mb_buf_size;
 
-        do {
+        while (tmpl) {
             char *next_space;
             int mb_mod_length;
             uint32_t offs = mbs.mb_buf_size;
+            char *one_file = tmpl->data;
 
-            next_initrd = get_opt_value(initrd_filename, &one_file);
-            not_last = *next_initrd;
             /* if a space comes after the module filename, treat everything
                after that as parameters */
             hwaddr c = mb_add_cmdline(&mbs, one_file);
@@ -352,10 +350,10 @@ int load_multiboot(FWCfgState *fw_cfg,
             mb_debug("mod_start: %p\nmod_end:   %p\n  cmdline: "TARGET_FMT_plx,
                      (char *)mbs.mb_buf + offs,
                      (char *)mbs.mb_buf + offs + mb_mod_length, c);
-            initrd_filename = next_initrd+1;
             g_free(one_file);
-            one_file = NULL;
-        } while (not_last);
+            tmpl = tmpl->next;
+        }
+        g_list_free(mods);
     }
 
     /* Commandline support */
-- 
2.17.0

Re: [Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules

[Eduardo Habkost]

On Mon, May 14, 2018 at 06:19:12PM +0100, Daniel P. Berrangé wrote:
> The multiboot code parses the initrd_filename twice, first to count how
> many entries there are, and second to process each entry. This changes
> the first loop to store the parse module names in a list, and the second
> loop can now use these names. This avoids having to pass NULL to the
> get_opt_value() method which means it can safely assume a non-NULL param.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
[...]
> @@ -352,10 +350,10 @@ int load_multiboot(FWCfgState *fw_cfg,
>              mb_debug("mod_start: %p\nmod_end:   %p\n  cmdline: "TARGET_FMT_plx,
>                       (char *)mbs.mb_buf + offs,
>                       (char *)mbs.mb_buf + offs + mb_mod_length, c);
> -            initrd_filename = next_initrd+1;
>              g_free(one_file);
> -            one_file = NULL;
> -        } while (not_last);
> +            tmpl = tmpl->next;
> +        }
> +        g_list_free(mods);

I thought this would cause double free of tmpl->data, but
g_list_free() won't free the elements' data.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Queueing on x86-next.

-- 
Eduardo

[Qemu-devel] [PATCH 3/3] opts: remove redundant check for NULL parameter

[Daniel P. Berrangé]

No callers of get_opt_value() pass in a NULL for the "value" parameter,
so the check is redundant.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/qemu-option.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 8a68bc2314..a1ff682aac 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -75,9 +75,7 @@ const char *get_opt_value(const char *p, char **value)
     size_t capacity = 0, length;
     const char *offset;
 
-    if (value) {
-        *value = NULL;
-    }
+    *value = NULL;
     while (1) {
         offset = strchr(p, ',');
         if (!offset) {
@@ -88,11 +86,9 @@ const char *get_opt_value(const char *p, char **value)
         if (*offset != '\0' && *(offset + 1) == ',') {
             length++;
         }
-        if (value) {
-            *value = g_renew(char, *value, capacity + length + 1);
-            strncpy(*value + capacity, p, length);
-            (*value)[capacity + length] = '\0';
-        }
+        *value = g_renew(char, *value, capacity + length + 1);
+        strncpy(*value + capacity, p, length);
+        (*value)[capacity + length] = '\0';
         capacity += length;
         if (*offset == '\0' ||
             *(offset + 1) != ',') {
-- 
2.17.0

Re: [Qemu-devel] [PATCH 3/3] opts: remove redundant check for NULL parameter

[Eduardo Habkost]

On Mon, May 14, 2018 at 06:19:13PM +0100, Daniel P. Berrangé wrote:
> No callers of get_opt_value() pass in a NULL for the "value" parameter,
> so the check is redundant.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Queueing on x86-next.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Daniel P. Berrangé]

ping...

On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> This short series fixes a flaw identified by coverity which broke the
> handling of multiboot modules.
> 
> Daniel P. Berrangé (3):
>   i386: fix regression parsing multiboot initrd modules
>   i386: only parse the initrd_filename once for multiboot modules
>   opts: remove redundant check for NULL parameter
> 
>  hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
>  util/qemu-option.c  |  8 +++-----
>  2 files changed, 19 insertions(+), 24 deletions(-)
> 
> -- 
> 2.17.0
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Roman Kagan]

On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> ping...
> 
> On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > This short series fixes a flaw identified by coverity which broke the
> > handling of multiboot modules.
> > 
> > Daniel P. Berrangé (3):
> >   i386: fix regression parsing multiboot initrd modules
> >   i386: only parse the initrd_filename once for multiboot modules
> >   opts: remove redundant check for NULL parameter
> > 
> >  hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
> >  util/qemu-option.c  |  8 +++-----
> >  2 files changed, 19 insertions(+), 24 deletions(-)
> > 
> > -- 
> > 2.17.0

Any reason this hasn't been merged yet?

This fixes a regression that, in particular, makes QEMU segfault when
running kvm-unit-tests.

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Eduardo Habkost]

On Tue, Jul 10, 2018 at 08:11:58PM +0300, Roman Kagan wrote:
> On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> > ping...
> > 
> > On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > > This short series fixes a flaw identified by coverity which broke the
> > > handling of multiboot modules.
> > > 
> > > Daniel P. Berrangé (3):
> > >   i386: fix regression parsing multiboot initrd modules
> > >   i386: only parse the initrd_filename once for multiboot modules
> > >   opts: remove redundant check for NULL parameter
> > > 
> > >  hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
> > >  util/qemu-option.c  |  8 +++-----
> > >  2 files changed, 19 insertions(+), 24 deletions(-)
> > > 
> > > -- 
> > > 2.17.0
> 
> Any reason this hasn't been merged yet?

Lack of reviews, unfortunately.  I guess we don't have many
people familiar with the multiboot code.

> 
> This fixes a regression that, in particular, makes QEMU segfault when
> running kvm-unit-tests.

As it is a bug fix, I will try to review merge it for the next
rc.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Roman Kagan]

On Tue, Jul 10, 2018 at 02:23:09PM -0300, Eduardo Habkost wrote:
> On Tue, Jul 10, 2018 at 08:11:58PM +0300, Roman Kagan wrote:
> > On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> > > ping...
> > > 
> > > On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > > > This short series fixes a flaw identified by coverity which broke the
> > > > handling of multiboot modules.
> > > > 
> > > > Daniel P. Berrangé (3):
> > > >   i386: fix regression parsing multiboot initrd modules
> > > >   i386: only parse the initrd_filename once for multiboot modules
> > > >   opts: remove redundant check for NULL parameter
> > > > 
> > > >  hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
> > > >  util/qemu-option.c  |  8 +++-----
> > > >  2 files changed, 19 insertions(+), 24 deletions(-)
> > > > 
> > > > -- 
> > > > 2.17.0
> > 
> > Any reason this hasn't been merged yet?
> 
> Lack of reviews, unfortunately.  I guess we don't have many
> people familiar with the multiboot code.
> 
> > 
> > This fixes a regression that, in particular, makes QEMU segfault when
> > running kvm-unit-tests.
> 
> As it is a bug fix, I will try to review merge it for the next
> rc.

Looks like it ended up missing 3.0, so kvm-unit-tests still segfault
with the released version of QEMU :(

Roman.

Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Daniel P. Berrangé]

On Thu, Aug 16, 2018 at 05:34:43PM +0300, Roman Kagan wrote:
> On Tue, Jul 10, 2018 at 02:23:09PM -0300, Eduardo Habkost wrote:
> > On Tue, Jul 10, 2018 at 08:11:58PM +0300, Roman Kagan wrote:
> > > On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> > > > ping...
> > > > 
> > > > On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > > > > This short series fixes a flaw identified by coverity which broke the
> > > > > handling of multiboot modules.
> > > > > 
> > > > > Daniel P. Berrangé (3):
> > > > >   i386: fix regression parsing multiboot initrd modules
> > > > >   i386: only parse the initrd_filename once for multiboot modules
> > > > >   opts: remove redundant check for NULL parameter
> > > > > 
> > > > >  hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
> > > > >  util/qemu-option.c  |  8 +++-----
> > > > >  2 files changed, 19 insertions(+), 24 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.17.0
> > > 
> > > Any reason this hasn't been merged yet?
> > 
> > Lack of reviews, unfortunately.  I guess we don't have many
> > people familiar with the multiboot code.
> > 
> > > 
> > > This fixes a regression that, in particular, makes QEMU segfault when
> > > running kvm-unit-tests.
> > 
> > As it is a bug fix, I will try to review merge it for the next
> > rc.
> 
> Looks like it ended up missing 3.0, so kvm-unit-tests still segfault
> with the released version of QEMU :(

Please check again, as I see it merged before 3.0:


  commit 59b5552f020b739e273e969a0933c23d8f4e2284
  Merge: ccf02d73d1 dfaa7d50b0
  Author: Peter Maydell <peter.maydell@linaro.org>
  Date:   Tue Jul 17 17:06:32 2018 +0100

    Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging
    
    Bug fixes.
    
    # gpg: Signature made Tue 17 Jul 2018 16:06:07 BST
    # gpg:                using RSA key BFFBD25F78C7AE83
    # gpg: Good signature from "Paolo Bonzini <bonzini@gnu.org>"
    # gpg:                 aka "Paolo Bonzini <pbonzini@redhat.com>"
    # Primary key fingerprint: 46F5 9FBD 57D6 12E7 BFD4  E2F7 7E15 100C CD36 69B1
    #      Subkey fingerprint: F133 3857 4B66 2389 866C  7682 BFFB D25F 78C7 AE83
    
    * remotes/bonzini/tags/for-upstream:
      Document command line options with single dash
      opts: remove redundant check for NULL parameter
      i386: only parse the initrd_filename once for multiboot modules
      i386: fix regression parsing multiboot initrd modules
      virtio-scsi: fix hotplug ->reset() vs event race
      qdev: add HotplugHandler->post_plug() callback
      hw/char/serial: retry write if EAGAIN
      PC Chipset: Improve serial divisor calculation
      vhost-user-test: added proper TestServer *dest initialization in test_migrate()
      hyperv: ensure VP index equal to QEMU cpu_index
      hyperv: rename vcpu_id to vp_index
      accel: Fix typo and grammar in comment
      dump: add kernel_gs_base to QEMU CPU state
    
    Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


$ git describe  59b5552f020b739e273e969a0933c23d8f4e2284
v3.0.0-rc0-80-g59b5552f02

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Roman Kagan]

On Thu, Aug 16, 2018 at 03:38:14PM +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 16, 2018 at 05:34:43PM +0300, Roman Kagan wrote:
> > On Tue, Jul 10, 2018 at 02:23:09PM -0300, Eduardo Habkost wrote:
> > > On Tue, Jul 10, 2018 at 08:11:58PM +0300, Roman Kagan wrote:
> > > > On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> > > > > ping...
> > > > > 
> > > > > On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > > > > > This short series fixes a flaw identified by coverity which broke the
> > > > > > handling of multiboot modules.
> > > > > > 
> > > > > > Daniel P. Berrangé (3):
> > > > > >   i386: fix regression parsing multiboot initrd modules
> > > > > >   i386: only parse the initrd_filename once for multiboot modules
> > > > > >   opts: remove redundant check for NULL parameter
> > > > > > 
> > > > > >  hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
> > > > > >  util/qemu-option.c  |  8 +++-----
> > > > > >  2 files changed, 19 insertions(+), 24 deletions(-)
> > > > > > 
> > > > > > -- 
> > > > > > 2.17.0
> > > > 
> > > > Any reason this hasn't been merged yet?
> > > 
> > > Lack of reviews, unfortunately.  I guess we don't have many
> > > people familiar with the multiboot code.
> > > 
> > > > 
> > > > This fixes a regression that, in particular, makes QEMU segfault when
> > > > running kvm-unit-tests.
> > > 
> > > As it is a bug fix, I will try to review merge it for the next
> > > rc.
> > 
> > Looks like it ended up missing 3.0, so kvm-unit-tests still segfault
> > with the released version of QEMU :(
> 
> Please check again, as I see it merged before 3.0:

Oh, you're right, I just ran the tests against a wrong QEMU.

It's OK now indeed.

Sorry for the noise,
Roman.

Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Roman Kagan]

On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> This short series fixes a flaw identified by coverity which broke the
> handling of multiboot modules.

This flaw also makes QEMU segfault when running kvm-unit-tests, and the
series fixes it.

So

Tested-by: Roman Kagan <rkagan@virtuozzo.com>

Wondering if anyone else runs kvm-unit-tests with QEMU master,
Roman.

> 
> Daniel P. Berrangé (3):
>   i386: fix regression parsing multiboot initrd modules
>   i386: only parse the initrd_filename once for multiboot modules
>   opts: remove redundant check for NULL parameter
> 
>  hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
>  util/qemu-option.c  |  8 +++-----
>  2 files changed, 19 insertions(+), 24 deletions(-)
> 
> -- 
> 2.17.0
> 
> 

Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules

[Michael S. Tsirkin]

On Wed, Jun 20, 2018 at 05:57:56PM +0300, Roman Kagan wrote:
> On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > This short series fixes a flaw identified by coverity which broke the
> > handling of multiboot modules.
> 
> This flaw also makes QEMU segfault when running kvm-unit-tests, and the
> series fixes it.
> 
> So
> 
> Tested-by: Roman Kagan <rkagan@virtuozzo.com>
> 
> Wondering if anyone else runs kvm-unit-tests with QEMU master,
> Roman.

It might be a good idea to make it a submodule, and
use it for unit tests. I'll take a look.

> > 
> > Daniel P. Berrangé (3):
> >   i386: fix regression parsing multiboot initrd modules
> >   i386: only parse the initrd_filename once for multiboot modules
> >   opts: remove redundant check for NULL parameter
> > 
> >  hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
> >  util/qemu-option.c  |  8 +++-----
> >  2 files changed, 19 insertions(+), 24 deletions(-)
> > 
> > -- 
> > 2.17.0
> > 
> >