Re: [Qemu-devel] [PATCH] Fix segfault after migration completes

[Eduardo Habkost]

On Fri, Oct 28, 2011 at 12:58:04PM -0200, Luiz Capitulino wrote:
[...]
> 
> So, 's->file' is NULL in migrate_fd_put_notify(). The interesting thing
> is that it's valid in the qemu_file_put_notify() call, which makes me
> think that either: there's a race somewhere or qemu_file_put_notify() is
> itself clearing 's->file'. In both cases the fix below could just be hiding
> the real issue, but let's get started...
> 
> Signed-off-by: Luiz Capitulino <address@hidden>

Acked-by: Eduardo Habkost <address@hidden>

However, it looks like the error-check interface of QEMUFile is really
easy to misuse, and can be improved:

- Either errors are always triggered synchronously inside
  qemu_file_put_notify(), or they can be triggered asynchronously
  elsewhere too.
- If they are always triggered synchronously during the
  qemu_file_put_notify() call, then qemu_file_put_notify() should return
  error information itself instead of requiring a qemu_file_get_error()
  call.
- If errors can be triggered asynchronously, then we need an error
  notification mechanism that makes sure no error is ever missed,
  instead of this error check on migrate_fd_put_notify().

> ---
>  migration.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index bdca72e..f6e6208 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -252,7 +252,7 @@ static void migrate_fd_put_notify(void *opaque)
>  
>      qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>      qemu_file_put_notify(s->file);
> -    if (qemu_file_get_error(s->file)) {
> +    if (s->file && qemu_file_get_error(s->file)) {
>          migrate_fd_error(s);
>      }
>  }
> -- 
> 1.7.7.1.488.ge8e1c.dirty
> 

-- 
Eduardo