Patchwork [15/34] net: inet_connect(), inet_connect_opts(): return -errno

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 2, 2012, 1:02 a.m.
Message ID <1343869374-23417-16-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/174677/
State New
Headers show

Comments

Luiz Capitulino - Aug. 2, 2012, 1:02 a.m.
Next commit wants to use this.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---

This patch is an interesting case, because one of the goal of the error
format that's being replaced was that callers could use it to know the
error cause (with error_is_type().

However, the new error format doesn't allow this as most errors are
class GenericError. So, we'll have to use errno to know the error cause,
this is the case of inet_connect() when called by
tcp_start_outgoing_migration().

 qemu-sockets.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Luiz Capitulino - Aug. 2, 2012, 1:41 p.m.
On Wed,  1 Aug 2012 22:02:35 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> Next commit wants to use this.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> 
> This patch is an interesting case, because one of the goal of the error
> format that's being replaced was that callers could use it to know the
> error cause (with error_is_type().
> 
> However, the new error format doesn't allow this as most errors are
> class GenericError. So, we'll have to use errno to know the error cause,
> this is the case of inet_connect() when called by
> tcp_start_outgoing_migration().

I'm thinking in doing this differently. Instead of returning errno, we
could have:

 error_sete(Error **err, ErrorClass err_class, int err_no,
            const char *fmt, ...);

Then we store err_no in Error, and also add error_get_errno().

Comments?
Markus Armbruster - Aug. 2, 2012, 3:50 p.m.
[cc: Juan & Amos]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed,  1 Aug 2012 22:02:35 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>
>> Next commit wants to use this.
>> 
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>> 
>> This patch is an interesting case, because one of the goal of the error
>> format that's being replaced was that callers could use it to know the
>> error cause (with error_is_type().
>> 
>> However, the new error format doesn't allow this as most errors are
>> class GenericError. So, we'll have to use errno to know the error cause,
>> this is the case of inet_connect() when called by
>> tcp_start_outgoing_migration().
>
> I'm thinking in doing this differently. Instead of returning errno, we
> could have:
>
>  error_sete(Error **err, ErrorClass err_class, int err_no,
>             const char *fmt, ...);
>
> Then we store err_no in Error, and also add error_get_errno().
>
> Comments?

Maybe that'll turn out to be useful elsewhere, but not here.

The purpose of PATCH 14+15 is to permit purging error_is_type() from
tcp_start_outgoing_migration() in PATCH 16.  Let's have a look at all
three patches together.

This is tcp_start_outgoing_migration() now:

    int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
                                     Error **errp)
    {
        s->get_error = socket_errno;
        s->write = socket_write;
        s->close = tcp_close;

        s->fd = inet_connect(host_port, false, 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");
            migrate_fd_error(s);
            return -1;
        } else {
            DPRINTF("unknown error\n");
            return -1;
        }

        return 0;
    }

Cases:

1. Success

   Proceeed to migrate_fd_connect().

2. QERR_SOCKET_CONNECT_IN_PROGRESS

   connect() failed with -EINPROGRESS.  Not actually an error.  Set up
   appropriate callback.

3. QERR_SOCKET_CONNECT_FAILED

   getaddrinfo() succeeded, but could not connect() to any of the
   addresses.  Fail migration with migrate_fd_error().

4. QERR_SOCKET_CREATE_FAILED

   The error grabbag:

   * inet_parse() failed, or

   * inet_connect_opts() misses host and/or port (should never happen)

   * getaddrinfo() failed

   Bug: neglects to migrate_fd_error()!  Broken in commit d5c5dacc.

5. Anything else

   Should never happen.  Handled exactly like 4.

If I undo d5c5dacc's damage (untested), it looks like this:

    int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
                                     Error **errp)
    {
        s->get_error = socket_errno;
        s->write = socket_write;
        s->close = tcp_close;

        s->fd = inet_connect(host_port, false, 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 {
            DPRINTF("connect failed\n");
            migrate_fd_error(s);
            return -1;
        }

        return 0;
    }

Et voilà, the only error_is_type() is the non-error "in progress", and
your new in_progress covers that already, no need for an errno.
Luiz Capitulino - Aug. 2, 2012, 4:49 p.m.
On Thu, 02 Aug 2012 17:50:30 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> [cc: Juan & Amos]
> 
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed,  1 Aug 2012 22:02:35 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >
> >> Next commit wants to use this.
> >> 
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> ---
> >> 
> >> This patch is an interesting case, because one of the goal of the error
> >> format that's being replaced was that callers could use it to know the
> >> error cause (with error_is_type().
> >> 
> >> However, the new error format doesn't allow this as most errors are
> >> class GenericError. So, we'll have to use errno to know the error cause,
> >> this is the case of inet_connect() when called by
> >> tcp_start_outgoing_migration().
> >
> > I'm thinking in doing this differently. Instead of returning errno, we
> > could have:
> >
> >  error_sete(Error **err, ErrorClass err_class, int err_no,
> >             const char *fmt, ...);
> >
> > Then we store err_no in Error, and also add error_get_errno().
> >
> > Comments?
> 
> Maybe that'll turn out to be useful elsewhere, but not here.
> 
> The purpose of PATCH 14+15 is to permit purging error_is_type() from
> tcp_start_outgoing_migration() in PATCH 16.  Let's have a look at all
> three patches together.
> 
> This is tcp_start_outgoing_migration() now:
> 
>     int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>                                      Error **errp)
>     {
>         s->get_error = socket_errno;
>         s->write = socket_write;
>         s->close = tcp_close;
> 
>         s->fd = inet_connect(host_port, false, 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");
>             migrate_fd_error(s);
>             return -1;
>         } else {
>             DPRINTF("unknown error\n");
>             return -1;
>         }
> 
>         return 0;
>     }
> 
> Cases:
> 
> 1. Success
> 
>    Proceeed to migrate_fd_connect().
> 
> 2. QERR_SOCKET_CONNECT_IN_PROGRESS
> 
>    connect() failed with -EINPROGRESS.  Not actually an error.  Set up
>    appropriate callback.
> 
> 3. QERR_SOCKET_CONNECT_FAILED
> 
>    getaddrinfo() succeeded, but could not connect() to any of the
>    addresses.  Fail migration with migrate_fd_error().
> 
> 4. QERR_SOCKET_CREATE_FAILED
> 
>    The error grabbag:
> 
>    * inet_parse() failed, or
> 
>    * inet_connect_opts() misses host and/or port (should never happen)
> 
>    * getaddrinfo() failed
> 
>    Bug: neglects to migrate_fd_error()!  Broken in commit d5c5dacc.
> 
> 5. Anything else
> 
>    Should never happen.  Handled exactly like 4.
> 
> If I undo d5c5dacc's damage (untested), it looks like this:
> 
>     int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>                                      Error **errp)
>     {
>         s->get_error = socket_errno;
>         s->write = socket_write;
>         s->close = tcp_close;
> 
>         s->fd = inet_connect(host_port, false, 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 {
>             DPRINTF("connect failed\n");
>             migrate_fd_error(s);
>             return -1;
>         }
> 
>         return 0;
>     }
> 
> Et voilà, the only error_is_type() is the non-error "in progress", and
> your new in_progress covers that already, no need for an errno.

You seem to be right. When I cooked these patches I thought that the
missing call to migrate_fd_error() was intentional, but now I see it's a bug.

I'll do what you suggest.
Amos Kong - Aug. 6, 2012, 6:52 a.m.
----- Original Message -----
> [cc: Juan & Amos]
> 
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed,  1 Aug 2012 22:02:35 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >
> >> Next commit wants to use this.
> >> 
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> ---
> >> 
> >> This patch is an interesting case, because one of the goal of the
> >> error
> >> format that's being replaced was that callers could use it to know
> >> the
> >> error cause (with error_is_type().
> >> 
> >> However, the new error format doesn't allow this as most errors
> >> are
> >> class GenericError. So, we'll have to use errno to know the error
> >> cause,
> >> this is the case of inet_connect() when called by
> >> tcp_start_outgoing_migration().
> >
> > I'm thinking in doing this differently. Instead of returning errno,
> > we
> > could have:
> >
> >  error_sete(Error **err, ErrorClass err_class, int err_no,
> >             const char *fmt, ...);
> >
> > Then we store err_no in Error, and also add error_get_errno().
> >
> > Comments?
> 
> Maybe that'll turn out to be useful elsewhere, but not here.
> 
> The purpose of PATCH 14+15 is to permit purging error_is_type() from
> tcp_start_outgoing_migration() in PATCH 16.  Let's have a look at all
> three patches together.
> 
> This is tcp_start_outgoing_migration() now:
> 
>     int tcp_start_outgoing_migration(MigrationState *s, const char
>     *host_port,
>                                      Error **errp)
>     {
>         s->get_error = socket_errno;
>         s->write = socket_write;
>         s->close = tcp_close;
> 
>         s->fd = inet_connect(host_port, false, 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");
>             migrate_fd_error(s);
>             return -1;
>         } else {
>             DPRINTF("unknown error\n");
>             return -1;
>         }
> 
>         return 0;
>     }
> 
> Cases:
> 
> 1. Success
> 
>    Proceeed to migrate_fd_connect().
> 
> 2. QERR_SOCKET_CONNECT_IN_PROGRESS
> 
>    connect() failed with -EINPROGRESS.  Not actually an error.  Set
>    up
>    appropriate callback.
> 
> 3. QERR_SOCKET_CONNECT_FAILED
> 
>    getaddrinfo() succeeded, but could not connect() to any of the
>    addresses.  Fail migration with migrate_fd_error().
> 
> 4. QERR_SOCKET_CREATE_FAILED
> 
>    The error grabbag:
> 
>    * inet_parse() failed, or
> 
>    * inet_connect_opts() misses host and/or port (should never
>    happen)
> 
>    * getaddrinfo() failed
> 
>    Bug: neglects to migrate_fd_error()!  Broken in commit d5c5dacc.


Before commit d5c5dacc,  migrate_fd_error() would not be called
when fail to create socket.

-    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (s->fd == -1) {
-        DPRINTF("Unable to open socket");
-        return -socket_error();
-    }


If you call migrate_fd_error() in this situation, qemu would hang.


> 5. Anything else
> 
>    Should never happen.  Handled exactly like 4.
> 
> If I undo d5c5dacc's damage (untested), it looks like this:
> 
>     int tcp_start_outgoing_migration(MigrationState *s, const char
>     *host_port,
>                                      Error **errp)
>     {
>         s->get_error = socket_errno;
>         s->write = socket_write;
>         s->close = tcp_close;
> 
>         s->fd = inet_connect(host_port, false, 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 {
>             DPRINTF("connect failed\n");
>             migrate_fd_error(s);

qemu would hang if socket isn't created successfully.

>             return -1;
>         }
> 
>         return 0;
>     }
> 
> Et voilà, the only error_is_type() is the non-error "in progress",
> and
> your new in_progress covers that already, no need for an errno.
>
Luiz Capitulino - Aug. 6, 2012, 7:59 p.m.
On Mon, 6 Aug 2012 02:52:24 -0400 (EDT)
Amos Kong <akong@redhat.com> wrote:

> 
> 
> ----- Original Message -----
> > [cc: Juan & Amos]
> > 
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> > 
> > > On Wed,  1 Aug 2012 22:02:35 -0300
> > > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > >
> > >> Next commit wants to use this.
> > >> 
> > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > >> ---
> > >> 
> > >> This patch is an interesting case, because one of the goal of the
> > >> error
> > >> format that's being replaced was that callers could use it to know
> > >> the
> > >> error cause (with error_is_type().
> > >> 
> > >> However, the new error format doesn't allow this as most errors
> > >> are
> > >> class GenericError. So, we'll have to use errno to know the error
> > >> cause,
> > >> this is the case of inet_connect() when called by
> > >> tcp_start_outgoing_migration().
> > >
> > > I'm thinking in doing this differently. Instead of returning errno,
> > > we
> > > could have:
> > >
> > >  error_sete(Error **err, ErrorClass err_class, int err_no,
> > >             const char *fmt, ...);
> > >
> > > Then we store err_no in Error, and also add error_get_errno().
> > >
> > > Comments?
> > 
> > Maybe that'll turn out to be useful elsewhere, but not here.
> > 
> > The purpose of PATCH 14+15 is to permit purging error_is_type() from
> > tcp_start_outgoing_migration() in PATCH 16.  Let's have a look at all
> > three patches together.
> > 
> > This is tcp_start_outgoing_migration() now:
> > 
> >     int tcp_start_outgoing_migration(MigrationState *s, const char
> >     *host_port,
> >                                      Error **errp)
> >     {
> >         s->get_error = socket_errno;
> >         s->write = socket_write;
> >         s->close = tcp_close;
> > 
> >         s->fd = inet_connect(host_port, false, 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");
> >             migrate_fd_error(s);
> >             return -1;
> >         } else {
> >             DPRINTF("unknown error\n");
> >             return -1;
> >         }
> > 
> >         return 0;
> >     }
> > 
> > Cases:
> > 
> > 1. Success
> > 
> >    Proceeed to migrate_fd_connect().
> > 
> > 2. QERR_SOCKET_CONNECT_IN_PROGRESS
> > 
> >    connect() failed with -EINPROGRESS.  Not actually an error.  Set
> >    up
> >    appropriate callback.
> > 
> > 3. QERR_SOCKET_CONNECT_FAILED
> > 
> >    getaddrinfo() succeeded, but could not connect() to any of the
> >    addresses.  Fail migration with migrate_fd_error().
> > 
> > 4. QERR_SOCKET_CREATE_FAILED
> > 
> >    The error grabbag:
> > 
> >    * inet_parse() failed, or
> > 
> >    * inet_connect_opts() misses host and/or port (should never
> >    happen)
> > 
> >    * getaddrinfo() failed
> > 
> >    Bug: neglects to migrate_fd_error()!  Broken in commit d5c5dacc.
> 
> 
> Before commit d5c5dacc,  migrate_fd_error() would not be called
> when fail to create socket.
> 
> -    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> -    if (s->fd == -1) {
> -        DPRINTF("Unable to open socket");
> -        return -socket_error();
> -    }
> 
> 
> If you call migrate_fd_error() in this situation, qemu would hang.

How did you test this?

I've added the call to migrate_fd_error() as Markus suggests above and
tested it by changing inet_connect_opts() to trigger all possible errors
and everything seems to work fine.

> 
> 
> > 5. Anything else
> > 
> >    Should never happen.  Handled exactly like 4.
> > 
> > If I undo d5c5dacc's damage (untested), it looks like this:
> > 
> >     int tcp_start_outgoing_migration(MigrationState *s, const char
> >     *host_port,
> >                                      Error **errp)
> >     {
> >         s->get_error = socket_errno;
> >         s->write = socket_write;
> >         s->close = tcp_close;
> > 
> >         s->fd = inet_connect(host_port, false, 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 {
> >             DPRINTF("connect failed\n");
> >             migrate_fd_error(s);
> 
> qemu would hang if socket isn't created successfully.
> 
> >             return -1;
> >         }
> > 
> >         return 0;
> >     }
> > 
> > Et voilà, the only error_is_type() is the non-error "in progress",
> > and
> > your new in_progress covers that already, no need for an errno.
> >
>

Patch

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 33b65cf..82f4736 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -234,7 +234,7 @@  int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
     if (addr == NULL || port == NULL) {
         fprintf(stderr, "inet_connect: host and/or port not specified\n");
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
-        return -1;
+        return -EINVAL;
     }
 
     if (qemu_opt_get_bool(opts, "ipv4", 0))
@@ -247,7 +247,7 @@  int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
         fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
                 gai_strerror(rc));
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
-	return -1;
+        return -EINVAL;
     }
 
     for (e = res; e != NULL; e = e->ai_next) {
@@ -300,7 +300,7 @@  int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
     }
     error_set(errp, QERR_SOCKET_CONNECT_FAILED);
     freeaddrinfo(res);
-    return -1;
+    return -ENOTCONN;
 }
 
 int inet_dgram_opts(QemuOpts *opts)
@@ -508,6 +508,7 @@  int inet_connect(const char *str, bool block, bool *in_progress, Error **errp)
         }
         sock = inet_connect_opts(opts, in_progress, errp);
     } else {
+        sock = -EINVAL;
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
     }
     qemu_opts_del(opts);