[cc: Juan & Amos]
Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Wed, 1 Aug 2012 22:02:35 -0300
> Luiz Capitulino <lcapitul...@redhat.com> wrote:
>
>> Next commit wants to use this.
>>
>> Signed-off-by: Luiz Capitulino <lcapitul...@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.
