Re: [Qemu-devel] [PATCH v3 3/4] monitor: Prevent removing fd from set during init

[Corey Bryant]

On 10/17/2012 12:02 AM, Eric Blake wrote:
> On 10/16/2012 12:08 PM, Corey Bryant wrote:
> > 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.
> >
> > Signed-off-by: Corey Bryant <address@hidden>
> > ---
> > v3:
> >    - This patch was split into it's own patch in v3
> >      (address@hidden, address@hidden)
> >
> >   monitor.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!

> > diff --git a/monitor.c b/monitor.c
> > index 5d5de41..0dae7ac 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2105,8 +2105,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)) {
> > +        if ((mon_fdset_fd->removed ||
> > +                (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) &&
> > +                runstate_is_running()) {
>
> This condition looks sufficient for command line parsing; however, I
> have to wonder if there are any long-term ill effects, such as if I
> pause a guest, then close an fdset, where qemu keeps the fd open until I
> continue the guest.  But thinking a bit more, we never promised that
> qemu would close fds right away, but only that it wouldn't leak fds by
> closing them whenever qemu thinks it is convenient.

I can't think of any ill effects.  I think you may be able to remove an 
fd or fd set followed by adding a drive using that fd set, but that is 
just a difference in behavior compared to when a guest is running.

> >               close(mon_fdset_fd->fd);
> >               g_free(mon_fdset_fd->opaque);
> >               QLIST_REMOVE(mon_fdset_fd, next);

--
Regards,
Corey Bryant