Re: [Qemu-devel] [PATCH v3 1/3] migration: Introduce unregister_savevm_live()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3426 bytes --]

On Wed, May 24, 2017 at 08:28:59AM +0200, Juan Quintela wrote:
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > Introduce a new function unregister_savevm_live() to unregister the vmstate
> > handlers registered via register_savevm_live().
> >
> > register_savevm() allocates SaveVMHandlers while register_savevm_live()
> > gets passed with SaveVMHandlers. During unregistration, we  want to
> > free SaveVMHandlers in the former case but not free in the latter case.
> > Hence this new API is needed to differentiate this.
> >
> > This new API will be needed by PowerPC to unregister the HTAB savevm
> > handlers.
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Hi
> 
> How about this one?
> I just test compiled it.
> 
> Advantage from my point of view is that we always do the right thing.
> And as migration code already knows if it has to be freed or not, I
> think it is a better API.
> 
> What do you think?

I think this is a better approach.  Do you want to push this one
directly, Juan, or do you want me to take it through my tree?

> 
> Later, Juan.
> 
> commit 6e71fac7a9c29cef9625135c58ce59ccfbb3b86f
> Author: Juan Quintela <quintela@redhat.com>
> Date:   Wed May 24 08:25:06 2017 +0200
> 
>     migration: Allow to free save_live handlers
>     
>     Migration save_live handlers have an ops member that is not dynamic.
>     Add a new member is_allocated that remembers if ops has to be freed.
>     
>     Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index f97411d..1d20e30 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -57,6 +57,8 @@ typedef struct SaveVMHandlers {
>                                uint64_t *non_postcopiable_pending,
>                                uint64_t *postcopiable_pending);
>      LoadStateHandler *load_state;
> +    /* Has been allocated by migratation code */
> +    bool is_allocated;
>  } SaveVMHandlers;
>  
>  int register_savevm(DeviceState *dev,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d971e5e..187f386 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -628,6 +628,7 @@ int register_savevm(DeviceState *dev,
>      SaveVMHandlers *ops = g_new0(SaveVMHandlers, 1);
>      ops->save_state = save_state;
>      ops->load_state = load_state;
> +    ops->is_allocated = true;
>      return register_savevm_live(dev, idstr, instance_id, version_id,
>                                  ops, opaque);
>  }
> @@ -651,7 +652,9 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
>          if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) {
>              QTAILQ_REMOVE(&savevm_state.handlers, se, entry);
>              g_free(se->compat);
> -            g_free(se->ops);
> +            if (se->ops->is_allocated) {
> +                g_free(se->ops);
> +            }
>              g_free(se);
>          }
>      }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]