Re: [Qemu-devel] [PATCH v3 1/3] migration: Introduce unregister_savevm_live()
David Gibson wrote: > On Wed, May 24, 2017 at 08:28:59AM +0200, Juan Quintela wrote: >> Bharata B Rao 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 >> > Reviewed-by: David Gibson >> > Cc: Juan Quintela >> > Cc: Dr. David Alan Gilbert >> >> 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.
Re: [Qemu-devel] [PATCH v3 1/3] migration: Introduce unregister_savevm_live()
On Wed, May 24, 2017 at 08:28:59AM +0200, Juan Quintela wrote:
> Bharata B Rao 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
> > Reviewed-by: David Gibson
> > Cc: Juan Quintela
> > Cc: Dr. David Alan Gilbert
>
> 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
> 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
>
> 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
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v3 1/3] migration: Introduce unregister_savevm_live()
Bharata B Rao 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
> Reviewed-by: David Gibson
> Cc: Juan Quintela
> Cc: Dr. David Alan Gilbert
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?
Later, Juan.
commit 6e71fac7a9c29cef9625135c58ce59ccfbb3b86f
Author: Juan Quintela
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
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);
}
}
[Qemu-devel] [PATCH v3 1/3] migration: Introduce unregister_savevm_live()
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
Reviewed-by: David Gibson
Cc: Juan Quintela
Cc: Dr. David Alan Gilbert
---
include/migration/vmstate.h | 1 +
migration/savevm.c | 17 +++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 8489659..02a1bac 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -79,6 +79,7 @@ int register_savevm_live(DeviceState *dev,
void *opaque);
void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque);
+void unregister_savevm_live(DeviceState *dev, const char *idstr, void *opaque);
typedef struct VMStateInfo VMStateInfo;
typedef struct VMStateDescription VMStateDescription;
diff --git a/migration/savevm.c b/migration/savevm.c
index f5e8194..4ef6fdc 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -630,7 +630,8 @@ int register_savevm(DeviceState *dev,
ops, opaque);
}
-void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
+static void unregister_savevm_common(DeviceState *dev, const char *idstr,
+ void *opaque, bool free_savevmhandlers)
{
SaveStateEntry *se, *new_se;
char id[256] = "";
@@ -649,12 +650,24 @@ 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 (free_savevmhandlers) {
+g_free(se->ops);
+}
g_free(se);
}
}
}
+void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
+{
+unregister_savevm_common(dev, idstr, opaque, true);
+}
+
+void unregister_savevm_live(DeviceState *dev, const char *idstr, void *opaque)
+{
+unregister_savevm_common(dev, idstr, opaque, false);
+}
+
int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
const VMStateDescription *vmsd,
void *opaque, int alias_id,
--
2.7.4
