[libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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>
---
Hi Helmut!

Please take a look at the following proposition of modifying the
relationship between chips and lines in C++ bindings. This is a follow-up
to our discussion under your suggestion for using time_point for
timestamps.

Bartosz

 bindings/cxx/chip.cpp                 |  6 ++++
 bindings/cxx/examples/gpiofindcxx.cpp |  6 ++--
 bindings/cxx/gpiod.hpp                | 31 +++++++++++++++++----
 bindings/cxx/iter.cpp                 |  2 +-
 bindings/cxx/line.cpp                 | 40 +++++++++++++++++++++------
 bindings/cxx/line_bulk.cpp            |  9 ++++++
 bindings/cxx/tests/tests-line.cpp     | 12 ++++----
 7 files changed, 81 insertions(+), 25 deletions(-)

diff --git a/bindings/cxx/chip.cpp b/bindings/cxx/chip.cpp
index 54c85da..1fc0723 100644
--- 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 08fb62c..aeba29d 100644
--- 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 8dfc172..50cdf7f 100644
--- 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_locker
+	{
+	public:
+		chip_locker(const line& line);
+		~chip_locker(void) = default;
+
+		chip_locker(const chip_locker& other) = delete;
+		chip_locker(chip_locker&& other) = delete;
+		chip_locker& operator=(const chip_locker&& other) = delete;
+		chip_locker& operator=(chip_locker&& 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 b1acc8c..7985910 100644
--- 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 52084bf..1ee4459 100644
--- 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker::chip_locker(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 e77baa2..78615a1 100644
--- 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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_locker 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 e2e4cbc..9841bea 100644
--- 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);
 	}
 }
 
-- 
2.28.0

Re: [libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Helmut Grohne]

On Fri, Oct 16, 2020 at 11:09:49AM +0200, Bartosz Golaszewski wrote:
> 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.

I don't think I understand the implications for this yet. Something in
my mental model must be wrong or the change doesn't make much sense.

> +chip::chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr)
> +	: _m_chip(chip_ptr)
> +{
> +
> +}
> +

I think what happens here is that you upgrade a weak_ptr to a
shared_ptr. Wouldn't it be more natural to request a

    ::std::shared_ptr<::gpiod_chip> &&

here and thus make the ownership-taking more explicit? It would be done
on the caller-side and thus be more transparent. Stuffing weak_ptrs
should continue to work.

> @@ -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_locker
> +	{
> +	public:
> +		chip_locker(const line& line);
> +		~chip_locker(void) = default;
> +
> +		chip_locker(const chip_locker& other) = delete;
> +		chip_locker(chip_locker&& other) = delete;
> +		chip_locker& operator=(const chip_locker&& other) = delete;
> +		chip_locker& operator=(chip_locker&& other) = delete;
> +
> +	private:
> +		::std::shared_ptr<::gpiod_chip> _m_chip;
> +	};
>  
>  	friend chip;
>  	friend line_bulk;

I don't quite get what problem this chip_lcoker solves. It seems to
prevent concurrent destruction of a ::gpiod_chip. How can this happen?
One option would be concurrency due to threads. However the whole API
does not look thread-safe so this would be wrong. The other could be
callbacks. I couldn't find any callbacks in the C++ bindings. So now I
am confused why one would need to lock the chip. Do you fear changes
inside signal handlers?

> @@ -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);

This makes the API a little less convenient to use.

> @@ -39,6 +39,7 @@ line::line(::gpiod_line* line, const chip& owner)
>  unsigned int line::offset(void) const
>  {
>  	this->throw_if_null();
> +	line::chip_locker lock_chip(*this);
>  
>  	return ::gpiod_line_offset(this->_m_line);
>  }

This example nicely shows why I am confused about the chip_locker. Why
can the chip not become null between the throw_if_null call and the
chip_locker construction, but it can be externally mutated between the
chip_locker construction and the gpiod_line_offset call? It would appear
to me that the chip_locker is entirely unnecessary here.

I also think that this refactoring still does not accurately represent
the kernel interfaces. When you dispose a chip, the owned line becomes
invalidated. Why do I have to keep the chip to use the line? Is it
really reasonable to have chips own lines?

When I use the bare kernel interfaces, I can open a chip, request a line
(which receives a separate fd) and then I can close the chip and
continue working with the line fd. I could even open the chip in one
process and transfer the open line fd to a different (possibly
sandboxed) process or close the chip and sandbox the process prohibiting
further VFS operations. As far as I can see, neither the old nor the
proposed API handles any of this.

Helmut

Re: [libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Bartosz Golaszewski]

On Fri, Oct 16, 2020 at 12:29 PM Helmut Grohne <helmut.grohne@intenta.de> wrote:
>

[snip]

> > +chip::chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr)
> > +     : _m_chip(chip_ptr)
> > +{
> > +
> > +}
> > +
>
> I think what happens here is that you upgrade a weak_ptr to a
> shared_ptr. Wouldn't it be more natural to request a
>
>     ::std::shared_ptr<::gpiod_chip> &&
>
> here and thus make the ownership-taking more explicit? It would be done
> on the caller-side and thus be more transparent. Stuffing weak_ptrs
> should continue to work.
>

Sure, sounds good.

> > @@ -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_locker
> > +     {
> > +     public:
> > +             chip_locker(const line& line);
> > +             ~chip_locker(void) = default;
> > +
> > +             chip_locker(const chip_locker& other) = delete;
> > +             chip_locker(chip_locker&& other) = delete;
> > +             chip_locker& operator=(const chip_locker&& other) = delete;
> > +             chip_locker& operator=(chip_locker&& other) = delete;
> > +
> > +     private:
> > +             ::std::shared_ptr<::gpiod_chip> _m_chip;
> > +     };
> >
> >       friend chip;
> >       friend line_bulk;
>
> I don't quite get what problem this chip_lcoker solves. It seems to
> prevent concurrent destruction of a ::gpiod_chip. How can this happen?
> One option would be concurrency due to threads. However the whole API
> does not look thread-safe so this would be wrong. The other could be
> callbacks. I couldn't find any callbacks in the C++ bindings. So now I
> am confused why one would need to lock the chip. Do you fear changes
> inside signal handlers?
>

When designing the core library written in C, I mostly followed the
concepts behind libabc[1] including this one:

    Zero global state -- Make your library threads-aware, but *not* thread-safe!

C++ bindings are not thread-safe but in general you don't need to
protect the entire lines+chip set from concurrent access. It's enough
to protect each separate line (or sets of lines requested together)
because the only moment where the line accesses chip's data is reading
its file descriptor number to update its line information by calling
the line info ioctl(). And since ioctl() is thread-safe for user-space
according to POSIX[2] the only danger is the chip being concurrently
destroyed. While in C we deal with pointers and implicitly expect
users to be aware of this, in C++ I figured it's better to ensure that
the chip object stays alive until the line operation completes or at
the very least throws an exception rather than crash the entire
program. This was the reason for the locker.

> > @@ -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);
>
> This makes the API a little less convenient to use.
>
> > @@ -39,6 +39,7 @@ line::line(::gpiod_line* line, const chip& owner)
> >  unsigned int line::offset(void) const
> >  {
> >       this->throw_if_null();
> > +     line::chip_locker lock_chip(*this);
> >
> >       return ::gpiod_line_offset(this->_m_line);
> >  }
>
> This example nicely shows why I am confused about the chip_locker. Why
> can the chip not become null between the throw_if_null call and the
> chip_locker construction, but it can be externally mutated between the
> chip_locker construction and the gpiod_line_offset call? It would appear
> to me that the chip_locker is entirely unnecessary here.
>

line::throw_if_null() doesn't touch the chip yet - all it does is
verify that line::offset() is not called on an empty object - hence
the logic_error it throws. If the chip is destroyed between these two
calls - we simply get the bad_weak_ptr exception.

> I also think that this refactoring still does not accurately represent
> the kernel interfaces. When you dispose a chip, the owned line becomes
> invalidated. Why do I have to keep the chip to use the line? Is it
> really reasonable to have chips own lines?
>
> When I use the bare kernel interfaces, I can open a chip, request a line
> (which receives a separate fd) and then I can close the chip and
> continue working with the line fd. I could even open the chip in one
> process and transfer the open line fd to a different (possibly
> sandboxed) process or close the chip and sandbox the process prohibiting
> further VFS operations. As far as I can see, neither the old nor the
> proposed API handles any of this.
>

Indeed and there's a reason for this.

The goal of this library isn't to map the kernel interface 1-to-1 but
to provide a simpler API that can be used without the knowledge of the
underlying ioctl()s and file descriptors and which clearly represents
the hierarchy of chips and lines. In libgpiod - unlike the kernel - a
line is represented as its own object even before it's requested or
after it's been released. This logical object keeps some state of the
line and information about it all the time - not only when the file
descriptor for the requested line exists.

I don't really see any benefit of trying to entirely detach lines from chips.

The kernel behavior you described isn't even something done on purpose
- it's just the result of how the kernel handles file descriptors
after all.

> Helmut

Bartosz

[1] https://git.kernel.org/pub/scm/linux/kernel/git/kay/libabc.git
[2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_01

Re: [libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Bartosz Golaszewski]

On Fri, Oct 16, 2020 at 3:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Oct 16, 2020 at 12:29 PM Helmut Grohne <helmut.grohne@intenta.de> wrote:
> >
>
> [snip]
>
> > > +chip::chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr)
> > > +     : _m_chip(chip_ptr)
> > > +{
> > > +
> > > +}
> > > +
> >
> > I think what happens here is that you upgrade a weak_ptr to a
> > shared_ptr. Wouldn't it be more natural to request a
> >
> >     ::std::shared_ptr<::gpiod_chip> &&
> >
> > here and thus make the ownership-taking more explicit? It would be done
> > on the caller-side and thus be more transparent. Stuffing weak_ptrs
> > should continue to work.
> >
>
> Sure, sounds good.
>

After a second look - I'm not sure if this is actually better. By
taking weak_ptr reference as argument we benefit from implicit
conversion to shared_ptr via shared_ptr's constructor taking weak_ptr
as argument. What you propose would require us to always instantiate a
shared_ptr in the argument list when calling the chip's constructor
and makes code uglier in the end IMO.

Bartosz

Re: [libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Helmut Grohne]

On Mon, Oct 19, 2020 at 02:17:49PM +0200, Bartosz Golaszewski wrote:
> On Fri, Oct 16, 2020 at 3:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Fri, Oct 16, 2020 at 12:29 PM Helmut Grohne <helmut.grohne@intenta.de> wrote:
> > >
> >
> > [snip]
> >
> > > > +chip::chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr)
> > > > +     : _m_chip(chip_ptr)
> > > > +{
> > > > +
> > > > +}
> > > > +
> > >
> > > I think what happens here is that you upgrade a weak_ptr to a
> > > shared_ptr. Wouldn't it be more natural to request a
> > >
> > >     ::std::shared_ptr<::gpiod_chip> &&
> > >
> > > here and thus make the ownership-taking more explicit? It would be done
> > > on the caller-side and thus be more transparent. Stuffing weak_ptrs
> > > should continue to work.
> > >
> >
> > Sure, sounds good.
> >
> 
> After a second look - I'm not sure if this is actually better. By
> taking weak_ptr reference as argument we benefit from implicit
> conversion to shared_ptr via shared_ptr's constructor taking weak_ptr
> as argument. What you propose would require us to always instantiate a
> shared_ptr in the argument list when calling the chip's constructor
> and makes code uglier in the end IMO.

On a second look, the use of an rvalue reference is suboptimal indeed.
The idea behind my change was this: Since chip stores a shared_ptr, it
can as well consume one. Instead of what I proposed, it should simply
take it by value (not rvalue):

    ::std::shared_ptr<::gpiod_chip>

An existing shared_ptr can be moved into the constructor and then moved
into the member variable. Doing so allows passing a shared_ptr around
without touching reference counts (which are prone to cache line
bouncing). When passing it by value, the implicit conversion from
weak_ptr should work again. Thus the caller would increase the reference
count and the chip would merely gain ownership of the shared_ptr and
move it around.

For reference, see Scott Myers' Effective Modern C++ "Item 41: Consider
pass by value for copyable parameters that are cheap to move and always
copied."

So yeah, it doesn't work the way I wrote initially, because I added the
rvalue reference.

Helmut

Re: [libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Bartosz Golaszewski]

On Mon, Oct 19, 2020 at 2:38 PM Helmut Grohne <helmut.grohne@intenta.de> wrote:
>
> On Mon, Oct 19, 2020 at 02:17:49PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Oct 16, 2020 at 3:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Fri, Oct 16, 2020 at 12:29 PM Helmut Grohne <helmut.grohne@intenta.de> wrote:
> > > >
> > >
> > > [snip]
> > >
> > > > > +chip::chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr)
> > > > > +     : _m_chip(chip_ptr)
> > > > > +{
> > > > > +
> > > > > +}
> > > > > +
> > > >
> > > > I think what happens here is that you upgrade a weak_ptr to a
> > > > shared_ptr. Wouldn't it be more natural to request a
> > > >
> > > >     ::std::shared_ptr<::gpiod_chip> &&
> > > >
> > > > here and thus make the ownership-taking more explicit? It would be done
> > > > on the caller-side and thus be more transparent. Stuffing weak_ptrs
> > > > should continue to work.
> > > >
> > >
> > > Sure, sounds good.
> > >
> >
> > After a second look - I'm not sure if this is actually better. By
> > taking weak_ptr reference as argument we benefit from implicit
> > conversion to shared_ptr via shared_ptr's constructor taking weak_ptr
> > as argument. What you propose would require us to always instantiate a
> > shared_ptr in the argument list when calling the chip's constructor
> > and makes code uglier in the end IMO.
>
> On a second look, the use of an rvalue reference is suboptimal indeed.
> The idea behind my change was this: Since chip stores a shared_ptr, it
> can as well consume one. Instead of what I proposed, it should simply
> take it by value (not rvalue):
>
>     ::std::shared_ptr<::gpiod_chip>
>
> An existing shared_ptr can be moved into the constructor and then moved
> into the member variable. Doing so allows passing a shared_ptr around
> without touching reference counts (which are prone to cache line
> bouncing). When passing it by value, the implicit conversion from
> weak_ptr should work again. Thus the caller would increase the reference
> count and the chip would merely gain ownership of the shared_ptr and
> move it around.
>
> For reference, see Scott Myers' Effective Modern C++ "Item 41: Consider
> pass by value for copyable parameters that are cheap to move and always
> copied."
>
> So yeah, it doesn't work the way I wrote initially, because I added the
> rvalue reference.
>
> Helmut

But this still forces us to do

    return chip(::std::shared_ptr<::gpiod_chip>(this->_m_owner));

instead of a much more elegant

    return chip(this->_m_owner);

in line.cpp and there's an even less elegant thing in iter.cpp. Or am
I missing something?

Bartosz

Re: [libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Helmut Grohne]

On Mon, Oct 19, 2020 at 03:06:18PM +0200, Bartosz Golaszewski wrote:
> But this still forces us to do
> 
>     return chip(::std::shared_ptr<::gpiod_chip>(this->_m_owner));
> 
> instead of a much more elegant
> 
>     return chip(this->_m_owner);
> 
> in line.cpp and there's an even less elegant thing in iter.cpp. Or am
> I missing something?

I confirm the behaviour you see. My intuition that the conversion would
happen implicitly was wrong.

Still the sticking point is this: Your constructor should allow for most
flexibility to the caller and in this case that means it should consume
a shared_ptr by value.

In order to make the case with a weak_ptr bearable, I suggest adding a
delegating constructor:

    chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr) :
        chip(::std::shared_ptr<::gpiod_chip>(chip_ptr)) {}

That way your desired way of calling should continue to work while not
forcing callers to convert a real shared_ptr to weak_ptr and back.

Sorry for the confusion about this.

Helmut

Re: [libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Bartosz Golaszewski]

On Tue, Oct 20, 2020 at 7:57 AM Helmut Grohne <helmut.grohne@intenta.de> wrote:
>
> On Mon, Oct 19, 2020 at 03:06:18PM +0200, Bartosz Golaszewski wrote:
> > But this still forces us to do
> >
> >     return chip(::std::shared_ptr<::gpiod_chip>(this->_m_owner));
> >
> > instead of a much more elegant
> >
> >     return chip(this->_m_owner);
> >
> > in line.cpp and there's an even less elegant thing in iter.cpp. Or am
> > I missing something?
>
> I confirm the behaviour you see. My intuition that the conversion would
> happen implicitly was wrong.
>
> Still the sticking point is this: Your constructor should allow for most
> flexibility to the caller and in this case that means it should consume
> a shared_ptr by value.
>
> In order to make the case with a weak_ptr bearable, I suggest adding a
> delegating constructor:
>
>     chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr) :
>         chip(::std::shared_ptr<::gpiod_chip>(chip_ptr)) {}
>
> That way your desired way of calling should continue to work while not
> forcing callers to convert a real shared_ptr to weak_ptr and back.
>
> Sorry for the confusion about this.

This is a private constructor though and it's not meant to be exposed
to callers of library interfaces. Only internal users will call it and
they'll pass a weak_ptr to it alright. I really think it's fine the
way it is now.

Bartosz

Re: [libgpiod][RFC PATCH] bindings: cxx: demote the line's parent chip reference to a weak_ptr

[Bartosz Golaszewski]

On Tue, Oct 20, 2020 at 8:59 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Oct 20, 2020 at 7:57 AM Helmut Grohne <helmut.grohne@intenta.de> wrote:
> >
> > On Mon, Oct 19, 2020 at 03:06:18PM +0200, Bartosz Golaszewski wrote:
> > > But this still forces us to do
> > >
> > >     return chip(::std::shared_ptr<::gpiod_chip>(this->_m_owner));
> > >
> > > instead of a much more elegant
> > >
> > >     return chip(this->_m_owner);
> > >
> > > in line.cpp and there's an even less elegant thing in iter.cpp. Or am
> > > I missing something?
> >
> > I confirm the behaviour you see. My intuition that the conversion would
> > happen implicitly was wrong.
> >
> > Still the sticking point is this: Your constructor should allow for most
> > flexibility to the caller and in this case that means it should consume
> > a shared_ptr by value.
> >
> > In order to make the case with a weak_ptr bearable, I suggest adding a
> > delegating constructor:
> >
> >     chip(const ::std::weak_ptr<::gpiod_chip>& chip_ptr) :
> >         chip(::std::shared_ptr<::gpiod_chip>(chip_ptr)) {}
> >
> > That way your desired way of calling should continue to work while not
> > forcing callers to convert a real shared_ptr to weak_ptr and back.
> >
> > Sorry for the confusion about this.
>
> This is a private constructor though and it's not meant to be exposed
> to callers of library interfaces. Only internal users will call it and
> they'll pass a weak_ptr to it alright. I really think it's fine the
> way it is now.
>
> Bartosz

I applied this for now in its current form as I want to start working
on the PImpl changes. We can tweak it later.

Bartosz