Re: [libgpiod][PATCH] core: deprecate gpiod_needs_update()

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Nov 21, 2019 at 06:31:15PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> 
> This function and the logic behind have been introduced in an early
> version of libgpiod for reasons that have been long forgotten.
> 
> When updating the line info after a line request fails, just propagate
> the error out of the request function instead of setting the internal
> needs_update variable. Drop the entire logic behind gpiod_needs_update(),
> make this routine always return false and mark it as deprecated in the
> header.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> Suggested-by: Kent Gibson <warthog618@xxxxxxxxx>
> ---
> 
> Kent,
> 
> please take a look at this patch. I thought about it and didn't find any
> good reason to keep this function in the API, so I propose to deprecate it
> and propagate any errors from gpiod_line_update() when it's called internally
> as you suggested.
> 

The patch makes total sense to me - though I will now have to update
my set_config and set_flags to match.  You can assume that change will
be in v3 of my patch series - assuming you apply this patch.


>  include/gpiod.h    | 31 +++++++++++--------------------
>  lib/core.c         | 30 ++++++++++++++----------------
>  tests/tests-line.c |  2 --
>  3 files changed, 25 insertions(+), 38 deletions(-)
> 
> diff --git a/include/gpiod.h b/include/gpiod.h
> index 6dfa18a..588198f 100644
> --- a/include/gpiod.h
> +++ b/include/gpiod.h
> @@ -729,33 +729,24 @@ bool gpiod_line_is_open_source(struct gpiod_line *line) GPIOD_API;
>   *         returns -1 and sets the last error number.
>   *
>   * The line info is initially retrieved from the kernel by
> - * gpiod_chip_get_line(). Users can use this line to manually re-read the line
> - * info.
> + * gpiod_chip_get_line() and after every successful request. Users can use
> + * this line to manually re-read the line info.
> + *
> + * We currently have no mechanism provided by the kernel for that and for the
> + * sake of speed and simplicity of this low-level library we don't want to
> + * re-read the line info automatically everytime a property is retrieved. Any
> + * daemon using this library must track the state of lines on its own and call
> + * ::gpiod_line_update if needed.
>   */
>  int gpiod_line_update(struct gpiod_line *line) GPIOD_API;
>  

The second paragraph, when combined with the first paragraph, can imply that we
have no mechanism to re-read line info, which is definitely not what you mean.
Perhaps change "kernel for that" to "kernel to keep the line info synchronized".

Perhaps also highlight that requested lines do keep themselves synced - any
functions that operate on those will update the info as well, so you
shouldn't need to use this function on requested lines.  I think you have
tried to cover that with the "and after every successful request" addition, but
the word "request" is overloaded in this context, making it a bit confusing.
I'd move that addition into a third paragraph that deals with requested lines.

Other than that, the patch looks good to me.

Cheers,
Kent.

>  /**
>   * @brief Check if the line info needs to be updated.
>   * @param line GPIO line object.
> - * @return Returns false if the line is up-to-date. True otherwise.
> - *
> - * The line is updated by calling gpiod_line_update() from within
> - * gpiod_chip_get_line() and on every line request/release. However: an error
> - * returned from gpiod_line_update() only breaks the execution of the former.
> - * The request/release routines only set the internal needs_update flag to true
> - * and continue their execution. This routine allows to check if a line info
> - * update failed at some point and we should call gpiod_line_update()
> - * explicitly.
> - *
> - * This routine will not indicate any potential changes introduced by external
> - * actors (such as a different process requesting the line). We currently have
> - * no mechanism provided by the kernel for that and for the sake of speed and
> - * simplicity of this low-level library we don't want to re-read the line info
> - * automatically everytime a property is retrieved. Any daemon using this
> - * library must track the state of lines on its own and call
> - * ::gpiod_line_update if needed.
> + * @return Deprecated and no longer functional - always returns false.
>   */
> -bool gpiod_line_needs_update(struct gpiod_line *line) GPIOD_API;
> +bool
> +gpiod_line_needs_update(struct gpiod_line *line) GPIOD_API GPIOD_DEPRECATED;
>  
>  /**
>   * @}
> diff --git a/lib/core.c b/lib/core.c
> index d79e52e..a21918c 100644
> --- a/lib/core.c
> +++ b/lib/core.c
> @@ -41,7 +41,6 @@ struct gpiod_line {
>  	bool open_drain;
>  
>  	int state;
> -	bool needs_update;
>  
>  	struct gpiod_chip *chip;
>  	struct line_fd_handle *fd_handle;
> @@ -320,15 +319,6 @@ static int line_get_fd(struct gpiod_line *line)
>  	return line->fd_handle->fd;
>  }
>  
> -static void line_maybe_update(struct gpiod_line *line)
> -{
> -	int rv;
> -
> -	rv = gpiod_line_update(line);
> -	if (rv < 0)
> -		line->needs_update = true;
> -}
> -
>  struct gpiod_chip *gpiod_line_get_chip(struct gpiod_line *line)
>  {
>  	return line->chip;
> @@ -374,9 +364,9 @@ bool gpiod_line_is_open_source(struct gpiod_line *line)
>  	return line->open_source;
>  }
>  
> -bool gpiod_line_needs_update(struct gpiod_line *line)
> +bool gpiod_line_needs_update(struct gpiod_line *line GPIOD_UNUSED)
>  {
> -	return line->needs_update;
> +	return false;
>  }
>  
>  int gpiod_line_update(struct gpiod_line *line)
> @@ -405,8 +395,6 @@ int gpiod_line_update(struct gpiod_line *line)
>  	strncpy(line->name, info.name, sizeof(line->name));
>  	strncpy(line->consumer, info.consumer, sizeof(line->consumer));
>  
> -	line->needs_update = false;
> -
>  	return 0;
>  }
>  
> @@ -537,7 +525,12 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
>  	gpiod_line_bulk_foreach_line(bulk, line, lineptr) {
>  		line->state = LINE_REQUESTED_VALUES;
>  		line_set_fd(line, line_fd);
> -		line_maybe_update(line);
> +
> +		rv = gpiod_line_update(line);
> +		if (rv) {
> +			gpiod_line_release_bulk(bulk);
> +			return rv;
> +		}
>  	}
>  
>  	return 0;
> @@ -577,7 +570,12 @@ static int line_request_event_single(struct gpiod_line *line,
>  
>  	line->state = LINE_REQUESTED_EVENTS;
>  	line_set_fd(line, line_fd);
> -	line_maybe_update(line);
> +
> +	rv = gpiod_line_update(line);
> +	if (rv) {
> +		gpiod_line_release(line);
> +		return rv;
> +	}
>  
>  	return 0;
>  }
> diff --git a/tests/tests-line.c b/tests/tests-line.c
> index 8411132..205c622 100644
> --- a/tests/tests-line.c
> +++ b/tests/tests-line.c
> @@ -78,7 +78,6 @@ GPIOD_TEST_CASE(consumer, 0, { 8 })
>  
>  	ret = gpiod_line_request_input(line, GPIOD_TEST_CONSUMER);
>  	g_assert_cmpint(ret, ==, 0);
> -	g_assert_false(gpiod_line_needs_update(line));
>  	g_assert_cmpstr(gpiod_line_consumer(line), ==, GPIOD_TEST_CONSUMER);
>  }
>  
> @@ -101,7 +100,6 @@ GPIOD_TEST_CASE(consumer_long_string, 0, { 8 })
>  	ret = gpiod_line_request_input(line,
>  			"consumer string over 32 characters long");
>  	g_assert_cmpint(ret, ==, 0);
> -	g_assert_false(gpiod_line_needs_update(line));
>  	g_assert_cmpstr(gpiod_line_consumer(line), ==,
>  			"consumer string over 32 charact");
>  	g_assert_cmpuint(strlen(gpiod_line_consumer(line)), ==, 31);
> -- 
> 2.23.0
>