[libcamera-devel] [PATCH v5 3/5] ipa: raspberrypi: Add move/copy ctors and operators to Metadata class

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Mon, Apr 19, 2021 at 02:34:49PM +0100, Naushir Patuck wrote:
> Add a default, move and copy constructor as well as a move operator
> implementation RPiController::Metadata class.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/metadata.hpp | 24 +++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp
> index 07dd28ed9e0a..319f2320fc70 100644
> --- a/src/ipa/raspberrypi/controller/metadata.hpp
> +++ b/src/ipa/raspberrypi/controller/metadata.hpp
> @@ -19,6 +19,21 @@ namespace RPiController {
>  class Metadata
>  {
>  public:
> +	Metadata() = default;
> +
> +	Metadata(Metadata const &other)
> +	{
> +		std::lock_guard<std::mutex> other_lock(other.mutex_);
> +		data_ = other.data_;
> +	}
> +
> +	Metadata(Metadata &&other)
> +	{
> +		std::lock_guard<std::mutex> other_lock(other.mutex_);
> +		data_ = std::move(other.data_);
> +		other.data_.clear();
> +	}
> +
>  	template<typename T>
>  	void Set(std::string const &tag, T const &value)
>  	{
> @@ -51,6 +66,15 @@ public:
>  		return *this;
>  	}
>  
> +	Metadata &operator=(Metadata &&other)
> +	{
> +		std::lock_guard<std::mutex> lock(mutex_);
> +		std::lock_guard<std::mutex> other_lock(other.mutex_);

This could cause a deadlock if two threads we to move A to B and B to A.
I suppose such a usage pattern never happens ? We may be able to improve
locking a bit, but that's out of scope for this series.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		data_ = std::move(other.data_);
> +		other.data_.clear();
> +		return *this;
> +	}
> +
>  	template<typename T>
>  	T *GetLocked(std::string const &tag)
>  	{

-- 
Regards,

Laurent Pinchart