[libcamera-devel] [PATCH 1/2] android: mm: Null check for CameraBufferManager

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Mon, Oct 11, 2021 at 05:36:49PM +0530, Umang Jain wrote:
> On 10/11/21 3:53 PM, Jacopo Mondi wrote:
> > On Thu, Oct 07, 2021 at 03:09:36PM +0530, Umang Jain wrote:
> >> cros::CameraBufferManager can be nullptr if there is an error in
> >> its creation. Place a null-check guard to check it.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >>   src/android/mm/cros_camera_buffer.cpp | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> >> index 86770135..97a04c68 100644
> >> --- a/src/android/mm/cros_camera_buffer.cpp
> >> +++ b/src/android/mm/cros_camera_buffer.cpp
> >> @@ -60,6 +60,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> >>   	  registered_(false)
> >>   {
> >>   	bufferManager_ = cros::CameraBufferManager::GetInstance();
> >> +	if (!bufferManager_) {
> >> +		LOG(HAL, Error)
> >> +			<< "Failed to get cros CameraBufferManager instance";
> >> +		return;
> >> +	}
> > I'm not sure if this could happen or is just a "just in case".
> > Anyway, the HAL won't be able to operate in that case and this is a
> > constructor so it's impossible to propagate the error up to handle it
> > gracefully: I would use Fatal.
> 
> 
> The "just in case" scenario is well documented in the framework
> 
>        // Gets the singleton instance.  Returns nullptr if any error 
> occurrs during
>        // instance creation.
> 
> in 
> $chromiumos/src/platform2/camera/include/cros-camera/camera_buffer_manager.h 
> 
> 
> I haven't been able to trigger the error condition, but since it's 
> document it's better to guard it beforehand.
> 
> Fatal sounds good to me too.

With Fatal,

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


> >>   	int ret = bufferManager_->Register(camera3Buffer);
> >>   	if (ret) {

-- 
Regards,

Laurent Pinchart