[libcamera-devel,1/3] Early return if no cameras are found on the system.
diff mbox series

Message ID 20201119123427.53864-2-email@uajain.com
State Superseded
Headers show
Series
  • simple-cam: Provide event-loop backed by libevent
Related show

Commit Message

Umang Jain Nov. 19, 2020, 12:34 p.m. UTC 
Failing to do so, the codepath will segfault while trying to acquire
a non-existent camera.

Signed-off-by: Umang Jain <email@uajain.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 simple-cam.cpp | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Kieran Bingham Nov. 19, 2020, 12:40 p.m. UTC | #1 
Hi Umang,

$SUBJECT is missing a "simple-cam:" prefix to match the others.

Was this patch already posted somewhere?

I don't seem to be able to find it, but I do half-recollect it ;-)
I just can't figure out where from.


On 19/11/2020 12:34, Umang Jain wrote:
> Failing to do so, the codepath will segfault while trying to acquire
> a non-existent camera.
> 
> Signed-off-by: Umang Jain <email@uajain.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  simple-cam.cpp | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/simple-cam.cpp b/simple-cam.cpp
> index 727bb6d..af9c8b1 100644
> --- a/simple-cam.cpp
> +++ b/simple-cam.cpp
> @@ -307,6 +307,13 @@ int main()
>  	for (std::unique_ptr<Request> &request : requests)
>  		camera->queueRequest(request.get());
>  
> +	if(!cm->cameras().size()) {
> +		std::cout << "No cameras were identified on the system."
> +			  << std::endl;
> +		cm->stop();
> +		return -1;

I think we should we return -ENODEV instead of -1 here. We already use
errno values (for example -ENOMEM is used).

--
Kieran


> +	}
> +
>  	/*
>  	 * --------------------------------------------------------------------
>  	 * Run an EventLoop
>
Umang Jain Nov. 19, 2020, 2:50 p.m. UTC | #2 
Hi Kieran,

On 11/19/20 6:10 PM, Kieran Bingham wrote:
> Hi Umang,
>
> $SUBJECT is missing a "simple-cam:" prefix to match the others.
>
> Was this patch already posted somewhere?
>
> I don't seem to be able to find it, but I do half-recollect it ;-)
> I just can't figure out where from.
Noted. Let me address this in a new version after 3/3 is reviewed. As 
mentioned in the cover-letter, I submitted  this patch earlier a while 
back on the list but it didn't make the merge somehow. Hence, I 
re-submitted it here. So let's try to get it merged via this series itself.
>
>
> On 19/11/2020 12:34, Umang Jain wrote:
>> Failing to do so, the codepath will segfault while trying to acquire
>> a non-existent camera.
>>
>> Signed-off-by: Umang Jain <email@uajain.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   simple-cam.cpp | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/simple-cam.cpp b/simple-cam.cpp
>> index 727bb6d..af9c8b1 100644
>> --- a/simple-cam.cpp
>> +++ b/simple-cam.cpp
>> @@ -307,6 +307,13 @@ int main()
>>   	for (std::unique_ptr<Request> &request : requests)
>>   		camera->queueRequest(request.get());
>>   
>> +	if(!cm->cameras().size()) {
>> +		std::cout << "No cameras were identified on the system."
>> +			  << std::endl;
>> +		cm->stop();
>> +		return -1;
> I think we should we return -ENODEV instead of -1 here. We already use
> errno values (for example -ENOMEM is used).
Noted.
>
> --
> Kieran
>
>
>> +	}
>> +
>>   	/*
>>   	 * --------------------------------------------------------------------
>>   	 * Run an EventLoop
>>
Kieran Bingham Nov. 19, 2020, 3:58 p.m. UTC | #3 
Hi Umang,

On 19/11/2020 14:50, Umang Jain wrote:
> Hi Kieran,
> 
> On 11/19/20 6:10 PM, Kieran Bingham wrote:
>> Hi Umang,
>>
>> $SUBJECT is missing a "simple-cam:" prefix to match the others.
>>
>> Was this patch already posted somewhere?
>>
>> I don't seem to be able to find it, but I do half-recollect it ;-)
>> I just can't figure out where from.

Ah - found the earlier series now.

> Noted. Let me address this in a new version after 3/3 is reviewed. As
> mentioned in the cover-letter, I submitted  this patch earlier a while
> back on the list but it didn't make the merge somehow. Hence, I

Hrm, sorry for letting that slip.


> re-submitted it here. So let's try to get it merged via this series itself.
>>

Yes, lets keep this series moving ;-)

>> On 19/11/2020 12:34, Umang Jain wrote:
>>> Failing to do so, the codepath will segfault while trying to acquire
>>> a non-existent camera.
>>>
>>> Signed-off-by: Umang Jain <email@uajain.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

This still stands with the -ENODEV resolved below, and the $SUBJECT
fixed of course.

>>> ---
>>>   simple-cam.cpp | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/simple-cam.cpp b/simple-cam.cpp
>>> index 727bb6d..af9c8b1 100644
>>> --- a/simple-cam.cpp
>>> +++ b/simple-cam.cpp
>>> @@ -307,6 +307,13 @@ int main()
>>>       for (std::unique_ptr<Request> &request : requests)
>>>           camera->queueRequest(request.get());
>>>   +    if(!cm->cameras().size()) {
>>> +        std::cout << "No cameras were identified on the system."
>>> +              << std::endl;
>>> +        cm->stop();
>>> +        return -1;
>> I think we should we return -ENODEV instead of -1 here. We already use
>> errno values (for example -ENOMEM is used).
> Noted.
>>
>> -- 
>> Kieran
>>
>>
>>> +    }
>>> +
>>>       /*
>>>        *
>>> --------------------------------------------------------------------
>>>        * Run an EventLoop
>>>
>
Laurent Pinchart Nov. 24, 2020, 2:40 p.m. UTC | #4 
Hi Kieran,

On Thu, Nov 19, 2020 at 12:40:08PM +0000, Kieran Bingham wrote:
> Hi Umang,
> 
> $SUBJECT is missing a "simple-cam:" prefix to match the others.
> 
> Was this patch already posted somewhere?
> 
> I don't seem to be able to find it, but I do half-recollect it ;-)
> I just can't figure out where from.
> 
> On 19/11/2020 12:34, Umang Jain wrote:
> > Failing to do so, the codepath will segfault while trying to acquire
> > a non-existent camera.
> > 
> > Signed-off-by: Umang Jain <email@uajain.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  simple-cam.cpp | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/simple-cam.cpp b/simple-cam.cpp
> > index 727bb6d..af9c8b1 100644
> > --- a/simple-cam.cpp
> > +++ b/simple-cam.cpp
> > @@ -307,6 +307,13 @@ int main()
> >  	for (std::unique_ptr<Request> &request : requests)
> >  		camera->queueRequest(request.get());
> >  
> > +	if(!cm->cameras().size()) {
> > +		std::cout << "No cameras were identified on the system."
> > +			  << std::endl;
> > +		cm->stop();
> > +		return -1;
> 
> I think we should we return -ENODEV instead of -1 here. We already use
> errno values (for example -ENOMEM is used).

Note that the return value of the main() function is a bit special. C
and C++ define EXIT_SUCCESS and EXIT_FAILURE to denote success and
failure respectively (not a surprise :-)). The value of those macros is
unspecified. See https://en.cppreference.com/w/c/program/EXIT_status.

(Interestingly, the standard defines that both EXIT_SUCCESS and the
value zero indicate successful program execution status, but don't
require EXIT_SUCCESS to be equal to zero.)

Other values are allowed, but only the 8 least significant bits are
conveyed to the parent, so the range of usable values is effectively
0-255. Furthermore, shells may reserve some values. For instance, bash
sets $? to 126 if a command is found but not executable, 127 if the
command is not found, and 128+N if the command terminates with a fatal
signal N.

As I don't expect we'll need to convey failure reasons through the exit
status for simple cam, how about returning either 0 and 1, or
EXIT_SUCCESS and EXIT_FAILURE ?

> > +	}
> > +
> >  	/*
> >  	 * --------------------------------------------------------------------
> >  	 * Run an EventLoop
Kieran Bingham Nov. 24, 2020, 7:22 p.m. UTC | #5 
Hi Laurent,

On 24/11/2020 14:40, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Thu, Nov 19, 2020 at 12:40:08PM +0000, Kieran Bingham wrote:
>> Hi Umang,
>>
>> $SUBJECT is missing a "simple-cam:" prefix to match the others.
>>
>> Was this patch already posted somewhere?
>>
>> I don't seem to be able to find it, but I do half-recollect it ;-)
>> I just can't figure out where from.
>>
>> On 19/11/2020 12:34, Umang Jain wrote:
>>> Failing to do so, the codepath will segfault while trying to acquire
>>> a non-existent camera.
>>>
>>> Signed-off-by: Umang Jain <email@uajain.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>>  simple-cam.cpp | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/simple-cam.cpp b/simple-cam.cpp
>>> index 727bb6d..af9c8b1 100644
>>> --- a/simple-cam.cpp
>>> +++ b/simple-cam.cpp
>>> @@ -307,6 +307,13 @@ int main()
>>>  	for (std::unique_ptr<Request> &request : requests)
>>>  		camera->queueRequest(request.get());
>>>  
>>> +	if(!cm->cameras().size()) {
>>> +		std::cout << "No cameras were identified on the system."
>>> +			  << std::endl;
>>> +		cm->stop();
>>> +		return -1;
>>
>> I think we should we return -ENODEV instead of -1 here. We already use
>> errno values (for example -ENOMEM is used).
> 
> Note that the return value of the main() function is a bit special. C
> and C++ define EXIT_SUCCESS and EXIT_FAILURE to denote success and
> failure respectively (not a surprise :-)). The value of those macros is
> unspecified. See https://en.cppreference.com/w/c/program/EXIT_status.
> 
> (Interestingly, the standard defines that both EXIT_SUCCESS and the
> value zero indicate successful program execution status, but don't
> require EXIT_SUCCESS to be equal to zero.)
> 
> Other values are allowed, but only the 8 least significant bits are
> conveyed to the parent, so the range of usable values is effectively
> 0-255. Furthermore, shells may reserve some values. For instance, bash
> sets $? to 126 if a command is found but not executable, 127 if the
> command is not found, and 128+N if the command terminates with a fatal
> signal N.
> 
> As I don't expect we'll need to convey failure reasons through the exit
> status for simple cam, how about returning either 0 and 1, or
> EXIT_SUCCESS and EXIT_FAILURE ?

Aha - good point, I'd totally missed that. I guess I don't spend much
time in main() :-)

I'd prefer the explicit EXIT_SUCCESS / EXIT_FAILURE in these cases, but
it might be nice to make the file consistent either way.

I'll leave it up to you Umang to decide what you prefer.

--
Kieran


> 
>>> +	}
>>> +
>>>  	/*
>>>  	 * --------------------------------------------------------------------
>>>  	 * Run an EventLoop
>

Patch
diff mbox series 

diff --git a/simple-cam.cpp b/simple-cam.cpp
index 727bb6d..af9c8b1 100644
--- a/simple-cam.cpp
+++ b/simple-cam.cpp
@@ -307,6 +307,13 @@  int main()
 	for (std::unique_ptr<Request> &request : requests)
 		camera->queueRequest(request.get());
 
+	if(!cm->cameras().size()) {
+		std::cout << "No cameras were identified on the system."
+			  << std::endl;
+		cm->stop();
+		return -1;
+	}
+
 	/*
 	 * --------------------------------------------------------------------
 	 * Run an EventLoop