Re: [Qemu-devel] [RFC PATCH v1 2/6] migration: Introduce unregister_savevm_live()

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

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

On Wed, May 17, 2017 at 09:19:18AM +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 <bharata@linux.vnet.ibm.com>

It's not my bailiwick, so I don't get final say, but I dislike
changing the signature of the existing unregister_savevm() interface.
I think it would be preferable to only add the 'live' paramter to a
new unregister_savevm_common() (or whatever) function.

> ---
>  hw/net/vmxnet3.c            |  2 +-
>  hw/s390x/s390-skeys.c       |  2 +-
>  include/migration/vmstate.h |  4 +++-
>  migration/savevm.c          | 12 ++++++++++--
>  slirp/slirp.c               |  2 +-
>  5 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 8b1fab2..2b923be 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2350,7 +2350,7 @@ static void vmxnet3_pci_uninit(PCIDevice *pci_dev)
>  
>      VMW_CBPRN("Starting uninit...");
>  
> -    unregister_savevm(dev, "vmxnet3-msix", s);
> +    unregister_savevm(dev, "vmxnet3-msix", s, false);
>  
>      vmxnet3_net_uninit(s);
>  
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index e2d4e1a..32b6435 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -379,7 +379,7 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
>          register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save,
>                          s390_storage_keys_load, ss);
>      } else {
> -        unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss);
> +        unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss, false);
>      }
>  }
>  
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index f4bf3f1..ba81b3e 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -78,7 +78,9 @@ int register_savevm_live(DeviceState *dev,
>                           SaveVMHandlers *ops,
>                           void *opaque);
>  
> -void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque);
> +void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque,
> +                       bool live);
> +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 7a268ec..fa7c3db 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)
> +void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque,
> +                       bool live)
>  {
>      SaveStateEntry *se, *new_se;
>      char id[256] = "";
> @@ -651,12 +652,19 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
>              if (dev) {
>                  g_free(se->compat);
>              }
> -            g_free(se->ops);
> +            if (!live) {
> +                g_free(se->ops);
> +            }
>              g_free(se);
>          }
>      }
>  }
>  
> +void unregister_savevm_live(DeviceState *dev, const char *idstr, void *opaque)
> +{
> +    unregister_savevm(dev, idstr, opaque, true);
> +}
> +
>  int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *opaque, int alias_id,
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 2f2ec2c..108e669 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -333,7 +333,7 @@ void slirp_cleanup(Slirp *slirp)
>  {
>      QTAILQ_REMOVE(&slirp_instances, slirp, entry);
>  
> -    unregister_savevm(NULL, "slirp", slirp);
> +    unregister_savevm(NULL, "slirp", slirp, false);
>  
>      ip_cleanup(slirp);
>      ip6_cleanup(slirp);

-- 
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 --]