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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 10 Aug 2012 11:13:57 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> > ---
> >  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.

Very minor, but I've changed it.

> 
> 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__,
>