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

[Steven Sistare]

On 2/18/2025 11:26 AM, Peter Xu wrote:

On Fri, Feb 14, 2025 at 03:31:29PM -0500, Steven Sistare wrote:

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.

If the caller know the fd was created, then IIUC the caller shouldn't
invoke the call.

For the other case, could you give an example when the caller may have been
created, but maybe not?  I'm a bit surprised we have such use case.

It avoids the need to remember that an fd was reused, and test that fact before
calling cpr_save_fd.  And sometimes those operations occur in different 
functions.
Thus resave saves a few lines of code.  Trivial, though.  I will just delete
cpr_resave_fd.

- Steve