[v4,4/4] Clear handler only for valid fd

Submitted by Orit Wasserman on Sept. 23, 2012, 2:49 p.m.

Details

Message ID 1348411747-29804-5-git-send-email-owasserm@redhat.com
State New
Headers show

Commit Message

Orit Wasserman Sept. 23, 2012, 2:49 p.m.
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 migration.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

Comments

Markus Armbruster Sept. 24, 2012, 9:45 a.m.
Orit Wasserman <owasserm@redhat.com> writes:

> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  migration.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 1edeec5..c20a2fe 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -240,8 +240,6 @@ static int migrate_fd_cleanup(MigrationState *s)
>  {
>      int ret = 0;
>  
> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> -
>      if (s->file) {
>          DPRINTF("closing file\n");
>          ret = qemu_fclose(s->file);
> @@ -249,6 +247,7 @@ static int migrate_fd_cleanup(MigrationState *s)
>      }
>  
>      if (s->fd != -1) {
> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>          close(s->fd);
>          s->fd = -1;
>      }

As far as I can see, qemu_set_fd_handler2() treats invalid file
descriptor -1 just like any other.  If it's in io_handlers, it gets
deleted, else it's a nop.  Thus, the old code works.  But I agree
passing invalid file descriptors is abusing the interface.

I'm not sufficiently familiar with the migration code to judge whether
moving the handler reset down is safe.
Orit Wasserman Sept. 24, 2012, 9:58 a.m.
On 09/24/2012 11:45 AM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  migration.c |    3 +--
>>  1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index 1edeec5..c20a2fe 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -240,8 +240,6 @@ static int migrate_fd_cleanup(MigrationState *s)
>>  {
>>      int ret = 0;
>>  
>> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> -
>>      if (s->file) {
>>          DPRINTF("closing file\n");
>>          ret = qemu_fclose(s->file);
>> @@ -249,6 +247,7 @@ static int migrate_fd_cleanup(MigrationState *s)
>>      }
>>  
>>      if (s->fd != -1) {
>> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>          close(s->fd);
>>          s->fd = -1;
>>      }
> 
> As far as I can see, qemu_set_fd_handler2() treats invalid file
> descriptor -1 just like any other.  If it's in io_handlers, it gets
> deleted, else it's a nop.  Thus, the old code works.
Not any more, there was an assert(fd >=0) added in commit bbdd2ad0814ea0911076419ea21b7957505cf1cc
recently.

  But I agree
> passing invalid file descriptors is abusing the interface.
> 
> I'm not sufficiently familiar with the migration code to judge whether
> moving the handler reset down is safe.
> 
I can keep the call in the same location if you think it is safer.

Orit
Paolo Bonzini Sept. 24, 2012, 10:06 a.m.
Il 24/09/2012 11:58, Orit Wasserman ha scritto:
>>> >>      if (s->fd != -1) {
>>> >> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>> >>          close(s->fd);
>>> >>          s->fd = -1;
>>> >>      }
>> > 
>> > As far as I can see, qemu_set_fd_handler2() treats invalid file
>> > descriptor -1 just like any other.  If it's in io_handlers, it gets
>> > deleted, else it's a nop.  Thus, the old code works.
> Not any more, there was an assert(fd >=0) added in commit bbdd2ad0814ea0911076419ea21b7957505cf1cc
> recently.
> 
>> > I'm not sufficiently familiar with the migration code to judge whether
>> > moving the handler reset down is safe.
>> > 
> I can keep the call in the same location if you think it is safer.

I think it's okay as you did, given how the code looks like now.

I plan to make qemu_fclose take care of closing the file descriptor, but
then I'll adjust the code myself.  I just want this series to go in. :)))

Paolo
Markus Armbruster Sept. 24, 2012, 11:34 a.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 24/09/2012 11:58, Orit Wasserman ha scritto:
>>>> >>      if (s->fd != -1) {
>>>> >> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>> >>          close(s->fd);
>>>> >>          s->fd = -1;
>>>> >>      }
>>> > 
>>> > As far as I can see, qemu_set_fd_handler2() treats invalid file
>>> > descriptor -1 just like any other.  If it's in io_handlers, it gets
>>> > deleted, else it's a nop.  Thus, the old code works.
>> Not any more, there was an assert(fd >=0) added in commit
>> bbdd2ad0814ea0911076419ea21b7957505cf1cc
>> recently.
>> 
>>> > I'm not sufficiently familiar with the migration code to judge whether
>>> > moving the handler reset down is safe.
>>> > 
>> I can keep the call in the same location if you think it is safer.

I have no idea, and...

> I think it's okay as you did, given how the code looks like now.

... I'm prepared to take Paolo's word for it.

[...]

Patch hide | download patch | download mbox

diff --git a/migration.c b/migration.c
index 1edeec5..c20a2fe 100644
--- a/migration.c
+++ b/migration.c
@@ -240,8 +240,6 @@  static int migrate_fd_cleanup(MigrationState *s)
 {
     int ret = 0;
 
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-
     if (s->file) {
         DPRINTF("closing file\n");
         ret = qemu_fclose(s->file);
@@ -249,6 +247,7 @@  static int migrate_fd_cleanup(MigrationState *s)
     }
 
     if (s->fd != -1) {
+        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
         close(s->fd);
         s->fd = -1;
     }