Re: [Qemu-devel] [PATCH v2 1/1] Coverity: Fix failure path for qemu_accept in migration

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <address@hidden> writes:

> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Coverity defects 1005733 & 1005734 complain about passing a negative
> value to closesocket in the error paths on incoming migration.
>
> Stash the error value and print it in the message (previously we gave
> no indication of the reason for the failure)
>
> Use error_report
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  migration-tcp.c  | 17 +++++++++++------
>  migration-unix.c | 17 +++++++++++------
>  2 files changed, 22 insertions(+), 12 deletions(-)
>
> V2
>   Use c < 0 everywhere
>   Use error_report instead of fprintf(stdrr,
>   Use strerror
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 782572d..57ea9d2 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -13,7 +13,10 @@
>   * GNU GPL, version 2 or (at your option) any later version.
>   */
>  
> +#include <string.h>
> +
>  #include "qemu-common.h"
> +#include "qemu/error-report.h"
>  #include "qemu/sockets.h"
>  #include "migration/migration.h"
>  #include "migration/qemu-file.h"
> @@ -56,24 +59,26 @@ static void tcp_accept_incoming_migration(void *opaque)
>      socklen_t addrlen = sizeof(addr);
>      int s = (intptr_t)opaque;
>      QEMUFile *f;
> -    int c;
> +    int c, err;
>  
>      do {
>          c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
> -    } while (c == -1 && socket_error() == EINTR);
> +        err = socket_error();
> +    } while (c < 0 && err == EINTR);
>      qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
>      closesocket(s);
>  
>      DPRINTF("accepted migration\n");
>  
> -    if (c == -1) {
> -        fprintf(stderr, "could not accept migration connection\n");
> -        goto out;
> +    if (c < 0) {
> +        error_report("could not accept migration connection (%s)",
> +                strerror(err));

If you need to respin for some other reason (which I doubt), fix up
indentation here.

> +        return;
>      }
>  
>      f = qemu_fopen_socket(c, "rb");
>      if (f == NULL) {
> -        fprintf(stderr, "could not qemu_fopen socket\n");
> +        error_report("could not qemu_fopen socket");
>          goto out;
>      }
>  
> diff --git a/migration-unix.c b/migration-unix.c
> index 651fc5b..05e2acd 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -13,7 +13,10 @@
>   * GNU GPL, version 2 or (at your option) any later version.
>   */
>  
> +#include <string.h>
> +
>  #include "qemu-common.h"
> +#include "qemu/error-report.h"
>  #include "qemu/sockets.h"
>  #include "qemu/main-loop.h"
>  #include "migration/migration.h"
> @@ -56,24 +59,26 @@ static void unix_accept_incoming_migration(void *opaque)
>      socklen_t addrlen = sizeof(addr);
>      int s = (intptr_t)opaque;
>      QEMUFile *f;
> -    int c;
> +    int c, err;
>  
>      do {
>          c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
> -    } while (c == -1 && errno == EINTR);
> +        err = errno;
> +    } while (c < 0 && err == EINTR);
>      qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
>      close(s);
>  
>      DPRINTF("accepted migration\n");
>  
> -    if (c == -1) {
> -        fprintf(stderr, "could not accept migration connection\n");
> -        goto out;
> +    if (c < 0) {
> +        error_report("could not accept migration connection (%s)",
> +                strerror(err));

Likewise.

> +        return;
>      }
>  
>      f = qemu_fopen_socket(c, "rb");
>      if (f == NULL) {
> -        fprintf(stderr, "could not qemu_fopen socket\n");
> +        error_report("could not qemu_fopen socket");
>          goto out;
>      }

Reviewed-by: Markus Armbruster <address@hidden>