| Message ID | 20211007093937.121357-1-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, thank you for the patch. On Thu, Oct 7, 2021 at 6:39 PM Umang Jain <umang.jain@ideasonboard.com> 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@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) I wonder if this should be Fatal. I would like to ask others' opinions. Indeed it is not a big deal. Reviewed-by: Hirokazu Honda <hiroh@chromium.org> -Hiro > + << "Failed to get cros CameraBufferManager instance"; > + return; > + } > > int ret = bufferManager_->Register(camera3Buffer); > if (ret) { > -- > 2.31.1 >
Hi Umang, 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@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. > > int ret = bufferManager_->Register(camera3Buffer); > if (ret) { > -- > 2.31.1 >
Hi Jacopo, On 10/11/21 3:53 PM, Jacopo Mondi wrote: > Hi Umang, > > 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@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. > >> int ret = bufferManager_->Register(camera3Buffer); >> if (ret) { >> -- >> 2.31.1 >>
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@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@ideasonboard.com> > >> int ret = bufferManager_->Register(camera3Buffer); > >> if (ret) {
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; + } int ret = bufferManager_->Register(camera3Buffer); if (ret) {
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@ideasonboard.com> --- src/android/mm/cros_camera_buffer.cpp | 5 +++++ 1 file changed, 5 insertions(+)