[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.9 2/2] Revert "hostmem: fix QEMU crash by
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [PATCH for-2.9 2/2] Revert "hostmem: fix QEMU crash by 'info memdev'" |
Date: |
Mon, 20 Mar 2017 16:10:54 -0500 |
User-agent: |
alot/0.3.6 |
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
>