Re: [Qemu-devel] [PATCH 15/35] migration: don't rely on any QERR_SOCKET_*

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> Use the in_progress argument for QERR_SOCKET_CONNECT_IN_PROGRESS. The
> other errors are handled the same by checking if the error is set and
> then calling migrate_fd_error() if it's.
>
> It's also necessary to change inet_connect_opts() not to set
> QERR_SOCKET_CONNECT_IN_PROGRESS. This error is only used by
> tcp_start_outgoing_migration() and not changing it along with the
> usage of in_progress would break migration.
>
> Furthermore this commit fixes a bug. Today, there's a spurious error
> report when migration succeeds:
>
> (qemu) migrate tcp:0:4444
> migrate: Connection can not be completed immediately
> (qemu)
>
> After this commit no spurious error is reported anymore.
>
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
>  migration-tcp.c | 22 ++++++++--------------
>  qemu-sockets.c  |  2 --
>  2 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 18944a4..40c277f 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -82,27 +82,21 @@ static void tcp_wait_for_connect(void *opaque)
>  int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>                                   Error **errp)
>  {
> +    bool in_progress;
> +
>      s->get_error = socket_errno;
>      s->write = socket_write;
>      s->close = tcp_close;
>  
> -    s->fd = inet_connect(host_port, false, NULL, errp);
> -
> -    if (!error_is_set(errp)) {
> -        migrate_fd_connect(s);
> -    } else if (error_is_type(*errp, QERR_SOCKET_CONNECT_IN_PROGRESS)) {
> -        DPRINTF("connect in progress\n");
> -        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
> -    } else if (error_is_type(*errp, QERR_SOCKET_CREATE_FAILED)) {
> -        DPRINTF("connect failed\n");
> -        return -1;
> -    } else if (error_is_type(*errp, QERR_SOCKET_CONNECT_FAILED)) {
> -        DPRINTF("connect failed\n");
> +    s->fd = inet_connect(host_port, false, &in_progress, errp);
> +    if (error_is_set(errp)) {
>          migrate_fd_error(s);
>          return -1;
> +    } else if (in_progress) {
> +        DPRINTF("connect in progress\n");
> +        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>      } else {
> -        DPRINTF("unknown error\n");
> -        return -1;
> +        migrate_fd_connect(s);
>      }
>  
>      return 0;

I'd prefer

       s->fd = inet_connect(host_port, false, &in_progress, errp);
       if (error_is_set(errp)) {
           migrate_fd_error(s);
           return -1;
       }
       if (in_progress) {
           DPRINTF("connect in progress\n");
           qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
       } else {
           migrate_fd_connect(s);
       }
       return 0;

because it separates abnormal and normal code paths more clearly.

Matter of taste.

> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 9cb47d4..361d890 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -284,8 +284,6 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, 
> Error **errp)
>              if (in_progress) {
>                  *in_progress = true;
>              }
> -
> -            error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
>          } else if (rc < 0) {
>              if (NULL == e->ai_next)
>                  fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", 
> __FUNCTION__,