* [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init()
@ 2015-07-16 20:39 Eduardo Habkost
2015-07-16 21:02 ` Igor Mammedov
2015-07-17 7:21 ` Marcel Apfelbaum
0 siblings, 2 replies; 7+ messages in thread
From: Eduardo Habkost @ 2015-07-16 20:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marcel Apfelbaum
This fixes the following crash, introduced by commit
49d2e648e8087d154d8bf8b91f27c8e05e79d5a6:
$ gdb --args qemu-system-x86_64 -machine pc,mem-merge=off -object memory-backend-ram,id=ram-node0,size=1024
[...]
Program received signal SIGABRT, Aborted.
(gdb) bt
#0 0x00007ffff253b8c7 in raise () at /lib64/libc.so.6
#1 0x00007ffff253d52a in abort () at /lib64/libc.so.6
#2 0x00007ffff253446d in __assert_fail_base () at /lib64/libc.so.6
#3 0x00007ffff2534522 in () at /lib64/libc.so.6
#4 0x00005555558bb80a in qemu_opt_get_bool_helper (opts=0x55555621b650, name=name@entry=0x5555558ec922 "mem-merge", defval=defval@entry=true, del=del@entry=false) at qemu/util/qemu-option.c:388
#5 0x00005555558bbb5a in qemu_opt_get_bool (opts=<optimized out>, name=name@entry=0x5555558ec922 "mem-merge", defval=defval@entry=true) at qemu/util/qemu-option.c:398
#6 0x0000555555720a24 in host_memory_backend_init (obj=0x5555562ac970) at qemu/backends/hostmem.c:226
Instead of using qemu_opt_get_bool(), that didn't work with
qemu_machine_opts for a long time, we can use the machine QOM properties
directly.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
backends/hostmem.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 61c1ac0..38a32ed 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -10,6 +10,7 @@
* See the COPYING file in the top-level directory.
*/
#include "sysemu/hostmem.h"
+#include "hw/boards.h"
#include "qapi/visitor.h"
#include "qapi-types.h"
#include "qapi-visit.h"
@@ -223,10 +224,10 @@ static void host_memory_backend_init(Object *obj)
{
HostMemoryBackend *backend = MEMORY_BACKEND(obj);
- backend->merge = qemu_opt_get_bool(qemu_get_machine_opts(),
- "mem-merge", true);
- backend->dump = qemu_opt_get_bool(qemu_get_machine_opts(),
- "dump-guest-core", true);
+ backend->merge = object_property_get_bool(OBJECT(current_machine),
+ "mem-merge", &error_abort);
+ backend->dump = object_property_get_bool(OBJECT(current_machine),
+ "dump-guest-core", &error_abort);
backend->prealloc = mem_prealloc;
object_property_add_bool(obj, "merge",
--
2.1.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init()
2015-07-16 20:39 [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init() Eduardo Habkost
@ 2015-07-16 21:02 ` Igor Mammedov
2015-07-17 20:33 ` Eduardo Habkost
2015-07-17 7:21 ` Marcel Apfelbaum
1 sibling, 1 reply; 7+ messages in thread
From: Igor Mammedov @ 2015-07-16 21:02 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, qemu-devel, Marcel Apfelbaum
On Thu, 16 Jul 2015 17:39:17 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> This fixes the following crash, introduced by commit
> 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6:
>
> $ gdb --args qemu-system-x86_64 -machine pc,mem-merge=off -object
> memory-backend-ram,id=ram-node0,size=1024 [...]
> Program received signal SIGABRT, Aborted.
> (gdb) bt
> #0 0x00007ffff253b8c7 in raise () at /lib64/libc.so.6
> #1 0x00007ffff253d52a in abort () at /lib64/libc.so.6
> #2 0x00007ffff253446d in __assert_fail_base () at /lib64/libc.so.6
> #3 0x00007ffff2534522 in () at /lib64/libc.so.6
> #4 0x00005555558bb80a in qemu_opt_get_bool_helper
> (opts=0x55555621b650, name=name@entry=0x5555558ec922 "mem-merge",
> defval=defval@entry=true, del=del@entry=false) at
> qemu/util/qemu-option.c:388 #5 0x00005555558bbb5a in
> qemu_opt_get_bool (opts=<optimized out>,
> name=name@entry=0x5555558ec922 "mem-merge", defval=defval@entry=true)
> at qemu/util/qemu-option.c:398 #6 0x0000555555720a24 in
> host_memory_backend_init (obj=0x5555562ac970) at
> qemu/backends/hostmem.c:226
>
> Instead of using qemu_opt_get_bool(), that didn't work with
> qemu_machine_opts for a long time, we can use the machine QOM
> properties directly.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> backends/hostmem.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 61c1ac0..38a32ed 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -10,6 +10,7 @@
> * See the COPYING file in the top-level directory.
> */
> #include "sysemu/hostmem.h"
> +#include "hw/boards.h"
> #include "qapi/visitor.h"
> #include "qapi-types.h"
> #include "qapi-visit.h"
> @@ -223,10 +224,10 @@ static void host_memory_backend_init(Object
> *obj) {
> HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>
> - backend->merge = qemu_opt_get_bool(qemu_get_machine_opts(),
> - "mem-merge", true);
> - backend->dump = qemu_opt_get_bool(qemu_get_machine_opts(),
> - "dump-guest-core", true);
> + backend->merge =
> object_property_get_bool(OBJECT(current_machine),
maybe use qdev_get_machine() instead of OBJECT(current_machine)
> + "mem-merge",
> &error_abort);
> + backend->dump = object_property_get_bool(OBJECT(current_machine),
> + "dump-guest-core",
> &error_abort); backend->prealloc = mem_prealloc;
>
> object_property_add_bool(obj, "merge",
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init()
2015-07-16 20:39 [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init() Eduardo Habkost
2015-07-16 21:02 ` Igor Mammedov
@ 2015-07-17 7:21 ` Marcel Apfelbaum
1 sibling, 0 replies; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-07-17 7:21 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel; +Cc: Paolo Bonzini, Marcel Apfelbaum
On 07/16/2015 11:39 PM, Eduardo Habkost wrote:
> This fixes the following crash, introduced by commit
> 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6:
>
> $ gdb --args qemu-system-x86_64 -machine pc,mem-merge=off -object memory-backend-ram,id=ram-node0,size=1024
> [...]
> Program received signal SIGABRT, Aborted.
> (gdb) bt
> #0 0x00007ffff253b8c7 in raise () at /lib64/libc.so.6
> #1 0x00007ffff253d52a in abort () at /lib64/libc.so.6
> #2 0x00007ffff253446d in __assert_fail_base () at /lib64/libc.so.6
> #3 0x00007ffff2534522 in () at /lib64/libc.so.6
> #4 0x00005555558bb80a in qemu_opt_get_bool_helper (opts=0x55555621b650, name=name@entry=0x5555558ec922 "mem-merge", defval=defval@entry=true, del=del@entry=false) at qemu/util/qemu-option.c:388
> #5 0x00005555558bbb5a in qemu_opt_get_bool (opts=<optimized out>, name=name@entry=0x5555558ec922 "mem-merge", defval=defval@entry=true) at qemu/util/qemu-option.c:398
> #6 0x0000555555720a24 in host_memory_backend_init (obj=0x5555562ac970) at qemu/backends/hostmem.c:226
>
> Instead of using qemu_opt_get_bool(), that didn't work with
> qemu_machine_opts for a long time, we can use the machine QOM properties
> directly.
This was the intent.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> backends/hostmem.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 61c1ac0..38a32ed 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -10,6 +10,7 @@
> * See the COPYING file in the top-level directory.
> */
> #include "sysemu/hostmem.h"
> +#include "hw/boards.h"
> #include "qapi/visitor.h"
> #include "qapi-types.h"
> #include "qapi-visit.h"
> @@ -223,10 +224,10 @@ static void host_memory_backend_init(Object *obj)
> {
> HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>
> - backend->merge = qemu_opt_get_bool(qemu_get_machine_opts(),
> - "mem-merge", true);
> - backend->dump = qemu_opt_get_bool(qemu_get_machine_opts(),
> - "dump-guest-core", true);
> + backend->merge = object_property_get_bool(OBJECT(current_machine),
> + "mem-merge", &error_abort);
> + backend->dump = object_property_get_bool(OBJECT(current_machine),
> + "dump-guest-core", &error_abort);
> backend->prealloc = mem_prealloc;
>
> object_property_add_bool(obj, "merge",
>
Thank you for fixing this,
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init()
2015-07-16 21:02 ` Igor Mammedov
@ 2015-07-17 20:33 ` Eduardo Habkost
2015-07-20 7:06 ` Igor Mammedov
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2015-07-17 20:33 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Paolo Bonzini, qemu-devel, Marcel Apfelbaum
On Thu, Jul 16, 2015 at 11:02:14PM +0200, Igor Mammedov wrote:
> On Thu, 16 Jul 2015 17:39:17 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > This fixes the following crash, introduced by commit
> > 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6:
> >
> > $ gdb --args qemu-system-x86_64 -machine pc,mem-merge=off -object
> > memory-backend-ram,id=ram-node0,size=1024 [...]
> > Program received signal SIGABRT, Aborted.
> > (gdb) bt
> > #0 0x00007ffff253b8c7 in raise () at /lib64/libc.so.6
> > #1 0x00007ffff253d52a in abort () at /lib64/libc.so.6
> > #2 0x00007ffff253446d in __assert_fail_base () at /lib64/libc.so.6
> > #3 0x00007ffff2534522 in () at /lib64/libc.so.6
> > #4 0x00005555558bb80a in qemu_opt_get_bool_helper
> > (opts=0x55555621b650, name=name@entry=0x5555558ec922 "mem-merge",
> > defval=defval@entry=true, del=del@entry=false) at
> > qemu/util/qemu-option.c:388 #5 0x00005555558bbb5a in
> > qemu_opt_get_bool (opts=<optimized out>,
> > name=name@entry=0x5555558ec922 "mem-merge", defval=defval@entry=true)
> > at qemu/util/qemu-option.c:398 #6 0x0000555555720a24 in
> > host_memory_backend_init (obj=0x5555562ac970) at
> > qemu/backends/hostmem.c:226
> >
> > Instead of using qemu_opt_get_bool(), that didn't work with
> > qemu_machine_opts for a long time, we can use the machine QOM
> > properties directly.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > backends/hostmem.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > index 61c1ac0..38a32ed 100644
> > --- a/backends/hostmem.c
> > +++ b/backends/hostmem.c
> > @@ -10,6 +10,7 @@
> > * See the COPYING file in the top-level directory.
> > */
> > #include "sysemu/hostmem.h"
> > +#include "hw/boards.h"
> > #include "qapi/visitor.h"
> > #include "qapi-types.h"
> > #include "qapi-visit.h"
> > @@ -223,10 +224,10 @@ static void host_memory_backend_init(Object
> > *obj) {
> > HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> >
> > - backend->merge = qemu_opt_get_bool(qemu_get_machine_opts(),
> > - "mem-merge", true);
> > - backend->dump = qemu_opt_get_bool(qemu_get_machine_opts(),
> > - "dump-guest-core", true);
> > + backend->merge =
> > object_property_get_bool(OBJECT(current_machine),
> maybe use qdev_get_machine() instead of OBJECT(current_machine)
What are the advantages you see in the extra layers of indirection of
qdev_get_machine()? (I am not against your proposal, but I would like to
understand the point of qdev_get_machine() yet.)
I'd prefer to use something that is guaranteed to be MachineState*,
qdev_get_machine() returns Object*. I am even considering using
current_machine->mem_merge and current_machine->dump_guest_core directly
instead of object_property_get_bool(). That would mean extra
compile-time checks, instead of runtime ones.
--
Eduardo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init()
2015-07-17 20:33 ` Eduardo Habkost
@ 2015-07-20 7:06 ` Igor Mammedov
2015-07-20 20:03 ` Eduardo Habkost
0 siblings, 1 reply; 7+ messages in thread
From: Igor Mammedov @ 2015-07-20 7:06 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, qemu-devel, Marcel Apfelbaum
On Fri, 17 Jul 2015 17:33:55 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Jul 16, 2015 at 11:02:14PM +0200, Igor Mammedov wrote:
> > On Thu, 16 Jul 2015 17:39:17 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > This fixes the following crash, introduced by commit
> > > 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6:
> > >
> > > $ gdb --args qemu-system-x86_64 -machine pc,mem-merge=off -object
> > > memory-backend-ram,id=ram-node0,size=1024 [...]
> > > Program received signal SIGABRT, Aborted.
> > > (gdb) bt
> > > #0 0x00007ffff253b8c7 in raise () at /lib64/libc.so.6
> > > #1 0x00007ffff253d52a in abort () at /lib64/libc.so.6
> > > #2 0x00007ffff253446d in __assert_fail_base () at /lib64/libc.so.6
> > > #3 0x00007ffff2534522 in () at /lib64/libc.so.6
> > > #4 0x00005555558bb80a in qemu_opt_get_bool_helper
> > > (opts=0x55555621b650, name=name@entry=0x5555558ec922 "mem-merge",
> > > defval=defval@entry=true, del=del@entry=false) at
> > > qemu/util/qemu-option.c:388 #5 0x00005555558bbb5a in
> > > qemu_opt_get_bool (opts=<optimized out>,
> > > name=name@entry=0x5555558ec922 "mem-merge", defval=defval@entry=true)
> > > at qemu/util/qemu-option.c:398 #6 0x0000555555720a24 in
> > > host_memory_backend_init (obj=0x5555562ac970) at
> > > qemu/backends/hostmem.c:226
> > >
> > > Instead of using qemu_opt_get_bool(), that didn't work with
> > > qemu_machine_opts for a long time, we can use the machine QOM
> > > properties directly.
> > >
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > backends/hostmem.c | 9 +++++----
> > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > > index 61c1ac0..38a32ed 100644
> > > --- a/backends/hostmem.c
> > > +++ b/backends/hostmem.c
> > > @@ -10,6 +10,7 @@
> > > * See the COPYING file in the top-level directory.
> > > */
> > > #include "sysemu/hostmem.h"
> > > +#include "hw/boards.h"
> > > #include "qapi/visitor.h"
> > > #include "qapi-types.h"
> > > #include "qapi-visit.h"
> > > @@ -223,10 +224,10 @@ static void host_memory_backend_init(Object
> > > *obj) {
> > > HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > >
> > > - backend->merge = qemu_opt_get_bool(qemu_get_machine_opts(),
> > > - "mem-merge", true);
> > > - backend->dump = qemu_opt_get_bool(qemu_get_machine_opts(),
> > > - "dump-guest-core", true);
> > > + backend->merge =
> > > object_property_get_bool(OBJECT(current_machine),
> > maybe use qdev_get_machine() instead of OBJECT(current_machine)
>
> What are the advantages you see in the extra layers of indirection of
> qdev_get_machine()? (I am not against your proposal, but I would like to
> understand the point of qdev_get_machine() yet.)
current_machine might be NULL where as qdev_get_machine() always returns
/machine object.
>
> I'd prefer to use something that is guaranteed to be MachineState*,
> qdev_get_machine() returns Object*. I am even considering using
> current_machine->mem_merge and current_machine->dump_guest_core directly
> instead of object_property_get_bool(). That would mean extra
> compile-time checks, instead of runtime ones.
Check difference 'git grep qdev_get_machine' vs 'git grep current_machine'.
I was under impression that policy was trying no to use globals unless one has to,
and not introduce new usage in presence of other means to get object.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init()
2015-07-20 7:06 ` Igor Mammedov
@ 2015-07-20 20:03 ` Eduardo Habkost
2015-07-20 20:06 ` Marcel Apfelbaum
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2015-07-20 20:03 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Paolo Bonzini, qemu-devel, Marcel Apfelbaum
On Mon, Jul 20, 2015 at 09:06:28AM +0200, Igor Mammedov wrote:
> On Fri, 17 Jul 2015 17:33:55 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Thu, Jul 16, 2015 at 11:02:14PM +0200, Igor Mammedov wrote:
> > > On Thu, 16 Jul 2015 17:39:17 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > This fixes the following crash, introduced by commit
> > > > 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6:
> > > >
> > > > $ gdb --args qemu-system-x86_64 -machine pc,mem-merge=off -object
> > > > memory-backend-ram,id=ram-node0,size=1024 [...]
> > > > Program received signal SIGABRT, Aborted.
> > > > (gdb) bt
> > > > #0 0x00007ffff253b8c7 in raise () at /lib64/libc.so.6
> > > > #1 0x00007ffff253d52a in abort () at /lib64/libc.so.6
> > > > #2 0x00007ffff253446d in __assert_fail_base () at /lib64/libc.so.6
> > > > #3 0x00007ffff2534522 in () at /lib64/libc.so.6
> > > > #4 0x00005555558bb80a in qemu_opt_get_bool_helper
> > > > (opts=0x55555621b650, name=name@entry=0x5555558ec922 "mem-merge",
> > > > defval=defval@entry=true, del=del@entry=false) at
> > > > qemu/util/qemu-option.c:388 #5 0x00005555558bbb5a in
> > > > qemu_opt_get_bool (opts=<optimized out>,
> > > > name=name@entry=0x5555558ec922 "mem-merge", defval=defval@entry=true)
> > > > at qemu/util/qemu-option.c:398 #6 0x0000555555720a24 in
> > > > host_memory_backend_init (obj=0x5555562ac970) at
> > > > qemu/backends/hostmem.c:226
> > > >
> > > > Instead of using qemu_opt_get_bool(), that didn't work with
> > > > qemu_machine_opts for a long time, we can use the machine QOM
> > > > properties directly.
> > > >
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > > backends/hostmem.c | 9 +++++----
> > > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > > > index 61c1ac0..38a32ed 100644
> > > > --- a/backends/hostmem.c
> > > > +++ b/backends/hostmem.c
> > > > @@ -10,6 +10,7 @@
> > > > * See the COPYING file in the top-level directory.
> > > > */
> > > > #include "sysemu/hostmem.h"
> > > > +#include "hw/boards.h"
> > > > #include "qapi/visitor.h"
> > > > #include "qapi-types.h"
> > > > #include "qapi-visit.h"
> > > > @@ -223,10 +224,10 @@ static void host_memory_backend_init(Object
> > > > *obj) {
> > > > HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > > >
> > > > - backend->merge = qemu_opt_get_bool(qemu_get_machine_opts(),
> > > > - "mem-merge", true);
> > > > - backend->dump = qemu_opt_get_bool(qemu_get_machine_opts(),
> > > > - "dump-guest-core", true);
> > > > + backend->merge =
> > > > object_property_get_bool(OBJECT(current_machine),
> > > maybe use qdev_get_machine() instead of OBJECT(current_machine)
> >
> > What are the advantages you see in the extra layers of indirection of
> > qdev_get_machine()? (I am not against your proposal, but I would like to
> > understand the point of qdev_get_machine() yet.)
> current_machine might be NULL where as qdev_get_machine() always returns
> /machine object.
In this case, I don't want "/machine" to be created as a generic
container object by accident because it must be a MachineState object.
If /machine is not initialized yet, that code should abort instead of
trying to continue.
But that can be solved by simply ensuring qdev_get_machine() is a
TYPE_MACHINE object (see below).
>
> >
> > I'd prefer to use something that is guaranteed to be MachineState*,
> > qdev_get_machine() returns Object*. I am even considering using
> > current_machine->mem_merge and current_machine->dump_guest_core directly
> > instead of object_property_get_bool(). That would mean extra
> > compile-time checks, instead of runtime ones.
> Check difference 'git grep qdev_get_machine' vs 'git grep current_machine'.
> I was under impression that policy was trying no to use globals unless one has to,
> and not introduce new usage in presence of other means to get object.
>
I will send a new version using:
MachineState *machine = MACHINE(qdev_get_machine());
backend->merge = machine_mem_merge(machine);
backend->dump = machine_dump_guest_core(machine);
Then we have no global variable, type safety, and no direct access to
struct fields.
--
Eduardo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init()
2015-07-20 20:03 ` Eduardo Habkost
@ 2015-07-20 20:06 ` Marcel Apfelbaum
0 siblings, 0 replies; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-07-20 20:06 UTC (permalink / raw)
To: Eduardo Habkost, Igor Mammedov
Cc: Paolo Bonzini, qemu-devel, Marcel Apfelbaum
On 07/20/2015 11:03 PM, Eduardo Habkost wrote:
> On Mon, Jul 20, 2015 at 09:06:28AM +0200, Igor Mammedov wrote:
>> On Fri, 17 Jul 2015 17:33:55 -0300
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>>> On Thu, Jul 16, 2015 at 11:02:14PM +0200, Igor Mammedov wrote:
>>>> On Thu, 16 Jul 2015 17:39:17 -0300
>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>
>>>>> This fixes the following crash, introduced by commit
>>>>> 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6:
>>>>>
>>>>> $ gdb --args qemu-system-x86_64 -machine pc,mem-merge=off -object
>>>>> memory-backend-ram,id=ram-node0,size=1024 [...]
>>>>> Program received signal SIGABRT, Aborted.
>>>>> (gdb) bt
>>>>> #0 0x00007ffff253b8c7 in raise () at /lib64/libc.so.6
>>>>> #1 0x00007ffff253d52a in abort () at /lib64/libc.so.6
>>>>> #2 0x00007ffff253446d in __assert_fail_base () at /lib64/libc.so.6
>>>>> #3 0x00007ffff2534522 in () at /lib64/libc.so.6
>>>>> #4 0x00005555558bb80a in qemu_opt_get_bool_helper
>>>>> (opts=0x55555621b650, name=name@entry=0x5555558ec922 "mem-merge",
>>>>> defval=defval@entry=true, del=del@entry=false) at
>>>>> qemu/util/qemu-option.c:388 #5 0x00005555558bbb5a in
>>>>> qemu_opt_get_bool (opts=<optimized out>,
>>>>> name=name@entry=0x5555558ec922 "mem-merge", defval=defval@entry=true)
>>>>> at qemu/util/qemu-option.c:398 #6 0x0000555555720a24 in
>>>>> host_memory_backend_init (obj=0x5555562ac970) at
>>>>> qemu/backends/hostmem.c:226
>>>>>
>>>>> Instead of using qemu_opt_get_bool(), that didn't work with
>>>>> qemu_machine_opts for a long time, we can use the machine QOM
>>>>> properties directly.
>>>>>
>>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>> ---
>>>>> backends/hostmem.c | 9 +++++----
>>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>>>>> index 61c1ac0..38a32ed 100644
>>>>> --- a/backends/hostmem.c
>>>>> +++ b/backends/hostmem.c
>>>>> @@ -10,6 +10,7 @@
>>>>> * See the COPYING file in the top-level directory.
>>>>> */
>>>>> #include "sysemu/hostmem.h"
>>>>> +#include "hw/boards.h"
>>>>> #include "qapi/visitor.h"
>>>>> #include "qapi-types.h"
>>>>> #include "qapi-visit.h"
>>>>> @@ -223,10 +224,10 @@ static void host_memory_backend_init(Object
>>>>> *obj) {
>>>>> HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>>>>>
>>>>> - backend->merge = qemu_opt_get_bool(qemu_get_machine_opts(),
>>>>> - "mem-merge", true);
>>>>> - backend->dump = qemu_opt_get_bool(qemu_get_machine_opts(),
>>>>> - "dump-guest-core", true);
>>>>> + backend->merge =
>>>>> object_property_get_bool(OBJECT(current_machine),
>>>> maybe use qdev_get_machine() instead of OBJECT(current_machine)
>>>
>>> What are the advantages you see in the extra layers of indirection of
>>> qdev_get_machine()? (I am not against your proposal, but I would like to
>>> understand the point of qdev_get_machine() yet.)
>> current_machine might be NULL where as qdev_get_machine() always returns
>> /machine object.
>
> In this case, I don't want "/machine" to be created as a generic
> container object by accident because it must be a MachineState object.
> If /machine is not initialized yet, that code should abort instead of
> trying to continue.
>
> But that can be solved by simply ensuring qdev_get_machine() is a
> TYPE_MACHINE object (see below).
>
>>
>>>
>>> I'd prefer to use something that is guaranteed to be MachineState*,
>>> qdev_get_machine() returns Object*. I am even considering using
>>> current_machine->mem_merge and current_machine->dump_guest_core directly
>>> instead of object_property_get_bool(). That would mean extra
>>> compile-time checks, instead of runtime ones.
>> Check difference 'git grep qdev_get_machine' vs 'git grep current_machine'.
>> I was under impression that policy was trying no to use globals unless one has to,
>> and not introduce new usage in presence of other means to get object.
>>
>
> I will send a new version using:
>
> MachineState *machine = MACHINE(qdev_get_machine());
> backend->merge = machine_mem_merge(machine);
> backend->dump = machine_dump_guest_core(machine);
>
> Then we have no global variable, type safety, and no direct access to
> struct fields.
+1
I am considering retiring curent_machine global variable for 2.5.
Thanks,
Marcel
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-07-20 20:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-16 20:39 [Qemu-devel] [PATCH] hostmem: Fix qemu_opt_get_bool() crash in host_memory_backend_init() Eduardo Habkost
2015-07-16 21:02 ` Igor Mammedov
2015-07-17 20:33 ` Eduardo Habkost
2015-07-20 7:06 ` Igor Mammedov
2015-07-20 20:03 ` Eduardo Habkost
2015-07-20 20:06 ` Marcel Apfelbaum
2015-07-17 7:21 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).