Re: [libgpiod v2][PATCH] chip: don't set the request's file descriptor to non-blocking

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Apr 26, 2022 at 01:48:50PM +0200, Bartosz Golaszewski wrote:
> Make the behavior consistent with the documentation and don't set the
> line request's fd to non-blocking. Add two more test-cases that make
> sure the library behaves correctly.
> 
> Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> ---
>  lib/chip.c               |  23 --------
>  tests/tests-edge-event.c | 119 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 119 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/chip.c b/lib/chip.c
> index eef3be2..fc3dda2 100644
> --- a/lib/chip.c
> +++ b/lib/chip.c
> @@ -181,23 +181,6 @@ GPIOD_API int gpiod_chip_get_line_offset_from_name(struct gpiod_chip *chip,
>  	return -1;
>  }
>  
> -static int set_fd_noblock(int fd)
> -{
> -	int ret, flags;
> -
> -	flags = fcntl(fd, F_GETFL, 0);
> -	if (flags < 0)
> -		return -1;
> -
> -	flags |= O_NONBLOCK;
> -
> -	ret = fcntl(fd, F_SETFL, flags);
> -	if (ret < 0)
> -		return -1;
> -
> -	return 0;
> -}
> -
>  GPIOD_API struct gpiod_line_request *
>  gpiod_chip_request_lines(struct gpiod_chip *chip,
>  			 struct gpiod_request_config *req_cfg,
> @@ -222,12 +205,6 @@ gpiod_chip_request_lines(struct gpiod_chip *chip,
>  	if (ret < 0)
>  		return NULL;
>  
> -	ret = set_fd_noblock(uapi_req.fd);
> -	if (ret) {
> -		close(uapi_req.fd);
> -		return NULL;
> -	}
> -
>  	request = gpiod_line_request_from_uapi(&uapi_req);
>  	if (!request) {
>  		close(uapi_req.fd);
> diff --git a/tests/tests-edge-event.c b/tests/tests-edge-event.c
> index 306383f..987155f 100644
> --- a/tests/tests-edge-event.c
> +++ b/tests/tests-edge-event.c
> @@ -357,6 +357,75 @@ GPIOD_TEST_CASE(read_rising_edge_event_polled)
>  	g_thread_join(thread);
>  }
>  
> +GPIOD_TEST_CASE(read_both_events_blocking)
> +{
> +	/*
> +	 * This time without polling so that the read gets a chance to block
> +	 * and we can make sure it doesn't immediately return an error.
> +	 */
> +
> +	static const guint offset = 2;
> +
> +	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 8, NULL);
> +	g_autoptr(struct_gpiod_chip) chip = NULL;
> +	g_autoptr(struct_gpiod_request_config) req_cfg = NULL;
> +	g_autoptr(struct_gpiod_line_config) line_cfg = NULL;
> +	g_autoptr(struct_gpiod_line_request) request = NULL;
> +	g_autoptr(GThread) thread = NULL;
> +	g_autoptr(struct_gpiod_edge_event_buffer) buffer = NULL;
> +	struct gpiod_edge_event *event;
> +	gint ret;
> +
> +	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
> +	req_cfg = gpiod_test_create_request_config_or_fail();
> +	line_cfg = gpiod_test_create_line_config_or_fail();
> +	buffer = gpiod_test_create_edge_event_buffer_or_fail(64);
> +
> +	gpiod_request_config_set_offsets(req_cfg, 1, &offset);
> +	gpiod_line_config_set_direction_default(line_cfg,
> +						GPIOD_LINE_DIRECTION_INPUT);
> +	gpiod_line_config_set_edge_detection_default(line_cfg,
> +						     GPIOD_LINE_EDGE_BOTH);
> +
> +	request = gpiod_test_request_lines_or_fail(chip, req_cfg, line_cfg);
> +
> +	thread = g_thread_new("request-release",
> +			      falling_and_rising_edge_events, sim);
> +	g_thread_ref(thread);
> +

Not a fan of using usleep in the tests (not shown here but in
falling_and_rising_edge_events()) to get the background thread to
generate events after the main thread is waiting - would prefer to flip
that around so that the background thread blocks, and can be shown to be
blocked, and the main thread generates the events and checks that the
background thread got them, so the handshaking doesn't involve sleeps.

> +	/* First event. */
> +
> +	ret = gpiod_line_request_read_edge_event(request, buffer, 1);
> +	g_assert_cmpint(ret, ==, 1);
> +	gpiod_test_join_thread_and_return_if_failed(thread);
> +
> +	g_assert_cmpuint(gpiod_edge_event_buffer_get_num_events(buffer), ==, 1);
> +	event = gpiod_edge_event_buffer_get_event(buffer, 0);
> +	g_assert_nonnull(event);
> +	gpiod_test_join_thread_and_return_if_failed(thread);
> +
> +	g_assert_cmpint(gpiod_edge_event_get_event_type(event),
> +			==, GPIOD_EDGE_EVENT_RISING_EDGE);
> +	g_assert_cmpuint(gpiod_edge_event_get_line_offset(event), ==, 2);
> +
> +	/* Second event. */
> +
> +	ret = gpiod_line_request_read_edge_event(request, buffer, 1);
> +	g_assert_cmpint(ret, ==, 1);
> +	gpiod_test_join_thread_and_return_if_failed(thread);
> +
> +	g_assert_cmpuint(gpiod_edge_event_buffer_get_num_events(buffer), ==, 1);
> +	event = gpiod_edge_event_buffer_get_event(buffer, 0);
> +	g_assert_nonnull(event);
> +	gpiod_test_join_thread_and_return_if_failed(thread);
> +
> +	g_assert_cmpint(gpiod_edge_event_get_event_type(event),
> +			==, GPIOD_EDGE_EVENT_FALLING_EDGE);
> +	g_assert_cmpuint(gpiod_edge_event_get_line_offset(event), ==, 2);
> +
> +	g_thread_join(thread);
> +}
> +
>  static gpointer rising_edge_events_on_two_offsets(gpointer data)
>  {
>  	GPIOSimChip *sim = data;
> @@ -488,3 +557,53 @@ GPIOD_TEST_CASE(event_copy)
>  	g_assert_nonnull(copy);
>  	g_assert_true(copy != event);
>  }
> +
> +GPIOD_TEST_CASE(reading_more_events_than_the_queue_contains_doesnt_block)
> +{
> +	static const guint offset = 2;
> +
> +	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 8, NULL);
> +	g_autoptr(struct_gpiod_chip) chip = NULL;
> +	g_autoptr(struct_gpiod_request_config) req_cfg = NULL;
> +	g_autoptr(struct_gpiod_line_config) line_cfg = NULL;
> +	g_autoptr(struct_gpiod_line_request) request = NULL;
> +	g_autoptr(GThread) thread = NULL;
> +	g_autoptr(struct_gpiod_edge_event_buffer) buffer = NULL;
> +	gint ret;
> +
> +	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
> +	req_cfg = gpiod_test_create_request_config_or_fail();
> +	line_cfg = gpiod_test_create_line_config_or_fail();
> +	buffer = gpiod_test_create_edge_event_buffer_or_fail(64);
> +
> +	gpiod_request_config_set_offsets(req_cfg, 1, &offset);
> +	gpiod_line_config_set_direction_default(line_cfg,
> +						GPIOD_LINE_DIRECTION_INPUT);
> +	gpiod_line_config_set_edge_detection_default(line_cfg,
> +						     GPIOD_LINE_EDGE_BOTH);
> +
> +	request = gpiod_test_request_lines_or_fail(chip, req_cfg, line_cfg);
> +
> +	g_gpiosim_chip_set_pull(sim, 2, G_GPIOSIM_PULL_UP);
> +	g_usleep(500);
> +	g_gpiosim_chip_set_pull(sim, 2, G_GPIOSIM_PULL_DOWN);
> +	g_usleep(500);
> +	g_gpiosim_chip_set_pull(sim, 2, G_GPIOSIM_PULL_UP);
> +	g_usleep(500);
> +	g_gpiosim_chip_set_pull(sim, 2, G_GPIOSIM_PULL_DOWN);
> +	g_usleep(500);
> +	g_gpiosim_chip_set_pull(sim, 2, G_GPIOSIM_PULL_UP);
> +	g_usleep(500);
> +	g_gpiosim_chip_set_pull(sim, 2, G_GPIOSIM_PULL_DOWN);
> +	g_usleep(500);
> +	g_gpiosim_chip_set_pull(sim, 2, G_GPIOSIM_PULL_UP);
> +	g_usleep(500);
> +

What is the sleep waiting for?
Why 500?
Is 1 sufficient?
Is there a critical threshold?
Perhaps that should be incorporated into g_gpiosim_chip_set_pull()?

> +	ret = gpiod_line_request_read_edge_event(request, buffer, 12);
> +	g_assert_cmpint(ret, ==, 7);
> +	gpiod_test_return_if_failed();
> +
> +	ret = gpiod_line_request_wait_edge_event(request, 1000);
> +	g_assert_cmpint(ret, ==, 0);
> +	gpiod_test_return_if_failed();
> +}
> -- 
> 2.32.0
> 

I'm fine with the code change.

As written the tests are probably ok, but to be more robust I would rework
them - arbitrary sleeps in tests are a red flag for me.
That is probably outside the scope of this patch though, and getting the
code change applied is far more important, so...

Reviewed-by: Kent Gibson <warthog618@xxxxxxxxx>

Cheers,
Kent.