Re: [Qemu-devel] [PATCH 08/12] migration: xxx_close will only be called once

[Orit Wasserman <owasserm@redhat.com>]

On 10/18/2012 12:22 PM, Paolo Bonzini wrote:
> No need to test s->fd again, it is tested in the caller.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration-exec.c | 14 ++++++--------
>  migration-fd.c   | 33 +++++++++++++++------------------
>  migration-tcp.c  |  7 ++-----
>  migration-unix.c |  7 ++-----
>  4 file modificati, 25 inserzioni(+), 36 rimozioni(-)
> 
> diff --git a/migration-exec.c b/migration-exec.c
> index 0964dbb..1c562ab 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -48,14 +48,12 @@ static int exec_close(MigrationState *s)
>  {
>      int ret = 0;
>      DPRINTF("exec_close\n");
> -    if (s->opaque) {
> -        ret = qemu_fclose(s->opaque);
> -        s->opaque = NULL;
> -        s->fd = -1;
> -        if (ret >= 0 && !(WIFEXITED(ret) && WEXITSTATUS(ret) == 0)) {
> -            /* close succeeded, but non-zero exit code: */
> -            ret = -EIO; /* fake errno value */
> -        }
> +    ret = qemu_fclose(s->opaque);
> +    s->opaque = NULL;
> +    s->fd = -1;
> +    if (ret >= 0 && !(WIFEXITED(ret) && WEXITSTATUS(ret) == 0)) {
> +        /* close succeeded, but non-zero exit code: */
> +        ret = -EIO; /* fake errno value */
>      }
>      return ret;
>  }
> diff --git a/migration-fd.c b/migration-fd.c
> index bb91e0a..67ee3d1 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -48,29 +48,26 @@ static int fd_close(MigrationState *s)
>      int ret;
>  
>      DPRINTF("fd_close\n");
> -    if (s->fd != -1) {
> -        ret = fstat(s->fd, &st);
> -        if (ret == 0 && S_ISREG(st.st_mode)) {
> -            /*
> -             * If the file handle is a regular file make sure the
> -             * data is flushed to disk before signaling success.
> -             */
> -            ret = fsync(s->fd);
> -            if (ret != 0) {
> -                ret = -errno;
> -                perror("migration-fd: fsync");
> -                return ret;
> -            }
> -        }
> -        ret = close(s->fd);
> -        s->fd = -1;
> +    ret = fstat(s->fd, &st);
> +    if (ret == 0 && S_ISREG(st.st_mode)) {
> +        /*
> +         * If the file handle is a regular file make sure the
> +         * data is flushed to disk before signaling success.
> +         */
> +        ret = fsync(s->fd);
>          if (ret != 0) {
>              ret = -errno;
> -            perror("migration-fd: close");
> +            perror("migration-fd: fsync");
>              return ret;
>          }
>      }
> -    return 0;
> +    ret = close(s->fd);
> +    s->fd = -1;
> +    if (ret != 0) {
> +        ret = -errno;
> +        perror("migration-fd: close");
> +    }
> +    return ret;
>  }
>  
>  int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
> diff --git a/migration-tcp.c b/migration-tcp.c
> index e797d23..07715de 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -44,11 +44,8 @@ static int tcp_close(MigrationState *s)
>  {
>      int r = 0;
>      DPRINTF("tcp_close\n");
> -    if (s->fd != -1) {
> -        if (closesocket(s->fd) < 0) {
> -            r = -errno;
> -        }
> -        s->fd = -1;
> +    if (closesocket(s->fd) < 0) {
> +        r = -socket_error();
>      }
>      return r;
>  }
> diff --git a/migration-unix.c b/migration-unix.c
> index a407af2..def1969 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -44,11 +44,8 @@ static int unix_close(MigrationState *s)
>  {
>      int r = 0;
>      DPRINTF("unix_close\n");
> -    if (s->fd != -1) {
> -        if (close(s->fd) < 0) {
> -            r = -errno;
> -        }
> -        s->fd = -1;
> +    if (close(s->fd) < 0) {
> +        r = -errno;
>      }
>      return r;
>  }
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com>