Re: [Qemu-devel] [PATCH v1 04/22] migration: split migration hooks out of QEMUFileOps

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> The QEMUFileOps struct contains the I/O subsystem callbacks
> and the migration stage hooks. Split the hooks out into a
> separate QEMUFileHooks struct to make it easier to refactor
> the I/O side of QEMUFile without affecting the hooks.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/migration/qemu-file.h  | 10 +++++++---
>  migration/qemu-file-internal.h |  1 +
>  migration/qemu-file.c          | 24 +++++++++++++++---------
>  migration/rdma.c               |  8 ++++++++
>  4 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 5debe8c..ddffc6a 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -109,13 +109,16 @@ typedef struct QEMUFileOps {
>      QEMUFileCloseFunc *close;
>      QEMUFileGetFD *get_fd;
>      QEMUFileWritevBufferFunc *writev_buffer;
> +    QEMURetPathFunc *get_return_path;
> +    QEMUFileShutdownFunc *shut_down;
> +} QEMUFileOps;
> +
> +typedef struct QEMUFileHooks {
>      QEMURamHookFunc *before_ram_iterate;
>      QEMURamHookFunc *after_ram_iterate;
>      QEMURamHookFunc *hook_ram_load;
>      QEMURamSaveFunc *save_page;
> -    QEMURetPathFunc *get_return_path;
> -    QEMUFileShutdownFunc *shut_down;
> -} QEMUFileOps;
> +} QEMUFileHooks;
>  
>  struct QEMUSizedBuffer {
>      struct iovec *iov;
> @@ -130,6 +133,7 @@ QEMUFile *qemu_fdopen(int fd, const char *mode);
>  QEMUFile *qemu_fopen_socket(int fd, const char *mode);
>  QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
>  QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input);
> +void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks);
>  int qemu_get_fd(QEMUFile *f);
>  int qemu_fclose(QEMUFile *f);
>  int64_t qemu_ftell(QEMUFile *f);
> diff --git a/migration/qemu-file-internal.h b/migration/qemu-file-internal.h
> index d95e853..8fdfa95 100644
> --- a/migration/qemu-file-internal.h
> +++ b/migration/qemu-file-internal.h
> @@ -33,6 +33,7 @@
>  
>  struct QEMUFile {
>      const QEMUFileOps *ops;
> +    const QEMUFileHooks *hooks;
>      void *opaque;
>  
>      int64_t bytes_xfer;
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index f89e64e..337cb4c 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -79,6 +79,12 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
>      return f;
>  }
>  
> +
> +void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
> +{
> +    f->hooks = hooks;
> +}
> +
>  /*
>   * Get last error for stream f
>   *
> @@ -148,8 +154,8 @@ void ram_control_before_iterate(QEMUFile *f, uint64_t flags)
>  {
>      int ret = 0;
>  
> -    if (f->ops->before_ram_iterate) {
> -        ret = f->ops->before_ram_iterate(f, f->opaque, flags, NULL);
> +    if (f->hooks && f->hooks->before_ram_iterate) {
> +        ret = f->hooks->before_ram_iterate(f, f->opaque, flags, NULL);
>          if (ret < 0) {
>              qemu_file_set_error(f, ret);
>          }
> @@ -160,8 +166,8 @@ void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
>  {
>      int ret = 0;
>  
> -    if (f->ops->after_ram_iterate) {
> -        ret = f->ops->after_ram_iterate(f, f->opaque, flags, NULL);
> +    if (f->hooks && f->hooks->after_ram_iterate) {
> +        ret = f->hooks->after_ram_iterate(f, f->opaque, flags, NULL);
>          if (ret < 0) {
>              qemu_file_set_error(f, ret);
>          }
> @@ -172,8 +178,8 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data)
>  {
>      int ret = -EINVAL;
>  
> -    if (f->ops->hook_ram_load) {
> -        ret = f->ops->hook_ram_load(f, f->opaque, flags, data);
> +    if (f->hooks && f->hooks->hook_ram_load) {
> +        ret = f->hooks->hook_ram_load(f, f->opaque, flags, data);
>          if (ret < 0) {
>              qemu_file_set_error(f, ret);
>          }
> @@ -192,9 +198,9 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>                               ram_addr_t offset, size_t size,
>                               uint64_t *bytes_sent)
>  {
> -    if (f->ops->save_page) {
> -        int ret = f->ops->save_page(f, f->opaque, block_offset,
> -                                    offset, size, bytes_sent);
> +    if (f->hooks && f->hooks->save_page) {
> +        int ret = f->hooks->save_page(f, f->opaque, block_offset,
> +                                      offset, size, bytes_sent);
>  
>          if (ret != RAM_SAVE_CONTROL_DELAYED) {
>              if (bytes_sent && *bytes_sent > 0) {
> diff --git a/migration/rdma.c b/migration/rdma.c
> index dcabb91..bffbfaf 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -3380,12 +3380,18 @@ static const QEMUFileOps rdma_read_ops = {
>      .get_buffer    = qemu_rdma_get_buffer,
>      .get_fd        = qemu_rdma_get_fd,
>      .close         = qemu_rdma_close,
> +};
> +
> +static const QEMUFileHooks rdma_read_hooks = {
>      .hook_ram_load = rdma_load_hook,
>  };
>  
>  static const QEMUFileOps rdma_write_ops = {
>      .put_buffer         = qemu_rdma_put_buffer,
>      .close              = qemu_rdma_close,
> +};
> +
> +static const QEMUFileHooks rdma_write_hooks = {
>      .before_ram_iterate = qemu_rdma_registration_start,
>      .after_ram_iterate  = qemu_rdma_registration_stop,
>      .save_page          = qemu_rdma_save_page,
> @@ -3404,8 +3410,10 @@ static void *qemu_fopen_rdma(RDMAContext *rdma, const char *mode)
>  
>      if (mode[0] == 'w') {
>          r->file = qemu_fopen_ops(r, &rdma_write_ops);
> +        qemu_file_set_hooks(r->file, &rdma_write_hooks);
>      } else {
>          r->file = qemu_fopen_ops(r, &rdma_read_ops);
> +        qemu_file_set_hooks(r->file, &rdma_read_hooks);
>      }
>  
>      return r->file;
> -- 
> 2.5.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK