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,
--
Peter Xu
