Re: [Qemu-devel] [RFC PATCH v2 1/4] migration: Introduce unregister_savevm_live()
On Fri, May 19, 2017 at 03:14:35PM +0200, Laurent Vivier wrote: > On 19/05/2017 09:33, David Gibson wrote: > > On Fri, May 19, 2017 at 11:10:36AM +0530, 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 > > > > I could take this through my tree, but it would need an ACK from Dave > > Gilbert or Juan Quintela. > > I cc: them for that. Dave, Juan: his is fairly urgent, since it's a prereq for (sanely) fixing a migration bug. Please let me know what I can do to expedite review. > > Just a comment on the patch. > > Instead of introducing a new function, perhaps we can homogenize the use > of register_savevm() by always providing a SaveVMHandlers pointer and > never a couple of (SaveStateHandler, LoadStateHandler) so the > unregister_save() has never to free se->ops? Sounds reasonable to me. Again, Dave? Juan? -- 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] [RFC PATCH v2 1/4] migration: Introduce unregister_savevm_live()
On 19/05/2017 09:33, David Gibson wrote: > On Fri, May 19, 2017 at 11:10:36AM +0530, 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 > > I could take this through my tree, but it would need an ACK from Dave > Gilbert or Juan Quintela. I cc: them for that. Just a comment on the patch. Instead of introducing a new function, perhaps we can homogenize the use of register_savevm() by always providing a SaveVMHandlers pointer and never a couple of (SaveStateHandler, LoadStateHandler) so the unregister_save() has never to free se->ops? Laurent
Re: [Qemu-devel] [RFC PATCH v2 1/4] migration: Introduce unregister_savevm_live()
On Fri, May 19, 2017 at 11:10:36AM +0530, 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
I could take this through my tree, but it would need an ACK from Dave
Gilbert or Juan Quintela.
> ---
> 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,
--
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
