[libcamera-devel] [PATCH 1/2] libcamera: utils: Wrap usage of lockf()

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Dec 02, 2022 at 05:41:44PM +0100, Jacopo Mondi via libcamera-devel wrote:
> The lockf() function is not implemented in the Bionic standard C
> library.
> 
> As an alternative, since Linux v2.0 the flock() function is available
> as a direct system call instead of being emulated in the C library.
> 
> As lockf() is instead usually implemented as an interface to fcntl()
> locking, locks placed by flock() and lockf() might not be detected.
> 
> As reported by the flock() function documentation:
> 
> Since kernel 2.0, flock() is implemented as a system call in its own right
> rather than being emulated in the GNU C library as a call to fcntl(2).
> With this implementation, there is no interaction between the types of
> lock placed by  flock()  and  fc‐ ntl(2), and flock() does not detect
> deadlock.  (Note, however, that on some systems, such as the modern
> BSDs, flock() and fc‐ ntl(2) locks do interact with one another.)

Wrong text rewrap, you end up with double spaces and "fc- ntl"

> To avoid risks of undetected deadlock, provide an wrapper function
> utils, which in case flock() is not available, deflects all calls to
> lockf() instead.

The patch doesn't match the commit message. I think I'd rather go for a
function in the utils namespace, to make sure we will never mix flock()
and lock(), and to group all code related to compatibility with
different libc versions in a single place.

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/media_device.cpp | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 52c8e66e9e99..bffb241efa7c 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -20,6 +20,34 @@
>  
>  #include <libcamera/base/log.h>
>  
> +/*
> + * Android NDK workaround.
> + *
> + * Bionic does not implement the lockf function before API release 24.
> + * If we're building for a recent enough Android release include the
> + * correct header, if we're building for an older release, deflect
> + * flock() on the lockf() system call.
> + *
> + * A note on flock()/lockf() co-existency: it would be easier to change
> + * all usages of lockf() in libcamera to flock(). However lockf() is
> + * implemented as an interface on fcntl() while flock() is a system call
> + * since Linux v2.0. Locks set with lockf() won't be detected by flock()
> + * and vice-versa, hence mixing the two is highly undesirable. For this
> + * reason if lockf() is available prefer it, assuming all other
> + * applications in the system will do the same. Only deflect on flock()
> + * as last resort and only on Android systems.
> + */
> +#if __ANDROID_API__ >= 24
> +	#include <bits/lockf.h>

fcntl.h includes bits/lockf.h when __USE_GNU or __USE_BSD is defined,
which I think is the case. Is this thus needed, as we include fcntl.h at
the beginning of this file ?

> +#elif defined(__ANDROID_API__)
> +	#undef F_TLOCK
> +	#undef F_ULOCK
> +	#include <sys/file.h>
> +	#define F_TLOCK (LOCK_EX | LOCK_NB)
> +	#define F_ULOCK LOCK_UN
> +	#define lockf(f, c, o) flock(f, c)
> +#endif
> +
>  /**
>   * \file media_device.h
>   * \brief Provide a representation of a Linux kernel Media Controller device

-- 
Regards,

Laurent Pinchart