core: rework gpiod_chip_find_line()
authorBartosz Golaszewski <bgolaszewski@baylibre.com>
Wed, 2 Dec 2020 14:18:36 +0000 (15:18 +0100)
committerBartosz Golaszewski <bgolaszewski@baylibre.com>
Mon, 14 Dec 2020 14:57:07 +0000 (15:57 +0100)
GPIO line names are not unique and the library has been incorrectly
making such assumption in v1.x series. We've already dropped interfaces
that would be too complicated to maintain when taking this into account.

Let's now rework the remaming API. This introduces a new function to the
C API: gpiod_chip_find_line_unique() which assumes that the line name is
unique and signals an error if it's not. The previous
gpiod_chip_find_line() function now returns a bulk object containing all
matching lines.

Python and C++ bindings are updated: their find_line() functions return
bulk objects (or an ::std::vector> too now and they take an additional
argument specifying whether we're looking for a unique line or not.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
bindings/cxx/chip.cpp
bindings/cxx/examples/gpiofindcxx.cpp
bindings/cxx/gpiod.hpp
bindings/cxx/tests/tests-chip.cpp
bindings/python/gpiodmodule.c
bindings/python/tests/gpiod_py_test.py
include/gpiod.h
lib/helpers.c
tests/tests-chip.c
tools/gpiofind.c

index ff35e532a1711192bd0dd87b731c4df02067ae84..82ba559a1ecb02607fee618eecdc5c11524d8009 100644 (file)
@@ -122,16 +122,22 @@ line chip::get_line(unsigned int offset) const
        return line(line_handle, *this);
 }
 
-line chip::find_line(const ::std::string& name) const
+::std::vector<line> chip::find_line(const ::std::string& name, bool unique) const
 {
        this->throw_if_noref();
 
-       ::gpiod_line* handle = ::gpiod_chip_find_line(this->_m_chip.get(), name.c_str());
-       if (!handle && errno != ENOENT)
-               throw ::std::system_error(errno, ::std::system_category(),
-                                         "error looking up GPIO line by name");
+       ::std::vector<line> lines;
+
+       for (auto& line: ::gpiod::line_iter(*this)) {
+               if (line.name() == name)
+                       lines.push_back(line);
+       }
 
-       return handle ? line(handle, *this) : line();
+       if (unique && lines.size() > 1)
+               throw ::std::system_error(ERANGE, ::std::system_category(),
+                                         "multiple lines with the same name found");
+
+       return lines;
 }
 
 line_bulk chip::get_lines(const ::std::vector<unsigned int>& offsets) const
index e9ab64aeb0b904e513f81690dfc5497dcf947bde..c817378edbf79f075e500f24befd7be2da34d5f5 100644 (file)
@@ -20,9 +20,9 @@ int main(int argc, char **argv)
        }
 
        for (auto& chip: ::gpiod::make_chip_iter()) {
-               auto line = chip.find_line(argv[1]);
-               if (line) {
-                       ::std::cout << line.name() << " " << line.offset() << ::std::endl;
+               auto lines = chip.find_line(argv[1], true);
+               if (!lines.empty()) {
+                       ::std::cout << lines.front().name() << " " << lines.front().offset() << ::std::endl;
                        return EXIT_SUCCESS;
                }
        }
index a16a27c6caa950bd447f4b595e8589d27696897a..9d081fecaebd13b9e582080f9d35d07d50230893 100644 (file)
@@ -127,11 +127,15 @@ public:
        GPIOD_API line get_line(unsigned int offset) const;
 
        /**
-        * @brief Get the line exposed by this chip by name.
+        * @brief Find all GPIO lines by name among lines exposed by this GPIO
+        *        chip.
         * @param name Line name.
-        * @return Line object.
+        * @param unique If set to true: throw an error if multiple lines match
+        *               the name.
+        * @return Vector of all matching lines.
         */
-       GPIOD_API line find_line(const ::std::string& name) const;
+       GPIOD_API ::std::vector<line> find_line(const ::std::string& name,
+                                               bool unique = false) const;
 
        /**
         * @brief Get a set of lines exposed by this chip at given offsets.
index 90ebc1ba322985d82a8aa161dea5d90abdfa8031..c45f2df7432e33c0c2c269a68ddd66e9470ecb0d 100644 (file)
@@ -168,7 +168,7 @@ TEST_CASE("Lines can be retrieved from chip objects", "[chip]")
 
        SECTION("find single line by name")
        {
-               auto line = chip.find_line("gpio-mockup-B-3");
+               auto line = chip.find_line("gpio-mockup-B-3", true).front();
                REQUIRE(line.offset() == 3);
        }
 
@@ -223,6 +223,6 @@ TEST_CASE("Errors occurring when retrieving lines are correctly reported", "[chi
 
        SECTION("line not found by name")
        {
-               REQUIRE_FALSE(chip.find_line("nonexistent-line"));
+               REQUIRE(chip.find_line("nonexistent-line").empty());
        }
 }
index 4948d5df0a6113f543f3c0de016b48f7b8b62e38..d183e6f24a90d40123440ffe385433c1b8518f63 100644 (file)
@@ -2148,61 +2148,115 @@ gpiod_Chip_get_line(gpiod_ChipObject *self, PyObject *args)
        return gpiod_MakeLineObject(self, line);
 }
 
+static gpiod_LineBulkObject *gpiod_ListToLineBulk(PyObject *lines)
+{
+       gpiod_LineBulkObject *bulk;
+       PyObject *arg;
+
+       arg = PyTuple_Pack(1, lines);
+       if (!arg)
+               return NULL;
+
+       bulk = (gpiod_LineBulkObject *)PyObject_CallObject(
+                                       (PyObject *)&gpiod_LineBulkType,
+                                       arg);
+       Py_DECREF(arg);
+
+       return bulk;
+}
+
+static gpiod_LineBulkObject *
+gpiod_LineBulkObjectFromBulk(gpiod_ChipObject *chip, struct gpiod_line_bulk *bulk)
+{
+       gpiod_LineBulkObject *bulk_obj;
+       gpiod_LineObject *line_obj;
+       struct gpiod_line *line;
+       unsigned int idx;
+       PyObject *list;
+       int rv;
+
+       list = PyList_New(gpiod_line_bulk_num_lines(bulk));
+       if (!list)
+               return NULL;
+
+       for (idx = 0; idx < gpiod_line_bulk_num_lines(bulk); idx++) {
+               line = gpiod_line_bulk_get_line(bulk, idx);
+               line_obj = gpiod_MakeLineObject(chip, line);
+               if (!line_obj) {
+                       Py_DECREF(list);
+                       return NULL;
+               }
+
+               rv = PyList_SetItem(list, idx, (PyObject *)line_obj);
+               if (rv < 0) {
+                       Py_DECREF(line_obj);
+                       Py_DECREF(list);
+                       return NULL;
+               }
+       }
+
+       bulk_obj = gpiod_ListToLineBulk(list);
+       Py_DECREF(list);
+       if (!bulk_obj)
+               return NULL;
+
+       return bulk_obj;
+}
+
 PyDoc_STRVAR(gpiod_Chip_find_line_doc,
-"find_line(name) -> gpiod.Line object or None\n"
+"find_line(name) -> gpiod.LineBulk object or None\n"
 "\n"
-"Get the GPIO line by name.\n"
+"Find all GPIO lines by name among lines exposed by this GPIO chip..\n"
 "\n"
 "  name\n"
 "    Line name (string)\n"
+"  unique\n"
+"    Indicates whether an exception should be raised if more than one lines\n"
+"    matches the name\n"
 "\n"
-"Returns a gpiod.Line object or None if line with given name is not\n"
-"associated with this chip.");
+"Returns a gpiod.LineBulk object containing all matching lines or None if\n"
+"line with given name is not associated with this chip.");
 
-static gpiod_LineObject *
-gpiod_Chip_find_line(gpiod_ChipObject *self, PyObject *args)
+static gpiod_LineBulkObject *
+gpiod_Chip_find_line(gpiod_ChipObject *self, PyObject *args, PyObject *kwds)
 {
-       struct gpiod_line *line;
+       static char *kwlist[] = { "unique", NULL };
+
+       gpiod_LineBulkObject *bulk_obj;
+       struct gpiod_line_bulk *bulk;
+       int rv, unique = 0;
        const char *name;
-       int rv;
 
        if (gpiod_ChipIsClosed(self))
                return NULL;
 
-       rv = PyArg_ParseTuple(args, "s", &name);
+       rv = PyArg_ParseTupleAndKeywords(args, kwds, "s|p",
+                                        kwlist, &name, &unique);
        if (!rv)
                return NULL;
 
        Py_BEGIN_ALLOW_THREADS;
-       line = gpiod_chip_find_line(self->chip, name);
+       bulk = gpiod_chip_find_line(self->chip, name);
        Py_END_ALLOW_THREADS;
-       if (!line) {
+       if (!bulk) {
                if (errno == ENOENT) {
                        Py_INCREF(Py_None);
-                       return (gpiod_LineObject *)Py_None;
+                       return (gpiod_LineBulkObject *)Py_None;
                }
 
-               return (gpiod_LineObject *)PyErr_SetFromErrno(PyExc_OSError);
+               return (gpiod_LineBulkObject *)PyErr_SetFromErrno(
+                                                       PyExc_OSError);
        }
 
-       return gpiod_MakeLineObject(self, line);
-}
-
-static gpiod_LineBulkObject *gpiod_ListToLineBulk(PyObject *lines)
-{
-       gpiod_LineBulkObject *bulk;
-       PyObject *arg;
-
-       arg = PyTuple_Pack(1, lines);
-       if (!arg)
+       if (unique && gpiod_line_bulk_num_lines(bulk) > 1) {
+               gpiod_line_bulk_free(bulk);
+               PyErr_SetString(PyExc_RuntimeError, "line not unique");
                return NULL;
+       }
 
-       bulk = (gpiod_LineBulkObject *)PyObject_CallObject(
-                                       (PyObject *)&gpiod_LineBulkType,
-                                       arg);
-       Py_DECREF(arg);
-
-       return bulk;
+       bulk_obj = gpiod_LineBulkObjectFromBulk(self, bulk);
+       gpiod_line_bulk_free(bulk);
+       return bulk_obj;
 }
 
 PyDoc_STRVAR(gpiod_Chip_get_lines_doc,
@@ -2293,10 +2347,6 @@ gpiod_Chip_get_all_lines(gpiod_ChipObject *self, PyObject *Py_UNUSED(ignored))
 {
        gpiod_LineBulkObject *bulk_obj;
        struct gpiod_line_bulk *bulk;
-       gpiod_LineObject *line_obj;
-       unsigned int idx;
-       PyObject *list;
-       int rv;
 
        if (gpiod_ChipIsClosed(self))
                return NULL;
@@ -2306,36 +2356,8 @@ gpiod_Chip_get_all_lines(gpiod_ChipObject *self, PyObject *Py_UNUSED(ignored))
                return (gpiod_LineBulkObject *)PyErr_SetFromErrno(
                                                        PyExc_OSError);
 
-       list = PyList_New(gpiod_line_bulk_num_lines(bulk));
-       if (!list) {
-               gpiod_line_bulk_free(bulk);
-               return NULL;
-       }
-
-       for (idx = 0; idx < gpiod_line_bulk_num_lines(bulk); idx++) {
-               line_obj = gpiod_MakeLineObject(self, gpiod_line_bulk_get_line(bulk, idx));
-               if (!line_obj) {
-                       gpiod_line_bulk_free(bulk);
-                       Py_DECREF(list);
-                       return NULL;
-               }
-
-               rv = PyList_SetItem(list, idx, (PyObject *)line_obj);
-               if (rv < 0) {
-                       gpiod_line_bulk_free(bulk);
-                       Py_DECREF(line_obj);
-                       Py_DECREF(list);
-                       return NULL;
-               }
-       }
-
+       bulk_obj = gpiod_LineBulkObjectFromBulk(self, bulk);
        gpiod_line_bulk_free(bulk);
-
-       bulk_obj = gpiod_ListToLineBulk(list);
-       Py_DECREF(list);
-       if (!bulk_obj)
-               return NULL;
-
        return bulk_obj;
 }
 
@@ -2384,8 +2406,8 @@ static PyMethodDef gpiod_Chip_methods[] = {
        },
        {
                .ml_name = "find_line",
-               .ml_meth = (PyCFunction)gpiod_Chip_find_line,
-               .ml_flags = METH_VARARGS,
+               .ml_meth = (PyCFunction)(void (*)(void))gpiod_Chip_find_line,
+               .ml_flags = METH_VARARGS | METH_KEYWORDS,
                .ml_doc = gpiod_Chip_find_line_doc,
        },
        {
index 6ee72a955ccd2d31566573f124ab64d82001d0ef..c490933597e462870736ddaa52374a9d76851286 100755 (executable)
@@ -149,8 +149,9 @@ class ChipGetLines(MockupTestCase):
 
     def test_find_single_line_by_name(self):
         with gpiod.Chip(mockup.chip_name(1)) as chip:
-            line = chip.find_line('gpio-mockup-B-4')
-            self.assertEqual(line.offset(), 4)
+            lines = chip.find_line('gpio-mockup-B-4').to_list()
+            self.assertEqual(len(lines), 1)
+            self.assertEqual(lines[0].offset(), 4)
 
     def test_get_single_line_invalid_offset(self):
         with gpiod.Chip(mockup.chip_name(1)) as chip:
@@ -161,8 +162,8 @@ class ChipGetLines(MockupTestCase):
 
     def test_find_single_line_nonexistent(self):
         with gpiod.Chip(mockup.chip_name(1)) as chip:
-            line = chip.find_line('nonexistent-line')
-            self.assertEqual(line, None)
+            lines = chip.find_line('nonexistent-line')
+            self.assertEqual(lines, None)
 
     def test_get_multiple_lines_by_offsets_in_tuple(self):
         with gpiod.Chip(mockup.chip_name(1)) as chip:
index 34313ed4386b3b2808fc098abe482e00254af7f3..fc50fe8887ac7a85cbd4ff2ecb1505961bed8a7a 100644 (file)
@@ -170,19 +170,31 @@ struct gpiod_line_bulk *
 gpiod_chip_get_all_lines(struct gpiod_chip *chip) GPIOD_API;
 
 /**
- * @brief Find a GPIO line by name among lines associated with given GPIO chip.
+ * @brief Find all GPIO lines by name among lines exposed by this GPIO chip.
  * @param chip The GPIO chip object.
- * @param name The name of the GPIO line.
+ * @param name GPIO line name to look for.
+ * @return New line bulk object containing all matching lines or NULL on error.
+ *
+ * If no line with given name is associated with this chip, the function sets
+ * errno to ENOENT.
+ */
+struct gpiod_line_bulk *
+gpiod_chip_find_line(struct gpiod_chip *chip, const char *name) GPIOD_API;
+
+/**
+ * @brief Find a unique line by name among lines exposed by this GPIO chip.
+ * @param chip The GPIO chip object.
+ * @param name Name of the GPIO line.
  * @return Pointer to the GPIO line handle or NULL if the line could not be
  *         found or an error occurred.
- * @note In case a line with given name is not associated with given chip, the
- *       function sets errno to ENOENT.
- * @attention GPIO line names are not unique in the linux kernel, neither
- *            globally nor within a single chip. This function finds the first
- *            line with given name.
+ *
+ * If no line with given name is associated with this chip, the function sets
+ * errno to ENOENT. If more than one line with given name is associated with
+ * this chip, the function sets errno to ERANGE.
  */
 struct gpiod_line *
-gpiod_chip_find_line(struct gpiod_chip *chip, const char *name) GPIOD_API;
+gpiod_chip_find_line_unique(struct gpiod_chip *chip,
+                           const char *name) GPIOD_API;
 
 /**
  * @}
index 509a1c8067fdb03a7669f60b50634ef1ead46644..ec2575d2c113435934096490241d8b1338746a15 100644 (file)
@@ -120,11 +120,48 @@ struct gpiod_line_bulk *gpiod_chip_get_all_lines(struct gpiod_chip *chip)
        return bulk;
 }
 
-struct gpiod_line *
+struct gpiod_line_bulk *
 gpiod_chip_find_line(struct gpiod_chip *chip, const char *name)
 {
+       struct gpiod_line_bulk *bulk = NULL;
+       unsigned int offset, num_lines;
        struct gpiod_line *line;
-       unsigned int offset;
+       const char *tmp;
+
+       num_lines = gpiod_chip_num_lines(chip);
+
+       for (offset = 0; offset < num_lines; offset++) {
+               line = gpiod_chip_get_line(chip, offset);
+               if (!line) {
+                       if (bulk)
+                               gpiod_line_bulk_free(bulk);
+
+                       return NULL;
+               }
+
+               tmp = gpiod_line_name(line);
+               if (tmp && strcmp(tmp, name) == 0) {
+                       if (!bulk) {
+                               bulk = gpiod_line_bulk_new(num_lines);
+                               if (!bulk)
+                                       return NULL;
+                       }
+
+                       gpiod_line_bulk_add_line(bulk, line);
+               }
+       }
+
+       if (!bulk)
+               errno = ENOENT;
+
+       return bulk;
+}
+
+struct gpiod_line *
+gpiod_chip_find_line_unique(struct gpiod_chip *chip, const char *name)
+{
+       struct gpiod_line *line, *matching = NULL;
+       unsigned int offset, num_found = 0;
        const char *tmp;
 
        for (offset = 0; offset < gpiod_chip_num_lines(chip); offset++) {
@@ -133,12 +170,22 @@ gpiod_chip_find_line(struct gpiod_chip *chip, const char *name)
                        return NULL;
 
                tmp = gpiod_line_name(line);
-               if (tmp && strcmp(tmp, name) == 0)
-                       return line;
+               if (tmp && strcmp(tmp, name) == 0) {
+                       matching = line;
+                       num_found++;
+               }
        }
 
-       errno = ENOENT;
+       if (matching) {
+               if (num_found > 1) {
+                       errno = ERANGE;
+                       return NULL;
+               }
 
+               return matching;
+       }
+
+       errno = ENOENT;
        return NULL;
 }
 
index 543c10356e1c19d83dbb66ca2ddfbf3adb22af79..1c365b9f883b707cc58d7d5fba0756effd71e534 100644 (file)
@@ -235,7 +235,7 @@ GPIOD_TEST_CASE(get_all_lines, 0, { 4 })
        g_assert_cmpuint(gpiod_line_offset(line3), ==, 3);
 }
 
-GPIOD_TEST_CASE(find_line_good, GPIOD_TEST_FLAG_NAMED_LINES, { 8, 8, 8 })
+GPIOD_TEST_CASE(find_line_good_unique, GPIOD_TEST_FLAG_NAMED_LINES, { 8, 8, 8 })
 {
        g_autoptr(gpiod_chip_struct) chip = NULL;
        struct gpiod_line *line;
@@ -244,14 +244,15 @@ GPIOD_TEST_CASE(find_line_good, GPIOD_TEST_FLAG_NAMED_LINES, { 8, 8, 8 })
        g_assert_nonnull(chip);
        gpiod_test_return_if_failed();
 
-       line = gpiod_chip_find_line(chip, "gpio-mockup-B-4");
+       line = gpiod_chip_find_line_unique(chip, "gpio-mockup-B-4");
        g_assert_nonnull(line);
        gpiod_test_return_if_failed();
        g_assert_cmpuint(gpiod_line_offset(line), ==, 4);
        g_assert_cmpstr(gpiod_line_name(line), ==, "gpio-mockup-B-4");
 }
 
-GPIOD_TEST_CASE(find_line_not_found, GPIOD_TEST_FLAG_NAMED_LINES, { 8, 8, 8 })
+GPIOD_TEST_CASE(find_line_unique_not_found,
+               GPIOD_TEST_FLAG_NAMED_LINES, { 8, 8, 8 })
 {
        g_autoptr(gpiod_chip_struct) chip = NULL;
        struct gpiod_line *line;
@@ -260,7 +261,7 @@ GPIOD_TEST_CASE(find_line_not_found, GPIOD_TEST_FLAG_NAMED_LINES, { 8, 8, 8 })
        g_assert_nonnull(chip);
        gpiod_test_return_if_failed();
 
-       line = gpiod_chip_find_line(chip, "nonexistent");
+       line = gpiod_chip_find_line_unique(chip, "nonexistent");
        g_assert_null(line);
        g_assert_cmpint(errno, ==, ENOENT);
 }
index 489cf339666a9c69bfdd0fb58cb9d248886873fe..ffb8fc055788d78e5c3150ef7416698cbfce0652 100644 (file)
@@ -69,7 +69,7 @@ int main(int argc, char **argv)
                die_perror("unable to access GPIO chips");
 
        gpiod_foreach_chip(iter, chip) {
-               line = gpiod_chip_find_line(chip, argv[0]);
+               line = gpiod_chip_find_line_unique(chip, argv[0]);
                if (line) {
                        printf("%s %u\n",
                               gpiod_chip_name(chip), gpiod_line_offset(line));