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
