[libcamera-devel,v5,5/5] libcamera: camera_manager: Enforce unique camera names
diff mbox series

Message ID 20200729115055.3840110-6-niklas.soderlund@ragnatech.se
State Superseded
Delegated to: Niklas Söderlund
Headers show
Series
  • libcamera: Generate unique and stable camera names
Related show

Commit Message

Niklas Söderlund July 29, 2020, 11:50 a.m. UTC 
The camera name have always been documented that it should be unique but
it has never been enforced. Change this by refusing to add cameras to
the CameraManager that would create two cameras with the exact same
name.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
* Changes since v4
- Update string in error message.

* Changes since v3
- Update commit message.
---
 src/libcamera/camera_manager.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart Aug. 2, 2020, 9:19 p.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Wed, Jul 29, 2020 at 01:50:55PM +0200, Niklas Söderlund wrote:
> The camera name have always been documented that it should be unique but
> it has never been enforced. Change this by refusing to add cameras to
> the CameraManager that would create two cameras with the exact same
> name.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> * Changes since v4
> - Update string in error message.
> 
> * Changes since v3
> - Update commit message.
> ---
>  src/libcamera/camera_manager.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index f60491d2c1a7500f..47e0f3129d346183 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -178,10 +178,10 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
>  
>  	for (std::shared_ptr<Camera> c : cameras_) {
>  		if (c->name() == camera->name()) {
> -			LOG(Camera, Warning)
> -				<< "Registering camera with duplicate name '"
> +			LOG(Camera, Error)
> +				<< "Skip registration of a camera with a duplicated name '"
>  				<< camera->name() << "'";
> -			break;
> +			return;

Shouldn't the error be propagated up to the pipelien handler ? Other
than that, this looks good to me, but I think we need to rename "name"
to "id".

>  		}
>  	}
>
Niklas Söderlund Aug. 2, 2020, 11:11 p.m. UTC | #2 
Hi Laurent,

Thanks for your comments.

On 2020-08-03 00:19:10 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Wed, Jul 29, 2020 at 01:50:55PM +0200, Niklas Söderlund wrote:
> > The camera name have always been documented that it should be unique but
> > it has never been enforced. Change this by refusing to add cameras to
> > the CameraManager that would create two cameras with the exact same
> > name.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> > * Changes since v4
> > - Update string in error message.
> > 
> > * Changes since v3
> > - Update commit message.
> > ---
> >  src/libcamera/camera_manager.cpp | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index f60491d2c1a7500f..47e0f3129d346183 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -178,10 +178,10 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> >  
> >  	for (std::shared_ptr<Camera> c : cameras_) {
> >  		if (c->name() == camera->name()) {
> > -			LOG(Camera, Warning)
> > -				<< "Registering camera with duplicate name '"
> > +			LOG(Camera, Error)
> > +				<< "Skip registration of a camera with a duplicated name '"
> >  				<< camera->name() << "'";
> > -			break;
> > +			return;
> 
> Shouldn't the error be propagated up to the pipelien handler ? Other
> than that, this looks good to me, but I think we need to rename "name"
> to "id".

I'm not sure it's useful to propagate the error to the pipeline handler, 
what will it do with it? The pipeline can't change the ID and try adding 
the camera again, as this would violate the requirements we have on IDs
being stable and unique. All it can do is print yet another LOG message 
saying a duplicated ID is found.

I originally had this as a Fatal error as this really should not happen, 
I'm tempted to switch it back. What do you think?

> 
> >  		}
> >  	}
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart Aug. 2, 2020, 11:16 p.m. UTC | #3 
Hi Niklas,

On Mon, Aug 03, 2020 at 01:11:16AM +0200, Niklas Söderlund wrote:
> On 2020-08-03 00:19:10 +0300, Laurent Pinchart wrote:
> > On Wed, Jul 29, 2020 at 01:50:55PM +0200, Niklas Söderlund wrote:
> > > The camera name have always been documented that it should be unique but
> > > it has never been enforced. Change this by refusing to add cameras to
> > > the CameraManager that would create two cameras with the exact same
> > > name.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > > * Changes since v4
> > > - Update string in error message.
> > > 
> > > * Changes since v3
> > > - Update commit message.
> > > ---
> > >  src/libcamera/camera_manager.cpp | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > > index f60491d2c1a7500f..47e0f3129d346183 100644
> > > --- a/src/libcamera/camera_manager.cpp
> > > +++ b/src/libcamera/camera_manager.cpp
> > > @@ -178,10 +178,10 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> > >  
> > >  	for (std::shared_ptr<Camera> c : cameras_) {
> > >  		if (c->name() == camera->name()) {
> > > -			LOG(Camera, Warning)
> > > -				<< "Registering camera with duplicate name '"
> > > +			LOG(Camera, Error)
> > > +				<< "Skip registration of a camera with a duplicated name '"
> > >  				<< camera->name() << "'";
> > > -			break;
> > > +			return;
> > 
> > Shouldn't the error be propagated up to the pipelien handler ? Other
> > than that, this looks good to me, but I think we need to rename "name"
> > to "id".
> 
> I'm not sure it's useful to propagate the error to the pipeline handler, 
> what will it do with it? The pipeline can't change the ID and try adding 
> the camera again, as this would violate the requirements we have on IDs
> being stable and unique. All it can do is print yet another LOG message 
> saying a duplicated ID is found.

I'm fine not propagating the error as long as we can ensure that no
memory leak or bad memory access will occur. The pipeline handler will
think registration succeeded, and will return true from match(). Can you
check that nothing can crash ?

> I originally had this as a Fatal error as this really should not happen, 
> I'm tempted to switch it back. What do you think?

I think it could make sense, yes.

> > >  		}
> > >  	}
> > >

Patch
diff mbox series 

diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index f60491d2c1a7500f..47e0f3129d346183 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -178,10 +178,10 @@  void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
 
 	for (std::shared_ptr<Camera> c : cameras_) {
 		if (c->name() == camera->name()) {
-			LOG(Camera, Warning)
-				<< "Registering camera with duplicate name '"
+			LOG(Camera, Error)
+				<< "Skip registration of a camera with a duplicated name '"
 				<< camera->name() << "'";
-			break;
+			return;
 		}
 	}