[libcamera-devel] [PATCH] libcamera: camera_manager: Return EBUSY if enumerator exists

[Niklas Söderlund]

Hi,

On 2019-01-21 19:05:05 +0200, Laurent Pinchart wrote:
> Hello,
> 
> On Mon, Jan 21, 2019 at 05:01:40PM +0100, Niklas Söderlund wrote:
> > On 2019-01-21 11:41:26 +0000, Kieran Bingham wrote:
> > > In the case that someone calls CameraManager::start() and it has already
> > > started/enumerated, instead of returning -ENODEV, return -EBUSY.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > While working with this on the cam utility it struck me, should we 
> > return negative error codes from CameraManager? This faces the 
> > application and it might be unexpected for it to receive a negative 
> > error.
> 
> We have to standardize on error codes (and of course document them). Is
> it unusual for libraries to return negative error codes ? I've been
> working with the kernel for so long that it just seems natural to me :-)

I agree negative error codes seems natural. I'm not sure how to best 
handle that in the interface between a library and application. I did a 
quick survey of a handful of libraries and it seems there is no 
'standard' way of doing it.

- zlib defines a set of values Z_OK, Z_STREAM_ERROR etc.
- libpcap like zlib defines PCAP_ERROR_ACTIVATED, PCAP_ERROR etc.
- libssh seems to mostly do '0 on success, < 0 on error'.

To no surprise it seems a return code 0 signals OK in all of them. So 
maybe we are OK mimicking libssh which seems to be what we are doing 
today? To me that feels more natural then adding a set of values an 
application needs to check for which are library specific.

> 
> > In any case this change matches our documentation and is a good change.
> > 
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > 
> > > ---
> > >  src/libcamera/camera_manager.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > > index d76eaa7ace86..21cb36dcb9b5 100644
> > > --- a/src/libcamera/camera_manager.cpp
> > > +++ b/src/libcamera/camera_manager.cpp
> > > @@ -76,7 +76,7 @@ CameraManager::~CameraManager()
> > >  int CameraManager::start()
> > >  {
> > >  	if (enumerator_)
> > > -		return -ENODEV;
> > > +		return -EBUSY;
> > >  
> > >  	enumerator_ = DeviceEnumerator::create();
> > >  	if (enumerator_->enumerate())
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund