[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts():
From: |
Amos Kong |
Subject: |
Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno |
Date: |
Mon, 6 Aug 2012 02:52:24 -0400 (EDT) |
----- 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.
> 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.
>
[Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno, Luiz Capitulino, 2012/08/01
- Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno, Luiz Capitulino, 2012/08/02
- Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno, Markus Armbruster, 2012/08/02
- Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno, Luiz Capitulino, 2012/08/02
- Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno,
Amos Kong <=
- Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno, Luiz Capitulino, 2012/08/06
[Qemu-devel] [PATCH 17/34] qerror: drop QERR_SOCKET_CONNECT_IN_PROGRESS, Luiz Capitulino, 2012/08/01
[Qemu-devel] [PATCH 16/34] migration: don't rely on QERR_SOCKET_*, Luiz Capitulino, 2012/08/01
[Qemu-devel] [PATCH 09/34] qerror: don't delay error message construction, Luiz Capitulino, 2012/08/01