[v3,3/4] monitor: Prevent removing fd from set during init

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 <coreyb@linux.vnet.ibm.com>
---
v3:
  - This patch was split into it's own patch in v3
    (eblake@redhat.com, kwolf@redhat.com)

 monitor.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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 <coreyb@linux.vnet.ibm.com>
> ---
> v3:
>   - This patch was split into it's own patch in v3
>     (eblake@redhat.com, kwolf@redhat.com)
> 
>  monitor.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> 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.

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

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 <coreyb@linux.vnet.ibm.com>
>> ---
>> v3:
>>    - This patch was split into it's own patch in v3
>>      (eblake@redhat.com, kwolf@redhat.com)
>>
>>   monitor.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

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);
>>
>