Re: [Qemu-devel] [PATCH 15/34] net: inet_connect(), inet_connect_opts(): return -errno

[Markus Armbruster]

[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.

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.