From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36656) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyU48-0006Wr-22 for qemu-devel@nongnu.org; Mon, 06 Aug 2012 16:38:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SyU46-0002NH-SD for qemu-devel@nongnu.org; Mon, 06 Aug 2012 16:38:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17993) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyU46-0002ND-KN for qemu-devel@nongnu.org; Mon, 06 Aug 2012 16:38:14 -0400 Date: Mon, 6 Aug 2012 16:59:45 -0300 From: Luiz Capitulino Message-ID: <20120806165945.466d81b5@doriath.home> In-Reply-To: <1367352037.189763.1344235944455.JavaMail.root@redhat.com> References: <877gthz41l.fsf@blackfin.pond.sub.org> <1367352037.189763.1344235944455.JavaMail.root@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong Cc: kwolf@redhat.com, aliguori@us.ibm.com, Juan Quintela , Markus Armbruster , mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, pbonzini@redhat.com, eblake@redhat.com On Mon, 6 Aug 2012 02:52:24 -0400 (EDT) Amos Kong wrote: >=20 >=20 > ----- Original Message ----- > > [cc: Juan & Amos] > >=20 > > Luiz Capitulino writes: > >=20 > > > On Wed, 1 Aug 2012 22:02:35 -0300 > > > Luiz Capitulino wrote: > > > > > >> Next commit wants to use this. > > >>=20 > > >> Signed-off-by: Luiz Capitulino > > >> --- > > >>=20 > > >> 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(). > > >>=20 > > >> 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? > >=20 > > Maybe that'll turn out to be useful elsewhere, but not here. > >=20 > > 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. > >=20 > > This is tcp_start_outgoing_migration() now: > >=20 > > int tcp_start_outgoing_migration(MigrationState *s, const char > > *host_port, > > Error **errp) > > { > > s->get_error =3D socket_errno; > > s->write =3D socket_write; > > s->close =3D tcp_close; > >=20 > > s->fd =3D inet_connect(host_port, false, errp); > >=20 > > 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; > > } > >=20 > > return 0; > > } > >=20 > > Cases: > >=20 > > 1. Success > >=20 > > Proceeed to migrate_fd_connect(). > >=20 > > 2. QERR_SOCKET_CONNECT_IN_PROGRESS > >=20 > > connect() failed with -EINPROGRESS. Not actually an error. Set > > up > > appropriate callback. > >=20 > > 3. QERR_SOCKET_CONNECT_FAILED > >=20 > > getaddrinfo() succeeded, but could not connect() to any of the > > addresses. Fail migration with migrate_fd_error(). > >=20 > > 4. QERR_SOCKET_CREATE_FAILED > >=20 > > The error grabbag: > >=20 > > * inet_parse() failed, or > >=20 > > * inet_connect_opts() misses host and/or port (should never > > happen) > >=20 > > * getaddrinfo() failed > >=20 > > Bug: neglects to migrate_fd_error()! Broken in commit d5c5dacc. >=20 >=20 > Before commit d5c5dacc, migrate_fd_error() would not be called > when fail to create socket. >=20 > - s->fd =3D qemu_socket(PF_INET, SOCK_STREAM, 0); > - if (s->fd =3D=3D -1) { > - DPRINTF("Unable to open socket"); > - return -socket_error(); > - } >=20 >=20 > 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. >=20 >=20 > > 5. Anything else > >=20 > > Should never happen. Handled exactly like 4. > >=20 > > If I undo d5c5dacc's damage (untested), it looks like this: > >=20 > > int tcp_start_outgoing_migration(MigrationState *s, const char > > *host_port, > > Error **errp) > > { > > s->get_error =3D socket_errno; > > s->write =3D socket_write; > > s->close =3D tcp_close; > >=20 > > s->fd =3D inet_connect(host_port, false, errp); > >=20 > > 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); >=20 > qemu would hang if socket isn't created successfully. >=20 > > return -1; > > } > >=20 > > return 0; > > } > >=20 > > Et voil=E0, the only error_is_type() is the non-error "in progress", > > and > > your new in_progress covers that already, no need for an errno. > > >=20