Patchwork [06/12] migration: use migrate_fd_close in migrate_fd_cleanup

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 18, 2012, 10:22 a.m.
Message ID <1350555758-29988-7-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/192276/
State New
Headers show

Comments

Paolo Bonzini - Oct. 18, 2012, 10:22 a.m.
migrate_fd_cleanup will usually close the file descriptor via
buffered_file_close's call to migrate_fd_close.  However, in the case
of s->file == NULL it is "inlining" migrate_fd_close (almost: there is a
direct close() instead of using s->close(s)).  To fix the inconsistency
and clean up the code, allow multiple calls to migrate_fd_close and use
the function in migrate_fd_cleanup.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration.c | 20 ++++++++------------
 1 file modificato, 8 inserzioni(+), 12 rimozioni(-)
Orit Wasserman - Oct. 28, 2012, 9:47 a.m.
On 10/18/2012 12:22 PM, Paolo Bonzini wrote:
> migrate_fd_cleanup will usually close the file descriptor via
> buffered_file_close's call to migrate_fd_close.  However, in the case
> of s->file == NULL it is "inlining" migrate_fd_close (almost: there is a
> direct close() instead of using s->close(s)).  To fix the inconsistency
> and clean up the code, allow multiple calls to migrate_fd_close and use
> the function in migrate_fd_cleanup.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration.c | 20 ++++++++------------
>  1 file modificato, 8 inserzioni(+), 12 rimozioni(-)
> 
> diff --git a/migration.c b/migration.c
> index 62e0304..c1807e0 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -246,21 +246,12 @@ static int migrate_fd_cleanup(MigrationState *s)
>  {
>      int ret = 0;
>  
> -    if (s->fd != -1) {
> -        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> -    }
> -
>      if (s->file) {
>          DPRINTF("closing file\n");
>          ret = qemu_fclose(s->file);
> -        s->file = NULL;
why remove this line?
> -    }
> -
> -    if (s->fd != -1) {
> -        close(s->fd);
> -        s->fd = -1;
>      }
>  
> +    migrate_fd_close(s);
>      return ret;
>  }
>  
> @@ -396,8 +387,13 @@ int migrate_fd_wait_for_unfreeze(MigrationState *s)
>  
>  int migrate_fd_close(MigrationState *s)
>  {
> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> -    return s->close(s);
> +    int rc = 0;
> +    if (s->fd != -1) {
> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +        rc = s->close(s);
> +        s->fd = -1;
> +    }
> +    return rc;
>  }
>  
>  void add_migration_state_change_notifier(Notifier *notify)
>
Paolo Bonzini - Oct. 28, 2012, 3:05 p.m.
Il 28/10/2012 10:47, Orit Wasserman ha scritto:
>> >      if (s->file) {
>> >          DPRINTF("closing file\n");
>> >          ret = qemu_fclose(s->file);
>> > -        s->file = NULL;
> why remove this line?

Good catch.

Paolo

Patch

diff --git a/migration.c b/migration.c
index 62e0304..c1807e0 100644
--- a/migration.c
+++ b/migration.c
@@ -246,21 +246,12 @@  static int migrate_fd_cleanup(MigrationState *s)
 {
     int ret = 0;
 
-    if (s->fd != -1) {
-        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-    }
-
     if (s->file) {
         DPRINTF("closing file\n");
         ret = qemu_fclose(s->file);
-        s->file = NULL;
-    }
-
-    if (s->fd != -1) {
-        close(s->fd);
-        s->fd = -1;
     }
 
+    migrate_fd_close(s);
     return ret;
 }
 
@@ -396,8 +387,13 @@  int migrate_fd_wait_for_unfreeze(MigrationState *s)
 
 int migrate_fd_close(MigrationState *s)
 {
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-    return s->close(s);
+    int rc = 0;
+    if (s->fd != -1) {
+        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+        rc = s->close(s);
+        s->fd = -1;
+    }
+    return rc;
 }
 
 void add_migration_state_change_notifier(Notifier *notify)