Patchwork [15/35] migration: don't rely on any QERR_SOCKET_*

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 7, 2012, 3:53 p.m.
Message ID <1344354826-10375-16-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/175719/
State New
Headers show

Comments

Luiz Capitulino - Aug. 7, 2012, 3:53 p.m.
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(-)
Markus Armbruster - Aug. 10, 2012, 9:13 a.m.
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.

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__,
Luiz Capitulino - Aug. 10, 2012, 2:49 p.m.
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__,
>
Juan Quintela - Aug. 14, 2012, 11:19 a.m.
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>
>
> 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.


The 1st migrate_fd_error() is not needed (it was already wrong).

migrate_fd_* functions are supposed to be called only after
migrate_fd_connect() has been called.

Later, Juan.
Markus Armbruster - Aug. 14, 2012, 11:35 a.m.
Juan Quintela <quintela@redhat.com> writes:

> 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>
>>
>> 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.
>
>
> The 1st migrate_fd_error() is not needed (it was already wrong).
>
> migrate_fd_* functions are supposed to be called only after
> migrate_fd_connect() has been called.

It's been committed.  Could you post a patch for it?
Luiz Capitulino - Aug. 14, 2012, 1:06 p.m.
On Tue, 14 Aug 2012 13:35:59 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Juan Quintela <quintela@redhat.com> writes:
> 
> > 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>
> >>
> >> 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.
> >
> >
> > The 1st migrate_fd_error() is not needed (it was already wrong).
> >
> > migrate_fd_* functions are supposed to be called only after
> > migrate_fd_connect() has been called.
> 
> It's been committed.  Could you post a patch for it?

Juan, are you going to do this or do you want me to do it?
Juan Quintela - Aug. 14, 2012, 1:09 p.m.
Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> 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>
>>>
>>> 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.
>>
>>
>> The 1st migrate_fd_error() is not needed (it was already wrong).
>>
>> migrate_fd_* functions are supposed to be called only after
>> migrate_fd_connect() has been called.
>
> It's been committed.  Could you post a patch for it?

I am doing it.  Thanks.

Patch

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