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

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

[Corey Bryant]

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

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()) {
             close(mon_fdset_fd->fd);
             g_free(mon_fdset_fd->opaque);
             QLIST_REMOVE(mon_fdset_fd, next);
-- 
1.7.11.4

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]

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

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

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

-- 
Regards,
Corey Bryant