| Message ID | 20190121114126.19577-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 29c3b54f0c8ec7e3aeed8971d24f939bc4bfab04 |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Mon, Jan 21, 2019 at 11:41:26AM +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@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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())
Hi Kieran, 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@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. In any case this change matches our documentation and is a good change. Reviewed-by: Niklas Söderlund <niklas.soderlund@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()) > -- > 2.17.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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 :-) > In any case this change matches our documentation and is a good change. > > Reviewed-by: Niklas Söderlund <niklas.soderlund@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())
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@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@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
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@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@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 >
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@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@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())
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())
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@ideasonboard.com> --- src/libcamera/camera_manager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)