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

[Peter Xu <peterx@redhat.com>]

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

-- 
Peter Xu