[libcamera-devel] [PATCH v3 07/11] libcamera: camera: Report function which fails access control
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Mar 26 16:33:40 CET 2021
Hi Laurent,
On 26/03/2021 02:33, Laurent Pinchart wrote:
> 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:
Indeed, I was avoiding macros ...
>
> 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;
>
But this I like... trying it now ;-)
>>
>> 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 ?
Yes, I believe applications should know if they have caused an action
that was invalid.
>
>> + << " 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,
with __func__ or __builtin_FUNCTION()?
if __func__ is standard, should we use that as the default initialiser?
../src/libcamera/camera.cpp:353:27: error: ‘__func__’ is not defined
outside of function scope [-Werror]
353 | const char *from = __func__) const;
| ^~~~~~~~
Well that answers that then ;-)
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Works nicely, and now tells us what function caused the issue at least:
(Ignore the fact that these are successful, I hacked out the actual
conditional to check the output)
> [113:13:27.373321847] [1457151] WARN Camera camera.cpp:401 Camera in Running state trying operation [queueRequest] requiring state Running
> [113:13:27.408814517] [1457154] WARN Camera camera.cpp:401 Camera in Running state trying operation [requestComplete] requiring state Running
It's a shame we don't report the line number of the invalid access as
opposed to the line number of the isAccessAllowed() but I think we're ok
there. The 'from' tells us how to get back to who made the call.
>
>> 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
--
Kieran
More information about the libcamera-devel
mailing list