On 09/21/2012 11:03 AM, Markus Armbruster wrote:
> Orit Wasserman <owass...@redhat.com> writes:
>
>> On 09/20/2012 04:14 PM, Markus Armbruster wrote:
>>> Orit Wasserman <owass...@redhat.com> writes:
>>>
>>>> getaddrinfo can give us a list of addresses, but we only try to
>>>> connect to the first one. If that fails we never proceed to
>>>> the next one. This is common on desktop setups that often have ipv6
>>>> configured but not actually working.
>>>>
>>>> To fix this make inet_connect_nonblocking retry connection with a different
>>>> address.
>>>> callers on inet_nonblocking_connect register a callback function that will
>>>> be called when connect opertion completes, in case of failure the fd will
>>>> have
>>>> a negative value
>>>>
>>>> Signed-off-by: Orit Wasserman <owass...@redhat.com>
>>>> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
>>>> ---
>>>> migration-tcp.c | 29 +++++-----------
>>>> qemu-char.c | 2 +-
>>>> qemu-sockets.c | 95
>>>> +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>> qemu_socket.h | 13 ++++++--
>>>> 4 files changed, 102 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/migration-tcp.c b/migration-tcp.c
>>>> index 7f6ad98..cadea36 100644
>>>> --- a/migration-tcp.c
>>>> +++ b/migration-tcp.c
>>>> @@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
>>>> return r;
>>>> }
>>>>
>>>> -static void tcp_wait_for_connect(void *opaque)
>>>> +static void tcp_wait_for_connect(int fd, void *opaque)
>>>> {
>>>> MigrationState *s = opaque;
>>>> - int val, ret;
>>>> - socklen_t valsize = sizeof(val);
>>>>
>>>> - DPRINTF("connect completed\n");
>>>> - do {
>>>> - ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val,
>>>> &valsize);
>>>> - } while (ret == -1 && (socket_error()) == EINTR);
>>>> -
>>>> - if (ret < 0) {
>>>> + if (fd < 0) {
>>>> + DPRINTF("migrate connect error\n");
>>>> + s->fd = -1;
>>>> migrate_fd_error(s);
>>>> - return;
>>>> - }
>>>> -
>>>> - qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>> -
>>>> - if (val == 0)
>>>> + } else {
>>>> + DPRINTF("migrate connect success\n");
>>>> + s->fd = fd;
>>>> migrate_fd_connect(s);
>>>> - else {
>>>> - DPRINTF("error connecting %d\n", val);
>>>> - migrate_fd_error(s);
>>>> }
>>>> }
>>>>
>>>> @@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s,
>>>> const char *host_port,
>>>> s->write = socket_write;
>>>> s->close = tcp_close;
>>>>
>>>> - s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
>>>> + s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
>>>> + &in_progress, errp);
>>>> if (error_is_set(errp)) {
>>>> migrate_fd_error(s);
>>>> return -1;
>>>> @@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s,
>>>> const char *host_port,
>>>>
>>>> if (in_progress) {
>>>> DPRINTF("connect in progress\n");
>>>> - qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>>>> } else {
>>>> migrate_fd_connect(s);
>>>> }
>>>> diff --git a/qemu-char.c b/qemu-char.c
>>>> index c442952..11cd5ef 100644
>>>> --- a/qemu-char.c
>>>> +++ b/qemu-char.c
>>>> @@ -2459,7 +2459,7 @@ static CharDriverState
>>>> *qemu_chr_open_socket(QemuOpts *opts)
>>>> if (is_listen) {
>>>> fd = inet_listen_opts(opts, 0, NULL);
>>>> } else {
>>>> - fd = inet_connect_opts(opts, true, NULL, NULL);
>>>> + fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
>>>> }
>>>> }
>>>> if (fd < 0) {
>>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>>>> index 212075d..d321c58 100644
>>>> --- a/qemu-sockets.c
>>>> +++ b/qemu-sockets.c
>>>> @@ -24,6 +24,7 @@
>>>>
>>>> #include "qemu_socket.h"
>>>> #include "qemu-common.h" /* for qemu_isdigit */
>>>> +#include "main-loop.h"
>>>>
>>>> #ifndef AI_ADDRCONFIG
>>>> # define AI_ADDRCONFIG 0
>>>> @@ -217,11 +218,69 @@ listen:
>>>> ((rc) == -EINPROGRESS)
>>>> #endif
>>>>
>>>> +/* Struct to store connect state for non blocking connect */
>>>> +typedef struct ConnectState {
>>>> + int fd;
>>>> + struct addrinfo *addr_list;
>>>> + struct addrinfo *current_addr;
>>>> + ConnectHandler *callback;
>>>> + void *opaque;
>>>> + Error *errp;
>>>> +} ConnectState;
>>>> +
>>>> static int inet_connect_addr(struct addrinfo *addr, bool block,
>>>> - bool *in_progress)
>>>> + bool *in_progress, ConnectState
>>>> *connect_state);
>>>> +
>>>> +static void wait_for_connect(void *opaque)
>>>> +{
>>>> + ConnectState *s = opaque;
>>>> + int val = 0, rc = 0;
>>>> + socklen_t valsize = sizeof(val);
>>>> + bool in_progress = false;
>>>> +
>>>> + qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>> +
>>>> + do {
>>>> + rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val,
>>>> &valsize);
>>>> + } while (rc == -1 && socket_error() == EINTR);
>>>> +
>>>> + /* update rc to contain error details */
>>>> + if (!rc && val) {
>>>> + rc = -val;
>>>
>>> Would rc = -1 suffice? I'd find that clearer.
>> I guess so, I want the errno for more detailed error message
>> but those will come in another patch set and I can handle it than.
>> I agree that using -1 will make the code much cleaner.
>>>
>>>> + }
>>>> +
>>>> + /* connect error */
>>>> + if (rc < 0) {
>>>> + closesocket(s->fd);
>>>> + s->fd = rc;
>>>> + }
>>>> +
>>>> + /* try to connect to the next address on the list */
>>>> + while (s->current_addr->ai_next != NULL && s->fd < 0) {
>>>> + s->current_addr = s->current_addr->ai_next;
>>>> + s->fd = inet_connect_addr(s->current_addr, false, &in_progress,
>>>> s);
>>>> + /* connect in progress */
>>>> + if (in_progress) {
>>>> + return;
>>>> + }
>>>> + }
>>>> +
>>>> + freeaddrinfo(s->addr_list);
>>>> + if (s->callback) {
>>>> + s->callback(s->fd, s->opaque);
>>>> + }
>>>> + g_free(s);
>>>> + return;
>>>> +}
>>>> +
>>>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>>>> + bool *in_progress, ConnectState
>>>> *connect_state)
>>>
>>> connect_state is needed only for non-blocking connect, isn't it? Could
>>> we drop block and simply use connect_state == NULL instead?
>> it is a good idea !
>>>
>>>> {
>>>> int sock, rc;
>>>>
>>>> + if (in_progress) {
>>>> + *in_progress = false;
>>>> + }
>>>> sock = qemu_socket(addr->ai_family, addr->ai_socktype,
>>>> addr->ai_protocol);
>>>> if (sock < 0) {
>>>> fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>>>> @@ -241,6 +300,9 @@ static int inet_connect_addr(struct addrinfo *addr,
>>>> bool block,
>>>> } while (rc == -EINTR);
>>>>
>>>> if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>>>> + connect_state->fd = sock;
>>>> + qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
>>>> + connect_state);
>>>> if (in_progress) {
>>>> *in_progress = true;
>>>> }
>>>> @@ -259,6 +321,7 @@ static struct addrinfo
>>>> *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>>> const char *port;
>>>>
>>>> memset(&ai, 0, sizeof(ai));
>>>> +
>>>> ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>>>> ai.ai_family = PF_UNSPEC;
>>>> ai.ai_socktype = SOCK_STREAM;
>>>> @@ -291,7 +354,7 @@ static struct addrinfo
>>>> *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>>> }
>>>>
>>>> int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>>> - Error **errp)
>>>> + Error **errp, ConnectState *connect_state)
>>>
>>> Same as for inet_connect_addr(): could we drop block and simply use
>>> connect_state == NULL instead?
>>>
>>>> {
>>>> struct addrinfo *res, *e;
>>>> int sock = -1;
>>>> @@ -301,19 +364,22 @@ int inet_connect_opts(QemuOpts *opts, bool block,
>>>> bool *in_progress,
>>>> return -1;
>>>> }
>>>>
>>>> - if (in_progress) {
>>>> - *in_progress = false;
>>>> - }
>>>> -
>>>> for (e = res; e != NULL; e = e->ai_next) {
>>>> - sock = inet_connect_addr(e, block, in_progress);
>>>> - if (sock >= 0) {
>>>> + if (!block) {
>>>> + connect_state->addr_list = res;
>>>> + connect_state->current_addr = e;
>>>> + }
>>>> + sock = inet_connect_addr(e, block, in_progress, connect_state);
>>>> + if (in_progress && *in_progress) {
>>>> + return sock;
>>>> + } else if (sock >= 0) {
>>>> break;
>>>> }
>>>> }
>>>> if (sock < 0) {
>>>> error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>>> }
>>>
>>> Testing in_progress is fishy: whether the caller passes in_progress or
>>> not shouldn't affect what this function does, only how it reports what
>>> it did.
>>>
>>> inet_connect_addr() either
>>>
>>> 1. completes connect (returns valid fd, sets *in_progress to false), or
>>>
>>> 2. starts connect (returns valid fd, sets *in_progress to true), or
>>>
>>> 3. fails (returns -1 and sets *in_progress to false).
>>>
>>> In case 3, we want to try the next address. If there is none, fail.
>>>
>>> In cases 1 and 2, we want to break the loop and return sock.
>>>
>>> What about:
>>>
>>> for (e = res; sock < 0 && e != NULL; e = e->ai_next) {
>>> if (!block) {
>>> connect_state->addr_list = res;
>>> connect_state->current_addr = e;
>>> }
>>> sock = inet_connect_addr(e, block, in_progress, connect_state);
>>> }
>>> if (sock < 0) {
>>> error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>> }
>>> freeaddrinfo(res);
>>> return sock;
>>>
>>>> + g_free(connect_state);
>>>
>>> connect_state isn't allocated in this function, are you sure you want to
>>> free it here? If yes, are you sure you want to free it only sometimes?
>>>
>> inet_nonblocking connect allocate connect_state.
>> In case connect succeeded/failed immediately than we need to free it
>> (i.e in_progress is true),
>> that the case here.
>> I will move it to inet_nonblocking_connect when I remove in_progress flag.
>>
>> We still need to free in two places in the code ,the second is in
>> tcp_wait_for_connect.
>
> Do you mean wait_for_connect()?
>
> Here's how I now think it's designed to work: inet_connect_opts() frees
> connect_state. Except when connect_state->callback is going to be
> called, it's freed only after the callback returns. In no case, the
> caller or its callback function need to free it.
>
> In short, inet_connect_opts()'s caller only allocates, the free is
> automatic. Needs mention in function comment.
>
> Ways to avoid splitting alloc/free responsibility between
> inet_connect_opts() and its callers:
>
> 1. Move free into caller code. Probably a bad idea, because it needs to
> be done by the caller when in_progress, else by the callback, which
> feels error-prone.
>
> 2. Move allocation into inet_connect_opts(), replace connect_state
> argument by whatever the caller needs put in connect_state. I'm not
> saying you should do this, just throwing out the idea; use it if you
> like it.
>
>>>> freeaddrinfo(res);
>>>> return sock;
>>>> }
>>>> @@ -518,7 +584,7 @@ int inet_connect(const char *str, Error **errp)
>>>>
>>>> opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>>> if (inet_parse(opts, str) == 0) {
>>>> - sock = inet_connect_opts(opts, true, NULL, errp);
>>>> + sock = inet_connect_opts(opts, true, NULL, errp, NULL);
>>>> } else {
>>>> error_set(errp, QERR_SOCKET_CREATE_FAILED);
>>>> }
>>>> @@ -526,16 +592,19 @@ int inet_connect(const char *str, Error **errp)
>>>> return sock;
>>>> }
>>>>
>>>> -
>>>> -int inet_nonblocking_connect(const char *str, bool *in_progress,
>>>> - Error **errp)
>>>> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
>>>> + void *opaque, bool *in_progress, Error
>>>> **errp)
>>>> {
>>>> QemuOpts *opts;
>>>> int sock = -1;
>>>> + ConnectState *connect_state;
>>>>
>>>> opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>>> if (inet_parse(opts, str) == 0) {
>>>> - sock = inet_connect_opts(opts, false, in_progress, errp);
>>>> + connect_state = g_malloc0(sizeof(*connect_state));
>>>> + connect_state->callback = callback;
>>>> + connect_state->opaque = opaque;
>>>> + sock = inet_connect_opts(opts, false, in_progress, errp,
>>>> connect_state);
>>>
>>> May leak connect_state, because inet_connect_opts() doesn't always free
>>> it.
>> it is freed by tcp_wait_for_connect after it calls the user callback function
>
> wait_for_connect(), actually. You're right.
>
> Now I actually understand how this works, let me have another try at
> pointing out allocation errors:
>
> int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
> Error **errp, ConnectState *connect_state)
> {
> struct addrinfo *res, *e;
> int sock = -1;
>
> res = inet_parse_connect_opts(opts, errp);
> if (!res) {
>
> Leaks connect_state.
right missed it, I will add a free here
>
> return -1;
> }
>
> for (e = res; e != NULL; e = e->ai_next) {
> if (!block) {
> connect_state->addr_list = res;
> connect_state->current_addr = e;
> }
> sock = inet_connect_addr(e, block, in_progress, connect_state);
> if (in_progress && *in_progress) {
>
> If inet_connect_addr() only started the connect, it arranged for
> wait_for_connect() to run when the connect completes.
> wait_for_connect() frees connect_state.
>
> If in_progress, we detect this correctly, and refrain from freeing
> connect_state.
>
> If !in_progress, we fail to detect it, and free connect_state(). When
> wait_for_connect() runs, we get a use-after-free, and a double-free.
>
> Possible fixes:
>
> 1. Document caller must pass non-null in_progress for non-blocking
> connect. Recommend assert().
>
> 2. Stick "if (!in_progress) in_progress = &local_in_progress;" before
> the loop.
>
> 3. Move the free into caller code, as described above (still not
> thrilled about that idea).
>
> I'd recommend 1. if you think passing null in_progress for non-blocking
> connect makes no sense. I doubt that's the case.
>
> Else I'd recommend 2.
>
> return sock;
> } else if (sock >= 0) {
> break;
> }
> }
> if (sock < 0) {
> error_set(errp, QERR_SOCKET_CONNECT_FAILED);
> }
> g_free(connect_state);
> freeaddrinfo(res);
> return sock;
> }
>
>
> [...]
>
Well I removed in_progress from inet_nonblocking_connect as mst requested, this
actually helps with
this issue as it becomes a local var in inet_connect_opts.
Orit
