Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno

[Luiz Capitulino]

On Thu, 02 Aug 2012 17:50:30 +0200
Markus Armbruster <address@hidden> wrote:

> [cc: Juan & Amos]
> 
> Luiz Capitulino <address@hidden> writes:
> 
> > On Wed,  1 Aug 2012 22:02:35 -0300
> > Luiz Capitulino <address@hidden> wrote:
> >
> >> Next commit wants to use this.
> >> 
> >> Signed-off-by: Luiz Capitulino <address@hidden>
> >> ---
> >> 
> >> 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.