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

[Helmut Grohne <helmut.grohne@xxxxxxxxxx>]

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