[libcamera-devel] [PATCH v3 07/11] libcamera: camera: Report function which fails access control

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Mar 25, 2021 at 01:42:27PM +0000, Kieran Bingham wrote:
> The camera object has a state machine to ensure calls are only made
> when in the correct state. It isn't easy to identify where things happen
> when assertions fail so add extra information to make this clearer.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/camera.cpp | 51 +++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index b86bff4745b2..336ab8695ab3 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -346,8 +346,9 @@ public:
>  		const std::set<Stream *> &streams);
>  	~Private();
>  
> -	int isAccessAllowed(State state, bool allowDisconnected = false) const;
> -	int isAccessAllowed(State low, State high,
> +	int isAccessAllowed(const char *from, State state,
> +			    bool allowDisconnected = false) const;
> +	int isAccessAllowed(const char *from, State low, State high,
>  			    bool allowDisconnected = false) const;

It's not very nice to have to pass the function name on every call :-S
Is there any way we could improve this ?

The option that first came to my mind was macros, which isn't really
nice either, and then I came across this really really nice trick:

	int isAccessAllowed(State state, bool allowDisconnected = false,
			    const char *from = __builtin_FUNCTION()) const;
	int isAccessAllowed(State low, State high,
			    bool allowDisconnected = false,
			    const char *from = __builtin_FUNCTION()) const;

>  
>  	void disconnect();
> @@ -384,7 +385,8 @@ static const char *const camera_state_names[] = {
>  	"Running",
>  };
>  
> -int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const
> +int Camera::Private::isAccessAllowed(const char *from, State state,
> +				     bool allowDisconnected) const
>  {
>  	if (!allowDisconnected && disconnected_)
>  		return -ENODEV;
> @@ -395,14 +397,16 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const
>  
>  	ASSERT(static_cast<unsigned int>(state) < std::size(camera_state_names));
>  
> -	LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState]
> -			   << " state trying operation requiring state "
> -			   << camera_state_names[state];
> +	LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState]

For operations coming from applications, should this really be a warning
? I suppose it's good to warn them. Could you mention this change in the
commit message ?

> +			     << " state trying operation ["
> +			     << from << "] requiring state "
> +			     << camera_state_names[state];
>  
>  	return -EACCES;
>  }
>  
> -int Camera::Private::isAccessAllowed(State low, State high,
> +int Camera::Private::isAccessAllowed(const char *from,
> +				     State low, State high,
>  				     bool allowDisconnected) const
>  {
>  	if (!allowDisconnected && disconnected_)
> @@ -415,10 +419,11 @@ int Camera::Private::isAccessAllowed(State low, State high,
>  	ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) &&
>  	       static_cast<unsigned int>(high) < std::size(camera_state_names));
>  
> -	LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState]
> -			   << " state trying operation requiring state between "
> -			   << camera_state_names[low] << " and "
> -			   << camera_state_names[high];
> +	LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState]
> +			     << " state trying operation [" << from
> +			     << "] requiring state between "
> +			     << camera_state_names[low] << " and "
> +			     << camera_state_names[high];
>  
>  	return -EACCES;
>  }
> @@ -646,7 +651,7 @@ int Camera::exportFrameBuffers(Stream *stream,
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> -	int ret = d->isAccessAllowed(Private::CameraConfigured);
> +	int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured);

__func__ is standard (since C99), __FUNCTION__ is gcc-specific. OK,
__builtin_FUNCTION() is also gcc-specific :-) I've tested it with gcc 7
to 10 and clang 7 to 11, it compiles on all of them.

With this change,

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

>  	if (ret < 0)
>  		return ret;
>  
> @@ -693,7 +698,7 @@ int Camera::acquire()
>  	 * No manual locking is required as PipelineHandler::lock() is
>  	 * thread-safe.
>  	 */
> -	int ret = d->isAccessAllowed(Private::CameraAvailable);
> +	int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraAvailable);
>  	if (ret < 0)
>  		return ret == -EACCES ? -EBUSY : ret;
>  
> @@ -726,7 +731,8 @@ int Camera::release()
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> -	int ret = d->isAccessAllowed(Private::CameraAvailable,
> +	int ret = d->isAccessAllowed(__FUNCTION__,
> +				     Private::CameraAvailable,
>  				     Private::CameraConfigured, true);
>  	if (ret < 0)
>  		return ret == -EACCES ? -EBUSY : ret;
> @@ -805,7 +811,8 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> -	int ret = d->isAccessAllowed(Private::CameraAvailable,
> +	int ret = d->isAccessAllowed(__FUNCTION__,
> +				     Private::CameraAvailable,
>  				     Private::CameraRunning);
>  	if (ret < 0)
>  		return nullptr;
> @@ -866,7 +873,8 @@ int Camera::configure(CameraConfiguration *config)
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> -	int ret = d->isAccessAllowed(Private::CameraAcquired,
> +	int ret = d->isAccessAllowed(__FUNCTION__,
> +				     Private::CameraAcquired,
>  				     Private::CameraConfigured);
>  	if (ret < 0)
>  		return ret;
> @@ -938,7 +946,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> -	int ret = d->isAccessAllowed(Private::CameraConfigured,
> +	int ret = d->isAccessAllowed(__FUNCTION__,
> +				     Private::CameraConfigured,
>  				     Private::CameraRunning);
>  	if (ret < 0)
>  		return nullptr;
> @@ -972,7 +981,7 @@ int Camera::queueRequest(Request *request)
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> -	int ret = d->isAccessAllowed(Private::CameraRunning);
> +	int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1022,7 +1031,7 @@ int Camera::start(const ControlList *controls)
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> -	int ret = d->isAccessAllowed(Private::CameraConfigured);
> +	int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1056,7 +1065,7 @@ int Camera::stop()
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> -	int ret = d->isAccessAllowed(Private::CameraRunning);
> +	int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1082,7 +1091,7 @@ void Camera::requestComplete(Request *request)
>  	Private *const d = LIBCAMERA_D_PTR();
>  
>  	/* Disconnected cameras are still able to complete requests. */
> -	int ret = d->isAccessAllowed(Private::CameraRunning);
> +	int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning);
>  	if (ret < 0 && ret != -ENODEV)
>  		LOG(Camera, Fatal) << "Trying to complete a request when stopped";
>  

-- 
Regards,

Laurent Pinchart