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

[Kieran Bingham]

Hi All,

On 22/01/2019 11:59, Niklas Söderlund wrote:
> 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.

I see nothing wrong with negative return codes, especially if we
document it.


> - zlib defines a set of values Z_OK, Z_STREAM_ERROR etc.
> - libpcap like zlib defines PCAP_ERROR_ACTIVATED, PCAP_ERROR etc.

https://github.com/the-tcpdump-group/libpcap/blob/master/pcap/pcap.h#L296

Those are negative numbers. We can define our own, but I don't see
anything wrong with using standard errno values.



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