| Message ID | 1343869374-23417-16-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 1 Aug 2012 22:02:35 -0300 Luiz Capitulino <lcapitulino@redhat.com> wrote: > Next commit wants to use this. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > > 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?
[cc: Juan & Amos] Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 1 Aug 2012 22:02:35 -0300 > Luiz Capitulino <lcapitulino@redhat.com> wrote: > >> Next commit wants to use this. >> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >> --- >> >> 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.
On Thu, 02 Aug 2012 17:50:30 +0200 Markus Armbruster <armbru@redhat.com> wrote: > [cc: Juan & Amos] > > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Wed, 1 Aug 2012 22:02:35 -0300 > > Luiz Capitulino <lcapitulino@redhat.com> wrote: > > > >> Next commit wants to use this. > >> > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > >> --- > >> > >> 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.
----- Original Message ----- > [cc: Juan & Amos] > > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Wed, 1 Aug 2012 22:02:35 -0300 > > Luiz Capitulino <lcapitulino@redhat.com> wrote: > > > >> Next commit wants to use this. > >> > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > >> --- > >> > >> 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. >
On Mon, 6 Aug 2012 02:52:24 -0400 (EDT) Amos Kong <akong@redhat.com> wrote: > > > ----- Original Message ----- > > [cc: Juan & Amos] > > > > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > > > On Wed, 1 Aug 2012 22:02:35 -0300 > > > Luiz Capitulino <lcapitulino@redhat.com> wrote: > > > > > >> Next commit wants to use this. > > >> > > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > >> --- > > >> > > >> 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. > > >
diff --git a/qemu-sockets.c b/qemu-sockets.c index 33b65cf..82f4736 100644 --- a/qemu-sockets.c +++ b/qemu-sockets.c @@ -234,7 +234,7 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) if (addr == NULL || port == NULL) { fprintf(stderr, "inet_connect: host and/or port not specified\n"); error_set(errp, QERR_SOCKET_CREATE_FAILED); - return -1; + return -EINVAL; } if (qemu_opt_get_bool(opts, "ipv4", 0)) @@ -247,7 +247,7 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, gai_strerror(rc)); error_set(errp, QERR_SOCKET_CREATE_FAILED); - return -1; + return -EINVAL; } for (e = res; e != NULL; e = e->ai_next) { @@ -300,7 +300,7 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) } error_set(errp, QERR_SOCKET_CONNECT_FAILED); freeaddrinfo(res); - return -1; + return -ENOTCONN; } int inet_dgram_opts(QemuOpts *opts) @@ -508,6 +508,7 @@ int inet_connect(const char *str, bool block, bool *in_progress, Error **errp) } sock = inet_connect_opts(opts, in_progress, errp); } else { + sock = -EINVAL; error_set(errp, QERR_SOCKET_CREATE_FAILED); } qemu_opts_del(opts);
Next commit wants to use this. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- 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(). qemu-sockets.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)