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

[Steven Sistare <steven.sistare@oracle.com>]

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.

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