Re: [Qemu-devel] Bug in recent postcopy patch

[Dr. David Alan Gilbert]

* Gary Hook (address@hidden) wrote:
> *Knock* *knock* *knock* Is this thing on?

Yes - but only by luck did I notice this; it's normally better
to reply to the thread that posted a patch and cc the authors!

> I applied the 47 pieces of the recent postcopy patch to 2.1.2 and am
> poking around. An attempt to migrate results in a NULL pointer dereference
> in savevm.c.  Here is info from gdb:

I've not tried migrating with block migration; so can you
show the command line you used on qemu and the sequence of commands
you used to trigger the migration?

> Most of qemu_savevm_state_pending() succeeds, until it gets to the end.
> Here¹s the relevant thread while calling is_active():
> 
> (gdb) backtrace
> #0  block_is_active (opaque=0x7fb0ae721200 <block_mig_state>) at
> block-migration.c:860
> #1  0x00007fb0adf4a13a in qemu_savevm_state_pending (f=0x7fb0b01e3a40,
> address@hidden,
>     address@hidden,
> address@hidden)
>     at /home/hook/src/qemu/postcopy2/savevm.c:983
> #2  0x00007fb0ae01bd82 in migration_thread (opaque=0x7fb0ae684420
> <current_migration>) at migration.c:1185
> #3  0x00007fb0a824d182 in start_thread (arg=0x7fb09d605700) at
> pthread_create.c:312
> #4  0x00007fb0a7f79fbd in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
> 
> Q: why is max_size == 0? Does this seem correct?

Yes, I think that's normal for the 1st time through the loop; (see 
migration_thread
near the start max_size is initialised to 0).

> We look at se->ops:
> 
> (gdb) print *se->ops
> $9 = {set_params = 0x7fb0ae028820 <block_set_params>, save_state = 0x0,
> cancel = 0x7fb0ae028f50 <block_migration_cancel>,
>   save_live_complete = 0x7fb0ae0299a0 <block_save_complete>, is_active =
> 0x7fb0ae028870 <block_is_active>,
>   save_live_iterate = 0x7fb0ae029480 <block_save_iterate>, save_live_setup
> = 0x7fb0ae029330 <block_save_setup>,
>   save_live_pending = 0x7fb0ae028b30 <block_save_pending>, can_postcopy =
> 0x0, load_state = 0x7fb0ae0288b0 <block_load>}
> 
> Why is can_postcopy() NULL?
> 
> (gdb) n
> qemu_savevm_state_pending (f=0x7fb0b01e3a40, address@hidden,
> address@hidden,
>     address@hidden) at
> /home/hook/src/qemu/postcopy2/savevm.c:989
> 989           if (se->ops->can_postcopy(se->opaque)) {
> (gdb) print *se
> $14 = {entry = {tqe_next = 0x7fb0aff9ab30, tqe_prev = 0x7fb0aff88f20},
> idstr = "block", '\000' <repeats 250 times>, instance_id = 0,
>   alias_id = 0, version_id = 1, section_id = 1, ops = 0x7fb0ae6848e0
> <savevm_block_handlers>, vmsd = 0x0,
>   opaque = 0x7fb0ae721200 <block_mig_state>, compat = 0x0, is_ram = 1}
> (gdb) step
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000000000 in ?? ()
> (gdb) 
> 
> 
> The patches appear to have been fully applied, but it would seem that the
> savevm_block_handlers structure needs to be updated to populate this
> field? Which implies that a new function will have to be written?
> 
> Or, if I have missed the obvious, I would appreciate enlightenment.

Simple bug on my part; the line:

        if (se->ops->can_postcopy(se->opaque)) {

needs to become:
        if (se->ops->can_postcopy &&
            se->ops->can_postcopy(se->opaque)) {

Thanks for the report.

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK