From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49275) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwyaI-0002lW-4B for qemu-devel@nongnu.org; Thu, 02 Aug 2012 12:49:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwyaG-0000u2-LU for qemu-devel@nongnu.org; Thu, 02 Aug 2012 12:49:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37221) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwyaG-0000tx-D8 for qemu-devel@nongnu.org; Thu, 02 Aug 2012 12:49:12 -0400 Date: Thu, 2 Aug 2012 13:49:39 -0300 From: Luiz Capitulino Message-ID: <20120802134939.5be5df44@doriath.home> In-Reply-To: <877gthz41l.fsf@blackfin.pond.sub.org> References: <1343869374-23417-1-git-send-email-lcapitulino@redhat.com> <1343869374-23417-16-git-send-email-lcapitulino@redhat.com> <20120802104135.546a2758@doriath.home> <877gthz41l.fsf@blackfin.pond.sub.org> 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: Markus Armbruster Cc: kwolf@redhat.com, aliguori@us.ibm.com, Juan Quintela , mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, pbonzini@redhat.com, Amos Kong , eblake@redhat.com On Thu, 02 Aug 2012 17:50:30 +0200 Markus Armbruster wrote: > [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 caus= e, > >> 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 > 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); > 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. 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 bu= g. I'll do what you suggest.