Hi,
at first glance I didn't like returning fd from wl_connection_destroy, but
at the other, I did!
If you think about the connection as a buffer for the fd (and that is
really the case),
then it make sense to do something like:
create conn ------------------ destroy conn
fd ----------------------> | connection | ------------------------> fd
------------------
The race in threaded environment is real (one thread closes fd, say 6,
other thread opens new fd
- it will get the first free value which is 6 - and the first thread close
the 6 again and we're in trouble..)
So it's important to close every fd only once.
For me it's OK and:
Reviewed-by: Marek Chalupa <mchqwe...@gmail.com>
On 30 September 2014 14:43, Benjamin Herr <b...@0x539.de> wrote:
> Calling close() on the same file descriptor that a previous call to
> close() already closed is wrong, and racy if another thread received
> that same file descriptor as a eg. new socket or actual file.
>
> There are two situations where wl_connection_destroy() would close its
> file descriptor and then another function up in the call chain would
> close the same file descriptor:
>
> * When wl_client_create() fails after calling wl_connection_create(),
> it will call wl_connection_destroy() before returning. However, its
> caller will always close the file descriptor if wl_client_create()
> fails.
>
> * wl_display_disconnect() unconditionally closes the display file
> descriptor and also calls wl_connection_destroy().
>
> So these two seem to expect wl_connection_destroy() to leave the file
> descriptor open. The other caller of wl_connection_destroy(),
> wl_client_destroy(), does however expect wl_connection_destroy() to
> close its file descriptor, alas.
>
> This patch changes wl_connection_destroy() to indulge this majority of
> two callers by simply not closing the file descriptor. For the benefit
> of wl_client_destroy(), wl_connection_destroy() then returns the
> unclosed file descriptor so that wl_client_destroy() can close it
> itself.
>
> Since wl_connection_destroy() is a private function called from few
> places, changing its semantics seemed like the more expedient way to
> address the double-close() problem than shuffling around the logic in
> wl_client_create() to somehow enable it to always avoid calling
> wl_connection_destroy().
>
> Signed-off-by: Benjamin Herr <b...@0x539.de>
> ---
> src/connection.c | 7 +++++--
> src/wayland-private.h | 2 +-
> src/wayland-server.c | 2 +-
> tests/connection-test.c | 8 ++++++--
> 4 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/src/connection.c b/src/connection.c
> index f292853..c5daca6 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -189,13 +189,16 @@ close_fds(struct wl_buffer *buffer, int max)
> buffer->tail += size;
> }
>
> -void
> +int
> wl_connection_destroy(struct wl_connection *connection)
> {
> + int fd = connection->fd;
> +
> close_fds(&connection->fds_out, -1);
> close_fds(&connection->fds_in, -1);
> - close(connection->fd);
> free(connection);
> +
> + return fd;
> }
>
> void
> diff --git a/src/wayland-private.h b/src/wayland-private.h
> index 67e8783..db76081 100644
> --- a/src/wayland-private.h
> +++ b/src/wayland-private.h
> @@ -86,7 +86,7 @@ int wl_interface_equal(const struct wl_interface *iface1,
> const struct wl_interface *iface2);
>
> struct wl_connection *wl_connection_create(int fd);
> -void wl_connection_destroy(struct wl_connection *connection);
> +int wl_connection_destroy(struct wl_connection *connection);
> void wl_connection_copy(struct wl_connection *connection, void *data,
> size_t size);
> void wl_connection_consume(struct wl_connection *connection, size_t size);
>
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 674aeca..7caeb30 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -670,7 +670,7 @@ wl_client_destroy(struct wl_client *client)
> wl_map_for_each(&client->objects, destroy_resource, &serial);
> wl_map_release(&client->objects);
> wl_event_source_remove(client->source);
> - wl_connection_destroy(client->connection);
> + close(wl_connection_destroy(client->connection));
> wl_list_remove(&client->link);
> free(client);
> }
> diff --git a/tests/connection-test.c b/tests/connection-test.c
> index 659bf68..4497128 100644
> --- a/tests/connection-test.c
> +++ b/tests/connection-test.c
> @@ -57,6 +57,7 @@ TEST(connection_create)
>
> connection = setup(s);
> wl_connection_destroy(connection);
> + close(s[0]);
> close(s[1]);
> }
>
> @@ -74,6 +75,7 @@ TEST(connection_write)
> assert(memcmp(message, buffer, sizeof message) == 0);
>
> wl_connection_destroy(connection);
> + close(s[0]);
> close(s[1]);
> }
>
> @@ -92,6 +94,7 @@ TEST(connection_data)
> wl_connection_consume(connection, sizeof message);
>
> wl_connection_destroy(connection);
> + close(s[0]);
> close(s[1]);
> }
>
> @@ -117,6 +120,7 @@ TEST(connection_queue)
> assert(memcmp(message, buffer + sizeof message, sizeof message) ==
> 0);
>
> wl_connection_destroy(connection);
> + close(s[0]);
> close(s[1]);
> }
>
> @@ -147,8 +151,8 @@ setup_marshal_data(struct marshal_data *data)
> static void
> release_marshal_data(struct marshal_data *data)
> {
> - wl_connection_destroy(data->read_connection);
> - wl_connection_destroy(data->write_connection);
> + close(wl_connection_destroy(data->read_connection));
> + close(wl_connection_destroy(data->write_connection));
> }
>
> static void
> --
> 2.1.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
