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

[Steven Sistare]

On 2/14/2025 11:37 AM, Peter Xu wrote:

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.sist...@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?

Bad, cpr will likely fail the next time it is used.
I suppose I could add a blocker instead of using error_fatal.
But, fundamentally something unknown has gone wrong, like for
any assertion failure, so continuing to run in an uncertain
state seems unwise.

I have only ever seen this during development after adding buggy code.

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.

Yes, I could check for dups in cpr_save_fd, though it would cost O(N) instead
of O(1).  That seems like overkill for a bug that should only bite during new
code development.

cpr_resave_fd is O(N), but not for error checking.  Callers use it when they
know the fd was (or may have been) already created.  It is a programming
convenience that simplifies the call sites.

+    }
+}
+
+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.

OK, I'll add a function header comment.

+
  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.

OK.

- Steve

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