Re: [PATCH v2 02/10] migration/rdma: Unfold ram_control_before_iterate()

[Fabiano Rosas <farosas@suse.de>]

Juan Quintela <quintela@redhat.com> writes:

> Once there:
> - Remove unused data parameter
> - unfold it in its callers.
> - change all callers to call qemu_rdma_registration_start()
> - We need to call QIO_CHANNEL_RDMA() after we check for migrate_rdma()
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> --
>
> initilazize rioc after checknig that rdma is enabled.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.h |  2 --
>  migration/rdma.h      |  7 +++++++
>  migration/qemu-file.c | 13 +------------
>  migration/ram.c       | 16 +++++++++++++---
>  migration/rdma.c      | 12 ++++--------
>  5 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 03e718c264..d6a370c569 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -55,7 +55,6 @@ typedef int (QEMURamSaveFunc)(QEMUFile *f,
>                                size_t size);
>  
>  typedef struct QEMUFileHooks {
> -    QEMURamHookFunc *before_ram_iterate;
>      QEMURamHookFunc *after_ram_iterate;
>      QEMURamHookFunc *hook_ram_load;
>      QEMURamSaveFunc *save_page;
> @@ -127,7 +126,6 @@ void qemu_fflush(QEMUFile *f);
>  void qemu_file_set_blocking(QEMUFile *f, bool block);
>  int qemu_file_get_to_fd(QEMUFile *f, int fd, size_t size);
>  
> -void ram_control_before_iterate(QEMUFile *f, uint64_t flags);
>  void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
>  void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
>  
> diff --git a/migration/rdma.h b/migration/rdma.h
> index de2ba09dc5..670c67a8cb 100644
> --- a/migration/rdma.h
> +++ b/migration/rdma.h
> @@ -22,4 +22,11 @@ void rdma_start_outgoing_migration(void *opaque, const char *host_port,
>  
>  void rdma_start_incoming_migration(const char *host_port, Error **errp);
>  
> +
> +#ifdef CONFIG_RDMA
> +int qemu_rdma_registration_start(QEMUFile *f, uint64_t flags);
> +#else
> +static inline
> +int qemu_rdma_registration_start(QEMUFile *f, uint64_t flags) { return 0; }
> +#endif
>  #endif
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 5e8207dae4..1a8170421f 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -32,6 +32,7 @@
>  #include "trace.h"
>  #include "options.h"
>  #include "qapi/error.h"
> +#include "rdma.h"
>  
>  #define IO_BUF_SIZE 32768
>  #define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)
> @@ -288,18 +289,6 @@ void qemu_fflush(QEMUFile *f)
>      f->iovcnt = 0;
>  }
>  
> -void ram_control_before_iterate(QEMUFile *f, uint64_t flags)
> -{
> -    int ret = 0;
> -
> -    if (f->hooks && f->hooks->before_ram_iterate) {
> -        ret = f->hooks->before_ram_iterate(f, flags, NULL);
> -        if (ret < 0) {
> -            qemu_file_set_error(f, ret);
> -        }
> -    }
> -}
> -
>  void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
>  {
>      int ret = 0;
> diff --git a/migration/ram.c b/migration/ram.c
> index e4bfd39f08..407760b3a2 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -59,6 +59,7 @@
>  #include "qemu/iov.h"
>  #include "multifd.h"
>  #include "sysemu/runstate.h"
> +#include "rdma.h"
>  #include "options.h"
>  #include "sysemu/dirtylimit.h"
>  #include "sysemu/kvm.h"
> @@ -3054,7 +3055,10 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>          }
>      }
>  
> -    ram_control_before_iterate(f, RAM_CONTROL_SETUP);
> +    ret = qemu_rdma_registration_start(f, RAM_CONTROL_SETUP);
> +    if (ret < 0) {
> +        qemu_file_set_error(f, ret);

Markus' patch 23 will turn the return code from
qemu_rdma_registration_start() from -EIO into -1. Any code that uses
strerr to report it will now see an EPERM (-1).

We should someday give an Error argument to savevm functions and stop
using QEMUFile as an error carrier. Or perhaps add an Error pointer to
RAMState?

Anyway, out scope for this patch.

Reviewed-by: Fabiano Rosas <farosas@suse.de>