bindings: cxx: demote the line's parent chip reference to a weak_ptr

Currently the line object has the power to control the parent chip's
lifetime because it stores a hard reference (in the form of a shared
pointer) to the underlying struct gpiod_chip. This is wrong from the
relationship point-of-view - the chip should control the exposed lines,
not the other way around.

Demote the parent reference to a weak_ptr. Introduce a sub-class that
allows line and line_bulk objects to lock the chip by temporarily
converting this weak_ptr to a full shared_ptr - this way we don't risk
dropping the last reference to the parent chip when the line is calling
the underlying library functions. Chip deleted during that time will
expire right after the concerned line method returns.

This requires an API change - the global find_line() function now
returns a <line, chip> pair so that the caller gets the chance to grab
the chip's reference.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

diff --git a/bindings/cxx/chip.cpp b/bindings/cxx/chip.cpp
index 54c85da2a2243eab166fcaf32565f660b5faa006..1fc072381c36b59760c47cb2286e4e8a247d6c42 100644 (file)
--- a/bindings/cxx/chip.cpp
+++ b/bindings/cxx/chip.cpp
@@ -69,6 +69,12 @@ chip::chip(::gpiod_chip* chip)
 
 }
 
+chip::chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr)
+       : _m_chip(chip_ptr)
+{
+
+}
+
 void chip::open(const ::std::string& device, int how)
 {
        auto func = open_funcs.at(how);

diff --git a/bindings/cxx/examples/gpiofindcxx.cpp b/bindings/cxx/examples/gpiofindcxx.cpp
index 08fb62cc68941f270a9af6a04e08bc016dbd03ba..aeba29df11121d4ad4f51b70e74bc851cc4f5e99 100644 (file)
--- a/bindings/cxx/examples/gpiofindcxx.cpp
+++ b/bindings/cxx/examples/gpiofindcxx.cpp
@@ -19,11 +19,11 @@ int main(int argc, char **argv)
                return EXIT_FAILURE;
        }
 
-       ::gpiod::line line = ::gpiod::find_line(argv[1]);
-       if (!line)
+       auto ret = ::gpiod::find_line(argv[1]);
+       if (!ret.first)
                return EXIT_FAILURE;
 
-       ::std::cout << line.get_chip().name() << " " << line.offset() << ::std::endl;
+       ::std::cout << ret.second.name() << " " << ret.first.offset() << ::std::endl;
 
        return EXIT_SUCCESS;
 }

diff --git a/bindings/cxx/gpiod.hpp b/bindings/cxx/gpiod.hpp
index 8dfc172485d47091270548728188802c4a35e4ec..8594f5e8c522fbebb1c96fca8afbfdbaf3337143 100644 (file)
--- a/bindings/cxx/gpiod.hpp
+++ b/bindings/cxx/gpiod.hpp
@@ -199,11 +199,13 @@ public:
 private:
 
        chip(::gpiod_chip* chip);
+       chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr);
 
        void throw_if_noref(void) const;
 
        ::std::shared_ptr<::gpiod_chip> _m_chip;
 
+       friend line;
        friend chip_iter;
        friend line_iter;
 };
@@ -438,10 +440,10 @@ public:
        GPIOD_API int event_get_fd(void) const;
 
        /**
-        * @brief Get the reference to the parent chip.
-        * @return Reference to the parent chip object.
+        * @brief Get the parent chip.
+        * @return Parent chip of this line.
         */
-       GPIOD_API const chip& get_chip(void) const;
+       GPIOD_API const chip get_chip(void) const;
 
        /**
         * @brief Re-read the line info from the kernel.
@@ -526,7 +528,22 @@ private:
        line_event make_line_event(const ::gpiod_line_event& event) const noexcept;
 
        ::gpiod_line* _m_line;
-       chip _m_chip;
+       ::std::weak_ptr<::gpiod_chip> _m_owner;
+
+       class chip_guard
+       {
+       public:
+               chip_guard(const line& line);
+               ~chip_guard(void) = default;
+
+               chip_guard(const chip_guard& other) = delete;
+               chip_guard(chip_guard&& other) = delete;
+               chip_guard& operator=(const chip_guard&& other) = delete;
+               chip_guard& operator=(chip_guard&& other) = delete;
+
+       private:
+               ::std::shared_ptr<::gpiod_chip> _m_chip;
+       };
 
        friend chip;
        friend line_bulk;
@@ -536,9 +553,11 @@ private:
 /**
  * @brief Find a GPIO line by name. Search all GPIO chips present on the system.
  * @param name Name of the line.
- * @return Returns a line object - empty if the line was not found.
+ * @return Returns a <line, chip> pair where line is the line with given name
+ *         and chip is the line's owner. Both objects are empty if the line was
+ *         not found.
  */
-GPIOD_API line find_line(const ::std::string& name);
+GPIOD_API ::std::pair<line, chip> find_line(const ::std::string& name);
 
 /**
  * @brief Describes a single GPIO line event.

diff --git a/bindings/cxx/iter.cpp b/bindings/cxx/iter.cpp
index b1acc8c9cef4ab8c623b52ceb5408762901dbdb6..79859108104fb443be2f13e95f73e0ba8e6c5f27 100644 (file)
--- a/bindings/cxx/iter.cpp
+++ b/bindings/cxx/iter.cpp
@@ -115,7 +115,7 @@ line_iter& line_iter::operator++(void)
 {
        ::gpiod_line* next = ::gpiod_line_iter_next(this->_m_iter.get());
 
-       this->_m_current = next ? line(next, this->_m_current._m_chip) : line();
+       this->_m_current = next ? line(next, this->_m_current._m_owner) : line();
 
        return *this;
 }

diff --git a/bindings/cxx/line.cpp b/bindings/cxx/line.cpp
index 52084bfc30636f3ed960fa0837327d963581655c..5589875bfcd3849bc33c87813977dbd38a583961 100644 (file)
--- a/bindings/cxx/line.cpp
+++ b/bindings/cxx/line.cpp
@@ -24,14 +24,14 @@ const ::std::map<int, int> bias_mapping = {
 
 line::line(void)
        : _m_line(nullptr),
-         _m_chip()
+         _m_owner()
 {
 
 }
 
 line::line(::gpiod_line* line, const chip& owner)
        : _m_line(line),
-         _m_chip(owner)
+         _m_owner(owner._m_chip)
 {
 
 }
@@ -39,6 +39,7 @@ line::line(::gpiod_line* line, const chip& owner)
 unsigned int line::offset(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        return ::gpiod_line_offset(this->_m_line);
 }
@@ -46,6 +47,7 @@ unsigned int line::offset(void) const
 ::std::string line::name(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        const char* name = ::gpiod_line_name(this->_m_line);
 
@@ -55,6 +57,7 @@ unsigned int line::offset(void) const
 ::std::string line::consumer(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        const char* consumer = ::gpiod_line_consumer(this->_m_line);
 
@@ -64,6 +67,7 @@ unsigned int line::offset(void) const
 int line::direction(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        int dir = ::gpiod_line_direction(this->_m_line);
 
@@ -73,6 +77,7 @@ int line::direction(void) const
 int line::active_state(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        int active = ::gpiod_line_active_state(this->_m_line);
 
@@ -82,6 +87,7 @@ int line::active_state(void) const
 int line::bias(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        return bias_mapping.at(::gpiod_line_bias(this->_m_line));
 }
@@ -89,6 +95,7 @@ int line::bias(void) const
 bool line::is_used(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        return ::gpiod_line_is_used(this->_m_line);
 }
@@ -96,6 +103,7 @@ bool line::is_used(void) const
 bool line::is_open_drain(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        return ::gpiod_line_is_open_drain(this->_m_line);
 }
@@ -103,6 +111,7 @@ bool line::is_open_drain(void) const
 bool line::is_open_source(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        return ::gpiod_line_is_open_source(this->_m_line);
 }
@@ -128,6 +137,7 @@ void line::release(void) const
 bool line::is_requested(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        return ::gpiod_line_is_requested(this->_m_line);
 }
@@ -227,6 +237,7 @@ line_event line::make_line_event(const ::gpiod_line_event& event) const noexcept
 line_event line::event_read(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        ::gpiod_line_event event_buf;
        line_event event;
@@ -243,6 +254,7 @@ line_event line::event_read(void) const
 ::std::vector<line_event> line::event_read_multiple(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        /* 16 is the maximum number of events stored in the kernel FIFO. */
        ::std::array<::gpiod_line_event, 16> event_buf;
@@ -265,6 +277,7 @@ line_event line::event_read(void) const
 int line::event_get_fd(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        int ret = ::gpiod_line_event_get_fd(this->_m_line);
 
@@ -275,14 +288,15 @@ int line::event_get_fd(void) const
        return ret;
 }
 
-const chip& line::get_chip(void) const
+const chip line::get_chip(void) const
 {
-       return this->_m_chip;
+       return chip(this->_m_owner);
 }
 
 void line::update(void) const
 {
        this->throw_if_null();
+       line::chip_guard lock_chip(*this);
 
        int ret = ::gpiod_line_update(this->_m_line);
 
@@ -294,7 +308,7 @@ void line::update(void) const
 void line::reset(void)
 {
        this->_m_line = nullptr;
-       this->_m_chip.reset();
+       this->_m_owner.reset();
 }
 
 bool line::operator==(const line& rhs) const noexcept
@@ -323,14 +337,22 @@ void line::throw_if_null(void) const
                throw ::std::logic_error("object not holding a GPIO line handle");
 }
 
-line find_line(const ::std::string& name)
+line::chip_guard::chip_guard(const line& line)
+       : _m_chip(line._m_owner)
 {
-       line ret;
+       
+}
+
+::std::pair<line, chip> find_line(const ::std::string& name)
+{
+       ::std::pair<line, chip> ret;
 
        for (auto& it: make_chip_iter()) {
-               ret = it.find_line(name);
-               if (ret)
+               ret.first = it.find_line(name);
+               if (ret.first) {
+                       ret.second = it;
                        break;
+               }
        }
 
        return ret;

diff --git a/bindings/cxx/line_bulk.cpp b/bindings/cxx/line_bulk.cpp
index e77baa286cc621cbcea66be292b99ea7d6f6dbfc..75381bddd4d0282f9acdbeca1bac8844366e91c1 100644 (file)
--- a/bindings/cxx/line_bulk.cpp
+++ b/bindings/cxx/line_bulk.cpp
@@ -101,6 +101,7 @@ void line_bulk::clear(void)
 void line_bulk::request(const line_request& config, const ::std::vector<int> default_vals) const
 {
        this->throw_if_empty();
+       line::chip_guard lock_chip(this->_m_bulk.front());
 
        if (!default_vals.empty() && this->size() != default_vals.size())
                throw ::std::invalid_argument("the number of default values must correspond with the number of lines");
@@ -131,6 +132,7 @@ void line_bulk::request(const line_request& config, const ::std::vector<int> def
 void line_bulk::release(void) const
 {
        this->throw_if_empty();
+       line::chip_guard lock_chip(this->_m_bulk.front());
 
        ::gpiod_line_bulk bulk;
 
@@ -142,6 +144,7 @@ void line_bulk::release(void) const
 ::std::vector<int> line_bulk::get_values(void) const
 {
        this->throw_if_empty();
+       line::chip_guard lock_chip(this->_m_bulk.front());
 
        ::std::vector<int> values;
        ::gpiod_line_bulk bulk;
@@ -161,6 +164,7 @@ void line_bulk::release(void) const
 void line_bulk::set_values(const ::std::vector<int>& values) const
 {
        this->throw_if_empty();
+       line::chip_guard lock_chip(this->_m_bulk.front());
 
        if (values.size() != this->_m_bulk.size())
                throw ::std::invalid_argument("the size of values array must correspond with the number of lines");
@@ -180,6 +184,7 @@ void line_bulk::set_config(int direction, ::std::bitset<32> flags,
                           const ::std::vector<int> values) const
 {
        this->throw_if_empty();
+       line::chip_guard lock_chip(this->_m_bulk.front());
 
        if (!values.empty() && this->_m_bulk.size() != values.size())
                throw ::std::invalid_argument("the number of default values must correspond with the number of lines");
@@ -206,6 +211,7 @@ void line_bulk::set_config(int direction, ::std::bitset<32> flags,
 void line_bulk::set_flags(::std::bitset<32> flags) const
 {
        this->throw_if_empty();
+       line::chip_guard lock_chip(this->_m_bulk.front());
 
        ::gpiod_line_bulk bulk;
        int rv, gflags;
@@ -228,6 +234,7 @@ void line_bulk::set_flags(::std::bitset<32> flags) const
 void line_bulk::set_direction_input() const
 {
        this->throw_if_empty();
+       line::chip_guard lock_chip(this->_m_bulk.front());
 
        ::gpiod_line_bulk bulk;
        int rv;
@@ -243,6 +250,7 @@ void line_bulk::set_direction_input() const
 void line_bulk::set_direction_output(const ::std::vector<int>& values) const
 {
        this->throw_if_empty();
+       line::chip_guard lock_chip(this->_m_bulk.front());
 
        if (values.size() != this->_m_bulk.size())
                throw ::std::invalid_argument("the size of values array must correspond with the number of lines");
@@ -262,6 +270,7 @@ void line_bulk::set_direction_output(const ::std::vector<int>& values) const
 line_bulk line_bulk::event_wait(const ::std::chrono::nanoseconds& timeout) const
 {
        this->throw_if_empty();
+       line::chip_guard lock_chip(this->_m_bulk.front());
 
        ::gpiod_line_bulk bulk, event_bulk;
        ::timespec ts;

diff --git a/bindings/cxx/tests/tests-line.cpp b/bindings/cxx/tests/tests-line.cpp
index e2e4cbca6271db8019a7b4e27856491c95d776b3..9841bea684274f43611401c51389197f94e8d82b 100644 (file)
--- a/bindings/cxx/tests/tests-line.cpp
+++ b/bindings/cxx/tests/tests-line.cpp
@@ -24,16 +24,16 @@ TEST_CASE("Global find_line() function works", "[line]")
 
        SECTION("line found")
        {
-               auto line = ::gpiod::find_line("gpio-mockup-C-5");
-               REQUIRE(line.offset() == 5);
-               REQUIRE(line.name() == "gpio-mockup-C-5");
-               REQUIRE(line.get_chip().label() == "gpio-mockup-C");
+               auto ret = ::gpiod::find_line("gpio-mockup-C-5");
+               REQUIRE(ret.first.offset() == 5);
+               REQUIRE(ret.first.name() == "gpio-mockup-C-5");
+               REQUIRE(ret.second.label() == "gpio-mockup-C");
        }
 
        SECTION("line not found")
        {
-               auto line = ::gpiod::find_line("nonexistent-line");
-               REQUIRE_FALSE(line);
+               auto ret = ::gpiod::find_line("nonexistent-line");
+               REQUIRE_FALSE(ret.first);
        }
 }