diff --git a/bufferevent.c b/bufferevent.c index 08c0486c..68b35b1b 100644 --- a/bufferevent.c +++ b/bufferevent.c @@ -876,6 +876,34 @@ bufferevent_setfd(struct bufferevent *bev, evutil_socket_t fd) return res; } +int +bufferevent_replacefd(struct bufferevent *bev, evutil_socket_t fd) +{ + union bufferevent_ctrl_data d; + int err = -1; + evutil_socket_t old_fd = EVUTIL_INVALID_SOCKET; + + BEV_LOCK(bev); + if (bev->be_ops->ctrl) { + err = bev->be_ops->ctrl(bev, BEV_CTRL_GET_FD, &d); + if (!err) { + old_fd = d.fd; + if (old_fd != EVUTIL_INVALID_SOCKET) { + err = evutil_closesocket(old_fd); + } + } + if (!err) { + d.fd = fd; + err = bev->be_ops->ctrl(bev, BEV_CTRL_SET_FD, &d); + } + } + if (err) + event_debug(("%s: cannot replace fd for %p from "EV_SOCK_FMT" to "EV_SOCK_FMT, __func__, bev, old_fd, fd)); + BEV_UNLOCK(bev); + + return err; +} + evutil_socket_t bufferevent_getfd(struct bufferevent *bev) { diff --git a/http-internal.h b/http-internal.h index feaf436d..22836032 100644 --- a/http-internal.h +++ b/http-internal.h @@ -53,7 +53,6 @@ struct evhttp_connection { * server */ TAILQ_ENTRY(evhttp_connection) next; - evutil_socket_t fd; struct bufferevent *bufev; struct event retry_ev; /* for retrying connects */ @@ -174,7 +173,7 @@ struct evhttp { /* XXX most of these functions could be static. */ /* resets the connection; can be reused for more requests */ -void evhttp_connection_reset_(struct evhttp_connection *); +void evhttp_connection_reset_(struct evhttp_connection *, int); /* connects if necessary */ int evhttp_connection_connect_(struct evhttp_connection *); diff --git a/http.c b/http.c index 04f089bc..9d1d5d15 100644 --- a/http.c +++ b/http.c @@ -777,7 +777,7 @@ evhttp_connection_fail_(struct evhttp_connection *evcon, evhttp_request_free_(evcon, req); /* reset the connection */ - evhttp_connection_reset_(evcon); + evhttp_connection_reset_(evcon, 1); /* We are trying the next request that was queued on us */ if (TAILQ_FIRST(&evcon->requests) != NULL) @@ -837,7 +837,7 @@ evhttp_connection_done(struct evhttp_connection *evcon) /* check if we got asked to close the connection */ if (need_close) - evhttp_connection_reset_(evcon); + evhttp_connection_reset_(evcon, 1); if (TAILQ_FIRST(&evcon->requests) != NULL) { /* @@ -1171,7 +1171,7 @@ evhttp_read_cb(struct bufferevent *bufev, void *arg) __func__, EV_SIZE_ARG(total_len))); #endif - evhttp_connection_reset_(evcon); + evhttp_connection_reset_(evcon, 1); } break; case EVCON_DISCONNECTED: @@ -1221,13 +1221,10 @@ void evhttp_connection_free(struct evhttp_connection *evcon) { struct evhttp_request *req; - int need_close = 0; /* notify interested parties that this connection is going down */ - if (evcon->fd != -1) { - if (evhttp_connected(evcon) && evcon->closecb != NULL) - (*evcon->closecb)(evcon, evcon->closecb_arg); - } + if (evhttp_connected(evcon) && evcon->closecb != NULL) + (*evcon->closecb)(evcon, evcon->closecb_arg); /* remove all requests that might be queued on this * connection. for server connections, this should be empty. @@ -1252,20 +1249,9 @@ evhttp_connection_free(struct evhttp_connection *evcon) &evcon->read_more_deferred_cb); if (evcon->bufev != NULL) { - need_close = - !(bufferevent_get_options_(evcon->bufev) & BEV_OPT_CLOSE_ON_FREE); - if (evcon->fd == -1) - evcon->fd = bufferevent_getfd(evcon->bufev); - bufferevent_free(evcon->bufev); } - if (evcon->fd != -1) { - shutdown(evcon->fd, EVUTIL_SHUT_WR); - if (need_close) - evutil_closesocket(evcon->fd); - } - if (evcon->bind_address != NULL) mm_free(evcon->bind_address); @@ -1324,18 +1310,21 @@ evhttp_request_dispatch(struct evhttp_connection* evcon) evhttp_write_buffer(evcon, evhttp_write_connectioncb, NULL); } -/* Reset our connection state: disables reading/writing, closes our fd (if -* any), clears out buffers, and puts us in state DISCONNECTED. */ -void -evhttp_connection_reset_(struct evhttp_connection *evcon) +/** Hard-reset our connection state + * + * This will: + * - reset fd + * - clears out buffers + * - call closecb + */ +static void +evhttp_connection_reset_hard_(struct evhttp_connection *evcon) { struct evbuffer *tmp; int err; - bufferevent_setcb(evcon->bufev, NULL, NULL, NULL, NULL); - /* XXXX This is not actually an optimal fix. Instead we ought to have - an API for "stop connecting", or use bufferevent_setfd to turn off + an API for "stop connecting", or use bufferevent_replacefd to turn off connecting. But for Libevent 2.0, this seems like a minimal change least likely to disrupt the rest of the bufferevent and http code. @@ -1347,19 +1336,12 @@ evhttp_connection_reset_(struct evhttp_connection *evcon) */ bufferevent_disable_hard_(evcon->bufev, EV_READ|EV_WRITE); - if (evcon->fd == -1) - evcon->fd = bufferevent_getfd(evcon->bufev); - - if (evcon->fd != -1) { - /* inform interested parties about connection close */ - if (evhttp_connected(evcon) && evcon->closecb != NULL) - (*evcon->closecb)(evcon, evcon->closecb_arg); + /* inform interested parties about connection close */ + if (evhttp_connected(evcon) && evcon->closecb != NULL) + (*evcon->closecb)(evcon, evcon->closecb_arg); - shutdown(evcon->fd, EVUTIL_SHUT_WR); - evutil_closesocket(evcon->fd); - evcon->fd = -1; - } - err = bufferevent_setfd(evcon->bufev, -1); + /** FIXME: manipulating with fd is unwanted */ + err = bufferevent_replacefd(evcon->bufev, -1); EVUTIL_ASSERT(!err && "setfd"); /* we need to clean up any buffered data */ @@ -1369,9 +1351,26 @@ evhttp_connection_reset_(struct evhttp_connection *evcon) tmp = bufferevent_get_input(evcon->bufev); err = evbuffer_drain(tmp, -1); EVUTIL_ASSERT(!err && "drain input"); +} - evcon->flags &= ~EVHTTP_CON_READING_ERROR; +/** Reset our connection state + * + * This will: + * - disables reading/writing + * - puts us in DISCONNECTED state + * + * @param hard - hard reset will (@see evhttp_connection_reset_hard_()) + */ +void +evhttp_connection_reset_(struct evhttp_connection *evcon, int hard) +{ + bufferevent_setcb(evcon->bufev, NULL, NULL, NULL, NULL); + if (hard) { + evhttp_connection_reset_hard_(evcon); + } + + evcon->flags &= ~EVHTTP_CON_READING_ERROR; evcon->state = EVCON_DISCONNECTED; } @@ -1403,7 +1402,8 @@ evhttp_connection_cb_cleanup(struct evhttp_connection *evcon) { struct evcon_requestq requests; - evhttp_connection_reset_(evcon); + evhttp_connection_reset_(evcon, 1); + if (evcon->retry_max < 0 || evcon->retry_cnt < evcon->retry_max) { struct timeval tv_retry = evcon->initial_retry_timeout; int i; @@ -1481,16 +1481,13 @@ evhttp_error_cb(struct bufferevent *bufev, short what, void *arg) struct evhttp_connection *evcon = arg; struct evhttp_request *req = TAILQ_FIRST(&evcon->requests); - if (evcon->fd == -1) - evcon->fd = bufferevent_getfd(bufev); - switch (evcon->state) { case EVCON_CONNECTING: if (what & BEV_EVENT_TIMEOUT) { event_debug(("%s: connection timeout for \"%s:%d\" on " EV_SOCK_FMT, __func__, evcon->address, evcon->port, - EV_SOCK_ARG(evcon->fd))); + EV_SOCK_ARG(bufferevent_getfd(bufev)))); evhttp_connection_cb_cleanup(evcon); return; } @@ -1526,7 +1523,7 @@ evhttp_error_cb(struct bufferevent *bufev, short what, void *arg) * disconnected. */ EVUTIL_ASSERT(evcon->state == EVCON_IDLE); - evhttp_connection_reset_(evcon); + evhttp_connection_reset_(evcon, 1); /* * If we have no more requests that need completion @@ -1572,11 +1569,6 @@ static void evhttp_connection_cb(struct bufferevent *bufev, short what, void *arg) { struct evhttp_connection *evcon = arg; - int error; - ev_socklen_t errsz = sizeof(error); - - if (evcon->fd == -1) - evcon->fd = bufferevent_getfd(bufev); if (!(what & BEV_EVENT_CONNECTED)) { /* some operating systems return ECONNREFUSED immediately @@ -1591,34 +1583,10 @@ evhttp_connection_cb(struct bufferevent *bufev, short what, void *arg) return; } - if (evcon->fd == -1) { - event_debug(("%s: bufferevent_getfd returned -1", - __func__)); - goto cleanup; - } - - /* Check if the connection completed */ - if (getsockopt(evcon->fd, SOL_SOCKET, SO_ERROR, (void*)&error, - &errsz) == -1) { - event_debug(("%s: getsockopt for \"%s:%d\" on "EV_SOCK_FMT, - __func__, evcon->address, evcon->port, - EV_SOCK_ARG(evcon->fd))); - goto cleanup; - } - - if (error) { - event_debug(("%s: connect failed for \"%s:%d\" on " - EV_SOCK_FMT": %s", - __func__, evcon->address, evcon->port, - EV_SOCK_ARG(evcon->fd), - evutil_socket_error_to_string(error))); - goto cleanup; - } - /* We are connected to the server now */ event_debug(("%s: connected to \"%s:%d\" on "EV_SOCK_FMT"\n", __func__, evcon->address, evcon->port, - EV_SOCK_ARG(evcon->fd))); + EV_SOCK_ARG(bufferevent_getfd(bufev)))); /* Reset the retry count as we were successful in connecting */ evcon->retry_cnt = 0; @@ -2280,7 +2248,7 @@ evhttp_read_firstline(struct evhttp_connection *evcon, if (res == DATA_CORRUPTED || res == DATA_TOO_LONG) { /* Error while reading, terminate */ event_debug(("%s: bad header lines on "EV_SOCK_FMT"\n", - __func__, EV_SOCK_ARG(evcon->fd))); + __func__, EV_SOCK_ARG(bufferevent_getfd(evcon->bufev)))); evhttp_connection_fail_(evcon, EVREQ_HTTP_INVALID_HEADER); return; } else if (res == MORE_DATA_EXPECTED) { @@ -2297,7 +2265,7 @@ evhttp_read_header(struct evhttp_connection *evcon, struct evhttp_request *req) { enum message_read_status res; - evutil_socket_t fd = evcon->fd; + evutil_socket_t fd = bufferevent_getfd(evcon->bufev); res = evhttp_parse_headers_(req, bufferevent_get_input(evcon->bufev)); if (res == DATA_CORRUPTED || res == DATA_TOO_LONG) { @@ -2388,7 +2356,6 @@ evhttp_connection_base_bufferevent_new(struct event_base *base, struct evdns_bas goto error; } - evcon->fd = -1; evcon->port = port; evcon->max_headers_size = EV_SIZE_MAX; @@ -2403,7 +2370,7 @@ evhttp_connection_base_bufferevent_new(struct event_base *base, struct evdns_bas } if (bev == NULL) { - if (!(bev = bufferevent_socket_new(base, -1, 0))) { + if (!(bev = bufferevent_socket_new(base, -1, BEV_OPT_CLOSE_ON_FREE))) { event_warn("%s: bufferevent_socket_new failed", __func__); goto error; } @@ -2571,24 +2538,30 @@ evhttp_connection_connect_(struct evhttp_connection *evcon) if (evcon->state == EVCON_CONNECTING) return (0); - evhttp_connection_reset_(evcon); + /* Do not do hard reset, since this will reset the fd, but someone may + * change some options for it (i.e. setsockopt(), #875) + * + * However don't think that this options will be preserved for all + * connection lifetime, they will be reseted in the following cases: + * - evhttp_connection_set_local_address() + * - evhttp_connection_set_local_port() + * - evhttp_connection_set_retries() + * */ + evhttp_connection_reset_(evcon, 0); EVUTIL_ASSERT(!(evcon->flags & EVHTTP_CON_INCOMING)); evcon->flags |= EVHTTP_CON_OUTGOING; if (evcon->bind_address || evcon->bind_port) { - evcon->fd = bind_socket( - evcon->bind_address, evcon->bind_port, 0 /*reuse*/); - if (evcon->fd == -1) { + int fd = bind_socket(evcon->bind_address, evcon->bind_port, + 0 /*reuse*/); + if (fd == -1) { event_debug(("%s: failed to bind to \"%s\"", __func__, evcon->bind_address)); return (-1); } - if (bufferevent_setfd(evcon->bufev, evcon->fd)) - return (-1); - } else { - if (bufferevent_setfd(evcon->bufev, -1)) + if (bufferevent_replacefd(evcon->bufev, fd)) return (-1); } @@ -2625,7 +2598,7 @@ evhttp_connection_connect_(struct evhttp_connection *evcon) if (ret < 0) { evcon->state = old_state; - event_sock_warn(evcon->fd, "%s: connection to \"%s\" failed", + event_sock_warn(bufferevent_getfd(evcon->bufev), "%s: connection to \"%s\" failed", __func__, evcon->address); /* some operating systems return ECONNREFUSED immediately * when connecting to a local address. the cleanup is going @@ -4265,9 +4238,7 @@ evhttp_get_request_connection( evcon->flags |= EVHTTP_CON_INCOMING; evcon->state = EVCON_READING_FIRSTLINE; - evcon->fd = fd; - - if (bufferevent_setfd(evcon->bufev, fd)) + if (bufferevent_replacefd(evcon->bufev, fd)) goto err; if (bufferevent_enable(evcon->bufev, EV_READ)) goto err; diff --git a/include/event2/bufferevent.h b/include/event2/bufferevent.h index 48cd1535..e4e5c21b 100644 --- a/include/event2/bufferevent.h +++ b/include/event2/bufferevent.h @@ -355,6 +355,18 @@ void bufferevent_getcb(struct bufferevent *bufev, EVENT2_EXPORT_SYMBOL int bufferevent_setfd(struct bufferevent *bufev, evutil_socket_t fd); +/** + Replaces the file descriptor on which the bufferevent operates. + Not supported for all bufferevent types. + + Unlike bufferevent_setfd() it will close previous file descriptor (if any). + + @param bufev the bufferevent object for which to change the file descriptor + @param fd the file descriptor to operate on +*/ +EVENT2_EXPORT_SYMBOL +int bufferevent_replacefd(struct bufferevent *bufev, evutil_socket_t fd); + /** Returns the file descriptor associated with a bufferevent, or -1 if no file descriptor is associated with the bufferevent. diff --git a/include/event2/http.h b/include/event2/http.h index 2a41303e..90f4cf9a 100644 --- a/include/event2/http.h +++ b/include/event2/http.h @@ -721,7 +721,11 @@ void evhttp_connection_free(struct evhttp_connection *evcon); EVENT2_EXPORT_SYMBOL void evhttp_connection_free_on_completion(struct evhttp_connection *evcon); -/** sets the ip address from which http connections are made */ +/** Sets the IP address from which http connections are made + * + * Note this resets internal bufferevent fd, so any options that had been + * installed will be flushed. + */ EVENT2_EXPORT_SYMBOL void evhttp_connection_set_local_address(struct evhttp_connection *evcon, const char *address);