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

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

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.

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

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

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.

[...]