| Message ID | 20240220224138.24759-31-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: File based migration with multifd and fixed-ram | expand |
Fabiano Rosas <farosas@suse.de> writes: > We're currently only removing an fd from the fdset if the VM is > running. This causes a QMP call to "remove-fd" to not actually remove > the fd if the VM happens to be stopped. > > While the fd would eventually be removed when monitor_fdset_cleanup() > is called again, the user request should be honored and the fd > actually removed. Calling remove-fd + query-fdset shows a recently > removed fd still present. > > The runstate_is_running() check was introduced by commit ebe52b592d > ("monitor: Prevent removing fd from set during init"), which by the > shortlog indicates that they were trying to avoid removing an > yet-unduplicated fd too early. > > I don't see why an fd explicitly removed with qmp_remove_fd() should > be under runstate_is_running(). I'm assuming this was a mistake when > adding the parenthesis around the expression. > > Move the runstate_is_running() check to apply only to the > QLIST_EMPTY(dup_fds) side of the expression and ignore it when > mon_fdset_fd->removed has been explicitly set. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> Eric, Kevin, your fingerprints are on commit ebe52b592d. Could you have a look at this fix? > --- > monitor/fds.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/monitor/fds.c b/monitor/fds.c > index d86c2c674c..4ec3b7eea9 100644 > --- a/monitor/fds.c > +++ b/monitor/fds.c > @@ -173,9 +173,9 @@ static void monitor_fdset_cleanup(MonFdset *mon_fdset) > MonFdsetFd *mon_fdset_fd_next; > > QLIST_FOREACH_SAFE(mon_fdset_fd, &mon_fdset->fds, next, mon_fdset_fd_next) { > - if ((mon_fdset_fd->removed || > - (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) && > - runstate_is_running()) { > + if (mon_fdset_fd->removed || > + (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0 && > + runstate_is_running())) { > close(mon_fdset_fd->fd); > g_free(mon_fdset_fd->opaque); > QLIST_REMOVE(mon_fdset_fd, next);
diff --git a/monitor/fds.c b/monitor/fds.c index d86c2c674c..4ec3b7eea9 100644 --- a/monitor/fds.c +++ b/monitor/fds.c @@ -173,9 +173,9 @@ static void monitor_fdset_cleanup(MonFdset *mon_fdset) MonFdsetFd *mon_fdset_fd_next; QLIST_FOREACH_SAFE(mon_fdset_fd, &mon_fdset->fds, next, mon_fdset_fd_next) { - if ((mon_fdset_fd->removed || - (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) && - runstate_is_running()) { + if (mon_fdset_fd->removed || + (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0 && + runstate_is_running())) { close(mon_fdset_fd->fd); g_free(mon_fdset_fd->opaque); QLIST_REMOVE(mon_fdset_fd, next);
We're currently only removing an fd from the fdset if the VM is running. This causes a QMP call to "remove-fd" to not actually remove the fd if the VM happens to be stopped. While the fd would eventually be removed when monitor_fdset_cleanup() is called again, the user request should be honored and the fd actually removed. Calling remove-fd + query-fdset shows a recently removed fd still present. The runstate_is_running() check was introduced by commit ebe52b592d ("monitor: Prevent removing fd from set during init"), which by the shortlog indicates that they were trying to avoid removing an yet-unduplicated fd too early. I don't see why an fd explicitly removed with qmp_remove_fd() should be under runstate_is_running(). I'm assuming this was a mistake when adding the parenthesis around the expression. Move the runstate_is_running() check to apply only to the QLIST_EMPTY(dup_fds) side of the expression and ignore it when mon_fdset_fd->removed has been explicitly set. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- monitor/fds.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)