Re: [PATCH V2 02/45] migration: cpr helpers

[Peter Xu <peterx@redhat.com>]

On Fri, Feb 14, 2025 at 06:13:44AM -0800, Steve Sistare wrote:
> Add cpr_needed_for_reuse, cpr_resave_fd helpers, cpr_is_incoming, and
> cpr_open_fd, for use when adding cpr support for vfio and iommufd.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  include/migration/cpr.h |  6 ++++++
>  migration/cpr.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
> index 3a6deb7..6ad04d4 100644
> --- a/include/migration/cpr.h
> +++ b/include/migration/cpr.h
> @@ -18,15 +18,21 @@
>  void cpr_save_fd(const char *name, int id, int fd);
>  void cpr_delete_fd(const char *name, int id);
>  int cpr_find_fd(const char *name, int id);
> +void cpr_resave_fd(const char *name, int id, int fd);
> +int cpr_open_fd(const char *path, int flags, const char *name, int id,
> +                bool *reused, Error **errp);
>  
>  MigMode cpr_get_incoming_mode(void);
>  void cpr_set_incoming_mode(MigMode mode);
> +bool cpr_is_incoming(void);
>  
>  int cpr_state_save(MigrationChannel *channel, Error **errp);
>  int cpr_state_load(MigrationChannel *channel, Error **errp);
>  void cpr_state_close(void);
>  struct QIOChannel *cpr_state_ioc(void);
>  
> +bool cpr_needed_for_reuse(void *opaque);
> +
>  QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp);
>  QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp);
>  
> diff --git a/migration/cpr.c b/migration/cpr.c
> index 584b0b9..12c489b 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -95,6 +95,39 @@ int cpr_find_fd(const char *name, int id)
>      trace_cpr_find_fd(name, id, fd);
>      return fd;
>  }
> +
> +void cpr_resave_fd(const char *name, int id, int fd)
> +{
> +    CprFd *elem = find_fd(&cpr_state.fds, name, id);
> +    int old_fd = elem ? elem->fd : -1;
> +
> +    if (old_fd < 0) {
> +        cpr_save_fd(name, id, fd);
> +    } else if (old_fd != fd) {
> +        error_setg(&error_fatal,
> +                   "internal error: cpr fd '%s' id %d value %d "
> +                   "already saved with a different value %d",
> +                   name, id, fd, old_fd);

How bad it is to trigger this?

I wonder if cpr_save_fd() should have checked this already on duplicated
entries; it looks risky there too if this happens to existing cpr_save_fd()
callers.

> +    }
> +}
> +
> +int cpr_open_fd(const char *path, int flags, const char *name, int id,
> +                bool *reused, Error **errp)
> +{
> +    int fd = cpr_find_fd(name, id);
> +
> +    if (reused) {
> +        *reused = (fd >= 0);
> +    }
> +    if (fd < 0) {
> +        fd = qemu_open(path, flags, errp);
> +        if (fd >= 0) {
> +            cpr_save_fd(name, id, fd);
> +        }
> +    }
> +    return fd;
> +}
> +
>  /*************************************************************************/
>  #define CPR_STATE "CprState"
>  
> @@ -128,6 +161,11 @@ void cpr_set_incoming_mode(MigMode mode)
>      incoming_mode = mode;
>  }
>  
> +bool cpr_is_incoming(void)
> +{
> +    return incoming_mode != MIG_MODE_NONE;
> +}

Maybe it'll be helpful to document either this function or incoming_mode;
it's probably not yet obvious to most readers incoming_mode is only set to
!NONE during a small window when VM loads.

> +
>  int cpr_state_save(MigrationChannel *channel, Error **errp)
>  {
>      int ret;
> @@ -222,3 +260,9 @@ void cpr_state_close(void)
>          cpr_state_file = NULL;
>      }
>  }
> +
> +bool cpr_needed_for_reuse(void *opaque)
> +{
> +    MigMode mode = migrate_mode();

Nit: can drop the var.

> +    return mode == MIG_MODE_CPR_TRANSFER;
> +}
> -- 
> 1.8.3.1
> 

-- 
Peter Xu