Daniel P. Berrangé <berra...@redhat.com> writes: > On Fri, Apr 26, 2024 at 11:20:34AM -0300, Fabiano Rosas wrote: >> We're enabling using the fdset interface to pass file descriptors for >> use in the migration code. Since migrations can happen more than once >> during the VMs lifetime, we need a way to remove an fd from the fdset >> at the end of migration. >> >> The current code only removes 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. > > IMHO that should be reverted. The justification says > > "If an fd is added to an fd set via the command line, and it is not > referenced by another command line option (ie. -drive), then clean > it up after QEMU initialization is complete" > > which I think is pretty weak. Why should QEMU forceably stop an app > from passing in an FD to be used by a QMP command issued just after > the VM starts running ? While it could just use QMP to pass in the > FD set, the mgmt app might have its own reason for wanting QEMU to > own the passed FD from the very start of the process execve().
I don't think that's what that patch does. That description is misleading. I read it as: "If an fd is added to an fd set via the command line, and it is not referenced by another command line option (ie. -drive), then clean it up ONLY after QEMU initialization is complete" ^ By the subject ("monitor: Prevent removing fd from set during init") and the fact that this function is only called when the monitor connection closes, I believe the idea was to *save* the fds until after the VM starts running, i.e. some fd was being lost because monitor_fdset_cleanup() was being called before the dup(). See my reply to Peter in this same patch (PATCH 1/9). > > Implicitly this cleanup is attempting to "fix" a bug where the mgmt > app passes in an FD that it never needed. If any such bug were ever > found, then the mgmt app should just be fixed to not pass it in. I > don't think QEMU needs to be trying to fix mgmt app bugs. > > IOW, this commit is imposing an arbitrary & unecessary usage policy > on passed in FD sets, and as your commit explains has further > unhelpful (& undocumented) side effects on the 'remove-fd' QMP command. > > Just revert it IMHO. > >> >> 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 <faro...@suse.de> >> --- >> 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); >> -- >> 2.35.3 >> > > With regards, > Daniel