core: deprecate gpiod_needs_update()

From: Bartosz Golaszewski <[email protected]>

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.

Suggested-by Kent Gibson <[email protected]>

Signed-off-by Bartosz Golaszewski <[email protected]>

---
v1 -> v2:
- made the documentation of gpiod_line_update() clearer

 include/gpiod.h    | 39 +++++++++++++++++----------------------
 lib/core.c         | 30 ++++++++++++++----------------
 tests/tests-line.c |  2 --
 3 files changed, 31 insertions(+), 40 deletions(-)

include/gpiod.h +17 -22

index 6dfa18a..3fc1c0c 100644
@@ -725,37 +725,32 @@ bool gpiod_line_is_open_source(struct gpiod_line *line) GPIOD_API;
 /**
  * @brief Re-read the line info.
  * @param line GPIO line object.
- * @return 0 is the operation succeeds. In case of an error this routine
+ * @return 0 if the operation succeeds. In case of an error this routine
  *         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 is later re-read after every successful request.
+ * Users can use this function to manually re-read the line info when needed.
+ *
+ * We currently have no mechanism provided by the kernel for keeping the line
+ * info synchronized 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 this routine if needed.
+ *
+ * The state of requested lines is kept synchronized (or rather cannot be
+ * changed by external agents while the ownership of the line is taken) so
+ * there's no need to call this function in that case.
  */
 int gpiod_line_update(struct gpiod_line *line) GPIOD_API;
 
 /**
  * @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.
- */
-bool gpiod_line_needs_update(struct gpiod_line *line) GPIOD_API;
+ * @return Deprecated and no longer functional - always returns false.
+ */
+bool
+gpiod_line_needs_update(struct gpiod_line *line) GPIOD_API GPIOD_DEPRECATED;
 
 /**
  * @}

lib/core.c +14 -16

index d79e52e..a21918c 100644
@@ -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;
 }

tests/tests-line.c +0 -3

index 8411132..205c622 100644
@@ -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

pt., 22 lis 2019 o 11:35 Bartosz Golaszewski <[email protected]> napisał(a):

Quoted 180 lines

From: Bartosz Golaszewski <[email protected]>

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.

Suggested-by Kent Gibson <[email protected]>

Signed-off-by Bartosz Golaszewski <[email protected]>

---
v1 -> v2:
- made the documentation of gpiod_line_update() clearer

 include/gpiod.h    | 39 +++++++++++++++++----------------------
 lib/core.c         | 30 ++++++++++++++----------------
 tests/tests-line.c |  2 --
 3 files changed, 31 insertions(+), 40 deletions(-)

include/gpiod.h +17 -22

index 6dfa18a..3fc1c0c 100644
@@ -725,37 +725,32 @@ bool gpiod_line_is_open_source(struct gpiod_line *line) GPIOD_API;
 /**
  * @brief Re-read the line info.
  * @param line GPIO line object.
- * @return 0 is the operation succeeds. In case of an error this routine
+ * @return 0 if the operation succeeds. In case of an error this routine
  *         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 is later re-read after every successful request.
+ * Users can use this function to manually re-read the line info when needed.
+ *
+ * We currently have no mechanism provided by the kernel for keeping the line
+ * info synchronized 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 this routine if needed.
+ *
+ * The state of requested lines is kept synchronized (or rather cannot be
+ * changed by external agents while the ownership of the line is taken) so
+ * there's no need to call this function in that case.
  */
 int gpiod_line_update(struct gpiod_line *line) GPIOD_API;
 
 /**
  * @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.
- */
-bool gpiod_line_needs_update(struct gpiod_line *line) GPIOD_API;
+ * @return Deprecated and no longer functional - always returns false.
+ */
+bool
+gpiod_line_needs_update(struct gpiod_line *line) GPIOD_API GPIOD_DEPRECATED;
 
 /**
  * @}

lib/core.c +14 -16

index d79e52e..a21918c 100644
@@ -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;
 }

tests/tests-line.c +0 -3

index 8411132..205c622 100644
@@ -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);
--

Quoted 2 lines

2.23.0

Seems I missed the [libgpiod] tag. Anyway - patch applied.

Kent: hopefully it won't be too much work for you to rebase on top of current master. Otherwise I can probably fix the patches up myself.

Quoted 186 lines

pt., 22 lis 2019 o 11:35 Bartosz Golaszewski <[email protected]> napisał(a):

Quoted 180 lines

From: Bartosz Golaszewski <[email protected]>

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.

Suggested-by Kent Gibson <[email protected]>

Signed-off-by Bartosz Golaszewski <[email protected]>

---
v1 -> v2:
- made the documentation of gpiod_line_update() clearer

 include/gpiod.h    | 39 +++++++++++++++++----------------------
 lib/core.c         | 30 ++++++++++++++----------------
 tests/tests-line.c |  2 --
 3 files changed, 31 insertions(+), 40 deletions(-)

include/gpiod.h +17 -22

index 6dfa18a..3fc1c0c 100644
@@ -725,37 +725,32 @@ bool gpiod_line_is_open_source(struct gpiod_line *line) GPIOD_API;
 /**
  * @brief Re-read the line info.
  * @param line GPIO line object.
- * @return 0 is the operation succeeds. In case of an error this routine
+ * @return 0 if the operation succeeds. In case of an error this routine
  *         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 is later re-read after every successful request.
+ * Users can use this function to manually re-read the line info when needed.
+ *
+ * We currently have no mechanism provided by the kernel for keeping the line
+ * info synchronized 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 this routine if needed.
+ *
+ * The state of requested lines is kept synchronized (or rather cannot be
+ * changed by external agents while the ownership of the line is taken) so
+ * there's no need to call this function in that case.
  */
 int gpiod_line_update(struct gpiod_line *line) GPIOD_API;
 
 /**
  * @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.
- */
-bool gpiod_line_needs_update(struct gpiod_line *line) GPIOD_API;
+ * @return Deprecated and no longer functional - always returns false.
+ */
+bool
+gpiod_line_needs_update(struct gpiod_line *line) GPIOD_API GPIOD_DEPRECATED;
 
 /**
  * @}

lib/core.c +14 -16

index d79e52e..a21918c 100644
@@ -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;
 }

tests/tests-line.c +0 -3

index 8411132..205c622 100644
@@ -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);
--

Quoted 2 lines

2.23.0

Seems I missed the [libgpiod] tag. Anyway - patch applied.

Quoted 3 lines

Kent: hopefully it won't be too much work for you to rebase on top of current master. Otherwise I can probably fix the patches up myself.

No problem at all. I've rebased my pre-v3 on the current master@296441, and made the corresponding changes to set_config and set_flags. Those changes are also committed to my development branch on github, if you want a preview.

Do you want to review the v2 or should I submit the v3 as is?

Cheers,
Kent.