Re: [Qemu-devel] [PATCH for-2.9 2/2] Revert "hostmem: fix QEMU crash by 'info memdev'"

[Michael Roth]

Quoting Markus Armbruster (2017-03-20 11:13:44)
> This reverts commit 1454d33f0507cb54d62ed80f494884157c9e7130.
> 
> The string input visitor regression fixed in the previous commit made
> visit_type_uint16List() fail on empty input.  query_memdev() calls it
> via object_property_get_uint16List().  Because it doesn't expect it to
> fail, it passes &error_abort, and duly crashes.
> 
> Commit 1454d33 "fixes" this crash by making
> host_memory_backend_get_host_nodes() return a list containing just
> MAX_NODES instead of the empty list.  Papers over the regression, and
> leads to bogus "info memdev" output, as shown below; revert.
> 
> I suspect that if we had bisected the crash back then, we would have
> found and fixed the actual bug instead of papering over it.
> 
> To reproduce, run HMP command "info memdev" with
> 
>     $ qemu-system-x86_64 --nodefaults -S -display none -monitor stdio -object 
> memory-backend-ram,id=mem1,size=4k
> 
> With this commit, "info memdev" prints
> 
>     memory backend: mem1
>       size:  4096
>       merge: true
>       dump: true
>       prealloc: false
>       policy: default
>       host nodes:
> 
> exactly like before commit 74f24cb.
> 
> Between commit 1454d33 and this commit, it prints
> 
>     memory backend: mem1
>       size:  4096
>       merge: true
>       dump: true
>       prealloc: false
>       policy: default
>       host nodes: 128
> 
> The last line is bogus.
> 
> Between commit 74f24cb and 1454d33, it crashes like this:
> 
>     Unexpected error in parse_str() at 
> /work/armbru/tmp/qemu/qapi/string-input-visitor.c:126:
>     Parameter 'null' expects an int64 value or range
>     Aborted (core dumped)
> 
> Cc: Xiao Guangrong <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>

Reviewed-by: Michael Roth <address@hidden>

> ---
>  backends/hostmem.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 162c218..89feb9e 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -64,14 +64,6 @@ out:
>      error_propagate(errp, local_err);
>  }
> 
> -static uint16List **host_memory_append_node(uint16List **node,
> -                                            unsigned long value)
> -{
> -     *node = g_malloc0(sizeof(**node));
> -     (*node)->value = value;
> -     return &(*node)->next;
> -}
> -
>  static void
>  host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
>                                     void *opaque, Error **errp)
> @@ -82,23 +74,25 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor 
> *v, const char *name,
>      unsigned long value;
> 
>      value = find_first_bit(backend->host_nodes, MAX_NODES);
> -
> -    node = host_memory_append_node(node, value);
> -
>      if (value == MAX_NODES) {
> -        goto out;
> +        return;
>      }
> 
> +    *node = g_malloc0(sizeof(**node));
> +    (*node)->value = value;
> +    node = &(*node)->next;
> +
>      do {
>          value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
>          if (value == MAX_NODES) {
>              break;
>          }
> 
> -        node = host_memory_append_node(node, value);
> +        *node = g_malloc0(sizeof(**node));
> +        (*node)->value = value;
> +        node = &(*node)->next;
>      } while (true);
> 
> -out:
>      visit_type_uint16List(v, name, &host_nodes, errp);
>  }
> 
> -- 
> 2.7.4
>