[Qemu-devel] [PATCH v2] migration: Allow unregister of save_live handlers

[Qemu-devel] [PATCH v2] migration: Allow unregister of save_live handlers

[Juan Quintela]

Migration non save_live handlers have an ops member that is
dinamically allocated by migration code.  Save_live handlers have it
passed as argument and are responsability of the caller.  Add a new
member is_allocated that remembers if ops has to be freed.  This
allows unregister_savevm() to work with save_live handlers.

Signed-off-by: Juan Quintela <quintela@redhat.com>

---

check that se->ops is not NULL (thanks peterx)
---
 include/migration/vmstate.h | 2 ++
 migration/savevm.c          | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

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..569add3 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 && se->ops->is_allocated) {
+                g_free(se->ops);
+            }
             g_free(se);
         }
     }
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2] migration: Allow unregister of save_live handlers

[Peter Xu]

On Wed, May 24, 2017 at 01:21:52PM +0200, Juan Quintela wrote:
> Migration non save_live handlers have an ops member that is
> dinamically allocated by migration code.  Save_live handlers have it
> passed as argument and are responsability of the caller.  Add a new
> member is_allocated that remembers if ops has to be freed.  This
> allows unregister_savevm() to work with save_live handlers.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

I thought Laurent will prepare one patch that will directly remove
existing register_savevm() callers, no?

> 
> ---
> 
> check that se->ops is not NULL (thanks peterx)
> ---
>  include/migration/vmstate.h | 2 ++
>  migration/savevm.c          | 5 ++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> 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..569add3 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 && se->ops->is_allocated) {
> +                g_free(se->ops);
> +            }
>              g_free(se);
>          }
>      }
> -- 
> 2.9.3
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2] migration: Allow unregister of save_live handlers

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, May 24, 2017 at 01:21:52PM +0200, Juan Quintela wrote:
>> Migration non save_live handlers have an ops member that is
>> dinamically allocated by migration code.  Save_live handlers have it
>> passed as argument and are responsability of the caller.  Add a new
>> member is_allocated that remembers if ops has to be freed.  This
>> allows unregister_savevm() to work with save_live handlers.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> I thought Laurent will prepare one patch that will directly remove
> existing register_savevm() callers, no?

s390, vmxnet3 and slirp are using it.  I am not sure that we will get
that ones "changed" so easily.

Later, Juan.

Re: [Qemu-devel] [PATCH v2] migration: Allow unregister of save_live handlers

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Wed, May 24, 2017 at 01:21:52PM +0200, Juan Quintela wrote:
> >> Migration non save_live handlers have an ops member that is
> >> dinamically allocated by migration code.  Save_live handlers have it
> >> passed as argument and are responsability of the caller.  Add a new
> >> member is_allocated that remembers if ops has to be freed.  This
> >> allows unregister_savevm() to work with save_live handlers.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >
> > I thought Laurent will prepare one patch that will directly remove
> > existing register_savevm() callers, no?
> 
> s390, vmxnet3 and slirp are using it.  I am not sure that we will get
> that ones "changed" so easily.

I think Halil is working on s390,  slirp has a tricky loop I need to
look at again.  And vmxnet3 is...odd.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK