On Thu, May 16, 2024 at 07:00:11PM -0300, Fabiano Rosas wrote:
> 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().
I know that, but I'm saying QEMU should not be doing *any* generic cleanup
of passed in FDs at any point.
A passed in FD should be taken by whatever part of the QEMU configuration
is told to use it when needed, and this takes responsibility for closing
it. If nothing is told to use the fdset /yet/, then it should stay in the
fdset untouched for later use.
If an application accidentally passes in a FD that it doesn't reference
in any configuration, that's simply a application bug to fix. QEMU does
not need to secondguess the app's intent and decide to arbitrarily close
it.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
