| Message ID | 87a8627opg.fsf@secure.mitica |
|---|---|
| State | New |
| Headers | show |
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 <david@gibson.dropbear.id.au> wrote: > 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? I will push it on my next pull request. I mean, I will send it for review on own top level. Thanks, Juan.
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); } }