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

[Luiz Capitulino]

On Mon, 6 Aug 2012 02:52:24 -0400 (EDT)
Amos Kong <address@hidden> wrote:

> 
> 
> ----- Original Message -----
> > [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.
> 
> 
> 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.
> >
>