On Fri, 10 Aug 2012 11:13:57 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@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 <lcapitul...@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__,
>
