[libcamera-devel] [PATCH v3 07/11] libcamera: camera: Report function which fails access control
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 26 16:54:24 CET 2021
Hi Kieran,
On Fri, Mar 26, 2021 at 03:33:40PM +0000, Kieran Bingham wrote:
> On 26/03/2021 02:33, Laurent Pinchart wrote:
> > 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?
I'd love if we could. I prefer standard solutions, but
__builtin_FUNCTION() is such a neat trick :-)
> ../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.
Note that there's a __builtin_LINE() ;-) But I don't think we need to go
that way, the name should be enough.
Nitpicking, how about "trying queueRequest()" instead of "trying
operation [queueRequest]" ? Up to you.
> >> 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
More information about the libcamera-devel
mailing list