On 9/29/26 6:37 PM, Peter Xu wrote:
> On Tue, Sep 29, 2026 at 04:28:19PM +0300, Andrey Drobyshev wrote:
>>> But I do see the existance of cpr_resave_fd(). I confess I don't know why
>>> it existed and didn't check. That looks exactly like sometimes we can save
>>> the same fd twice, if the 1st one is fd=-1. Any quick answer?
>>
>> AFAICT the sole purpose of cpr_resave_fd() is to be called from
>> different sites with the same (name, id, fd), to make sure the key is
>> registered in the table. What it does:
>>
>> - no entry - save it into hash table;
>> - same FD already there - do nothing, table isn't updated;
>> - different FD present - abort as an internal error.
>>
>> That means there's no case of injecting the same (name, id) twice into
>> the hash table. That is actually consistent with our patch here.
>>
>> I'd leave the code as-is and only reword the comment, if that's fine
>> with you.
>
> The whole cpr_resave_fd() API is confusing, I still don't understand why we
> need it: we should make sure the fd is always properly inserted, and only
> once somehow.. but I agree it's not an issue for the current patch.
>
> Said that, for this patch, IIUC we still should need something like this:
>
> diff --git a/migration/cpr.c b/migration/cpr.c
> index bca43e9bf3..d26656c5c1 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -173,14 +173,13 @@ int cpr_find_fd(const char *name, int id)
> void cpr_resave_fd(const char *name, int id, int fd)
> {
> CprFd *elem = find_fd(name, id);
> - int old_fd = elem ? elem->fd : -1;
>
> - if (old_fd < 0) {
> + if (!elem) {
> cpr_save_fd(name, id, fd);
> - } else if (old_fd != fd) {
> + } else if (elem->fd != fd) {
> error_report("internal error: cpr fd '%s' id %d value %d "
> "already saved with a different value %d",
> - name, id, fd, old_fd);
> + name, id, fd, elem->fd);
> g_assert_not_reached();
> }
> }
>
> Otherwise at least from API level some user can inject one fd=-1 entry,
> then resave fd with something else causing cpr_save_fd() done twice on the
> same (name, id) index, triggering the new assert.
>
> Thanks,
AFAIU this bug (injecting fd=-1) is non-existing at the moment, but
surely adding some API hardening won't hurt. I'll include your hunk in
the patch, thanks.
Andrey
