[PATCH 1/2] media_device: Add bool return type to unlock()

[Laurent Pinchart]

Hello Khem,

Thank you for the patch.

On Wed, Jan 31, 2024 at 09:08:09PM -0800, Khem Raj wrote:
> unlock uses lockf which is marked with __attribute__

s/unlock/MediaDevice::unlock()/

> ((warn_unused_result)) and compilers warn about it and some treat
> -Wunused-result as error with -Werror turned on, It would be good to
> check if lockf failed or succeeded, however, that piece is not changed
> with this, this fixes build with clang++ 18

I wouldn't mention clang in the commit message, it's not a
compiler-specific issue, all compilers will warn about the unused
result. Where does that unused result attribute come from though ? I
don't see it in glibc :
https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fcntl.h;h=9cee0b5900cb4f7c3bd94344e17824c843815e3d;hb=HEAD#l274

> 
>     ../git/src/libcamera/media_device.cpp:167:2: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
>       167 |         lockf(fd_.get(), F_ULOCK, 0);
>           |         ^~~~~ ~~~~~~~~~~~~~~~~~~~~~
>     1 error generated.
> 
> Signed-off-by: Khem Raj <raj.khem at gmail.com>
> ---
>  include/libcamera/internal/media_device.h | 2 +-
>  src/libcamera/media_device.cpp            | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h
> index eb8cfde4..b09dfd16 100644
> --- a/include/libcamera/internal/media_device.h
> +++ b/include/libcamera/internal/media_device.h
> @@ -33,7 +33,7 @@ public:
>  	bool busy() const { return acquired_; }
>  
>  	bool lock();
> -	void unlock();
> +	bool unlock();
>  
>  	int populate();
>  	bool isValid() const { return valid_; }
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 2949816b..eaa2fdb0 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -159,12 +159,12 @@ bool MediaDevice::lock()
>   *
>   * \sa lock()
>   */
> -void MediaDevice::unlock()
> +bool MediaDevice::unlock()
>  {
>  	if (!fd_.isValid())
> -		return;
> +		return false;
>  
> -	lockf(fd_.get(), F_ULOCK, 0);
> +	return lockf(fd_.get(), F_ULOCK, 0) == 0;

If you don't check the return value in the callers this is just a hack
to silence the warning, in which case you may as well silence it here
without changing the return value.

>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart