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

17939 diff mbox series

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.)

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

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/media_device.cpp | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

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