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

[Laurent Pinchart]

Hi Niklas,

On Tue, Jan 22, 2019 at 12:59:52PM +0100, Niklas Söderlund wrote:
>  On 2019-01-21 19:05:05 +0200, Laurent Pinchart wrote:
> > 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.

That would be my preference too. It feels more natural to me as well,
but I'm biased :-)

> >> 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