| Message ID | 20190718042754.26535-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote: > Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing > without crashing. > > Camera::configure() validates that each StreamConfiguration has a > Stream* and reports a Fatal error otherwise. The code then goes on to > dereference the Stream pointer which will be a nullptr in the event of > the Fatal error being triggered. > > In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should > attempt to continue or report the error up the call-stack. > > Make Camera::configure() return an error in the event that the Stream* > is not valid. This will cause the configure operation to fail and the > application will be notified just as other errors in the > Camera::configure() operation do. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera.cpp | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 76c737cb9381..7ad9e0e48686 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config) > activeStreams_.clear(); > for (const StreamConfiguration &cfg : *config) { > Stream *stream = cfg.stream(); > - if (!stream) > + if (!stream) { > LOG(Camera, Fatal) > << "Pipeline handler failed to update stream configuration"; > + return -EINVAL; Overall this makes sense, but I wonder if -EINVAL is the appropriate error code. The issue at hand is that the pipeline handler is buggy. A specific error code to denote errors that are not supposed to happen ever would be best I think (and bonus points if we could use that error code consistently through libcamera). > + } > > stream->configuration_ = cfg; > activeStreams_.insert(stream);
Hi Laurent, On 18/07/2019 14:56, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote: >> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing >> without crashing. >> >> Camera::configure() validates that each StreamConfiguration has a >> Stream* and reports a Fatal error otherwise. The code then goes on to >> dereference the Stream pointer which will be a nullptr in the event of >> the Fatal error being triggered. >> >> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should >> attempt to continue or report the error up the call-stack. >> >> Make Camera::configure() return an error in the event that the Stream* >> is not valid. This will cause the configure operation to fail and the >> application will be notified just as other errors in the >> Camera::configure() operation do. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/camera.cpp | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >> index 76c737cb9381..7ad9e0e48686 100644 >> --- a/src/libcamera/camera.cpp >> +++ b/src/libcamera/camera.cpp >> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config) >> activeStreams_.clear(); >> for (const StreamConfiguration &cfg : *config) { >> Stream *stream = cfg.stream(); >> - if (!stream) >> + if (!stream) { >> LOG(Camera, Fatal) >> << "Pipeline handler failed to update stream configuration"; >> + return -EINVAL; > > Overall this makes sense, but I wonder if -EINVAL is the appropriate > error code. The issue at hand is that the pipeline handler is buggy. A > specific error code to denote errors that are not supposed to happen > ever would be best I think (and bonus points if we could use that error > code consistently through libcamera). -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't have a stream pointer - as long as that doesn't get too confusing against other types of pipe :) Or we could use: "ESTRPIPE 86 Streams pipe error"? But then again for a generic 'Fatal' error code we could choose one of: EOWNERDEAD 130 Owner died ENOTRECOVERABLE 131 State not recoverable I quite like both of those for a 'fatal' error. >> + } >> >> stream->configuration_ = cfg; >> activeStreams_.insert(stream); >
Hi Kieran, Thanks for your work. On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote: > Hi Laurent, > > On 18/07/2019 14:56, Laurent Pinchart wrote: > > Hi Kieran, > > > > Thank you for the patch. > > > > On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote: > >> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing > >> without crashing. > >> > >> Camera::configure() validates that each StreamConfiguration has a > >> Stream* and reports a Fatal error otherwise. The code then goes on to > >> dereference the Stream pointer which will be a nullptr in the event of > >> the Fatal error being triggered. > >> > >> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should > >> attempt to continue or report the error up the call-stack. > >> > >> Make Camera::configure() return an error in the event that the Stream* > >> is not valid. This will cause the configure operation to fail and the > >> application will be notified just as other errors in the > >> Camera::configure() operation do. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/libcamera/camera.cpp | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > >> index 76c737cb9381..7ad9e0e48686 100644 > >> --- a/src/libcamera/camera.cpp > >> +++ b/src/libcamera/camera.cpp > >> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config) > >> activeStreams_.clear(); > >> for (const StreamConfiguration &cfg : *config) { > >> Stream *stream = cfg.stream(); > >> - if (!stream) > >> + if (!stream) { > >> LOG(Camera, Fatal) > >> << "Pipeline handler failed to update stream configuration"; > >> + return -EINVAL; > > > > Overall this makes sense, but I wonder if -EINVAL is the appropriate > > error code. The issue at hand is that the pipeline handler is buggy. A > > specific error code to denote errors that are not supposed to happen > > ever would be best I think (and bonus points if we could use that error > > code consistently through libcamera). > > -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't > have a stream pointer - as long as that doesn't get too confusing > against other types of pipe :) > > Or we could use: "ESTRPIPE 86 Streams pipe error"? > > But then again for a generic 'Fatal' error code we could choose one of: > > EOWNERDEAD 130 Owner died > ENOTRECOVERABLE 131 State not recoverable > > I quite like both of those for a 'fatal' error. Is this not an academic question? As LOG(..., Fatal) will never return and terminate the application? If we need a return statement to keep analysers happy add one in the LOG macro. The return code could be EXIT_FAILURE. > > > >> + } > >> > >> stream->configuration_ = cfg; > >> activeStreams_.insert(stream); > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On 20/07/2019 14:22, Niklas Söderlund wrote: > Hi Kieran, > > Thanks for your work. > > On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote: >> Hi Laurent, >> >> On 18/07/2019 14:56, Laurent Pinchart wrote: >>> Hi Kieran, >>> >>> Thank you for the patch. >>> >>> On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote: >>>> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing >>>> without crashing. >>>> >>>> Camera::configure() validates that each StreamConfiguration has a >>>> Stream* and reports a Fatal error otherwise. The code then goes on to >>>> dereference the Stream pointer which will be a nullptr in the event of >>>> the Fatal error being triggered. >>>> >>>> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should >>>> attempt to continue or report the error up the call-stack. >>>> >>>> Make Camera::configure() return an error in the event that the Stream* >>>> is not valid. This will cause the configure operation to fail and the >>>> application will be notified just as other errors in the >>>> Camera::configure() operation do. >>>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> src/libcamera/camera.cpp | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >>>> index 76c737cb9381..7ad9e0e48686 100644 >>>> --- a/src/libcamera/camera.cpp >>>> +++ b/src/libcamera/camera.cpp >>>> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config) >>>> activeStreams_.clear(); >>>> for (const StreamConfiguration &cfg : *config) { >>>> Stream *stream = cfg.stream(); >>>> - if (!stream) >>>> + if (!stream) { >>>> LOG(Camera, Fatal) >>>> << "Pipeline handler failed to update stream configuration"; >>>> + return -EINVAL; >>> >>> Overall this makes sense, but I wonder if -EINVAL is the appropriate >>> error code. The issue at hand is that the pipeline handler is buggy. A >>> specific error code to denote errors that are not supposed to happen >>> ever would be best I think (and bonus points if we could use that error >>> code consistently through libcamera). >> >> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't >> have a stream pointer - as long as that doesn't get too confusing >> against other types of pipe :) >> >> Or we could use: "ESTRPIPE 86 Streams pipe error"? >> >> But then again for a generic 'Fatal' error code we could choose one of: >> >> EOWNERDEAD 130 Owner died >> ENOTRECOVERABLE 131 State not recoverable >> >> I quite like both of those for a 'fatal' error. > > Is this not an academic question? As LOG(..., Fatal) will never return > and terminate the application? Currently, you are correct - but while discussing with Laurent on another patch somewhere, or perhaps it was IRC - I can't recall, we realised that we might have to make LOG(x, Fatal) *non-fatal* on release builds. (Just as assertions are compiled out in release builds) It might be that we leave this to a packaging policy as well. Anyway, this is the only part of the code that in the event of the failure being detected would then go on to do a null-dereference - so I thought the small fix up was worth it. Perhaps I should add a comment before the return explaining that this code is unreachable in debug builds, but could be enabled in future release builds? > If we need a return statement to keep analysers happy add one in the LOG > macro. The return code could be EXIT_FAILURE. Eeeep - no - I really don't want to have a macro which returns implicitly. I would really prefer to keep return paths explicit. > >> >> >>>> + } >>>> >>>> stream->configuration_ = cfg; >>>> activeStreams_.insert(stream); >>> >> >> -- >> Regards >> -- >> Kieran >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
Hello, On Mon, Jul 22, 2019 at 10:54:26AM +0100, Kieran Bingham wrote: > On 20/07/2019 14:22, Niklas Söderlund wrote: > > On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote: > >> On 18/07/2019 14:56, Laurent Pinchart wrote: > >>> On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote: > >>>> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing > >>>> without crashing. > >>>> > >>>> Camera::configure() validates that each StreamConfiguration has a > >>>> Stream* and reports a Fatal error otherwise. The code then goes on to > >>>> dereference the Stream pointer which will be a nullptr in the event of > >>>> the Fatal error being triggered. > >>>> > >>>> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should > >>>> attempt to continue or report the error up the call-stack. > >>>> > >>>> Make Camera::configure() return an error in the event that the Stream* > >>>> is not valid. This will cause the configure operation to fail and the > >>>> application will be notified just as other errors in the > >>>> Camera::configure() operation do. > >>>> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> --- > >>>> src/libcamera/camera.cpp | 4 +++- > >>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > >>>> index 76c737cb9381..7ad9e0e48686 100644 > >>>> --- a/src/libcamera/camera.cpp > >>>> +++ b/src/libcamera/camera.cpp > >>>> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config) > >>>> activeStreams_.clear(); > >>>> for (const StreamConfiguration &cfg : *config) { > >>>> Stream *stream = cfg.stream(); > >>>> - if (!stream) > >>>> + if (!stream) { > >>>> LOG(Camera, Fatal) > >>>> << "Pipeline handler failed to update stream configuration"; > >>>> + return -EINVAL; > >>> > >>> Overall this makes sense, but I wonder if -EINVAL is the appropriate > >>> error code. The issue at hand is that the pipeline handler is buggy. A > >>> specific error code to denote errors that are not supposed to happen > >>> ever would be best I think (and bonus points if we could use that error > >>> code consistently through libcamera). > >> > >> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't > >> have a stream pointer - as long as that doesn't get too confusing > >> against other types of pipe :) > >> > >> Or we could use: "ESTRPIPE 86 Streams pipe error"? > >> > >> But then again for a generic 'Fatal' error code we could choose one of: > >> > >> EOWNERDEAD 130 Owner died > >> ENOTRECOVERABLE 131 State not recoverable > >> > >> I quite like both of those for a 'fatal' error. I quite like ENOTRECOVERABLE too. > > Is this not an academic question? As LOG(..., Fatal) will never return > > and terminate the application? > > Currently, you are correct - but while discussing with Laurent on > another patch somewhere, or perhaps it was IRC - I can't recall, we > realised that we might have to make LOG(x, Fatal) *non-fatal* on release > builds. > > (Just as assertions are compiled out in release builds) > > It might be that we leave this to a packaging policy as well. > > Anyway, this is the only part of the code that in the event of the > failure being detected would then go on to do a null-dereference - so I > thought the small fix up was worth it. > > Perhaps I should add a comment before the return explaining that this > code is unreachable in debug builds, but could be enabled in future > release builds? I think mentioning it in the commit log is enough, there's no need to sprinkle copies of the information through the code. > > If we need a return statement to keep analysers happy add one in the LOG > > macro. The return code could be EXIT_FAILURE. > > Eeeep - no - I really don't want to have a macro which returns implicitly. > > I would really prefer to keep return paths explicit. I also dislike hiding return statements in macros, even if it could help standardising the return value. Let's also note that LOG(..., Fatal) can be used in functions returning any type, so we can't simply return -EWHATEVER there. > >>>> + } > >>>> > >>>> stream->configuration_ = cfg; > >>>> activeStreams_.insert(stream);
Hi, On 2019-07-30 07:09:05 +0300, Laurent Pinchart wrote: > Hello, > > On Mon, Jul 22, 2019 at 10:54:26AM +0100, Kieran Bingham wrote: > > On 20/07/2019 14:22, Niklas Söderlund wrote: > > > On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote: > > >> On 18/07/2019 14:56, Laurent Pinchart wrote: > > >>> On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote: > > >>>> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing > > >>>> without crashing. > > >>>> > > >>>> Camera::configure() validates that each StreamConfiguration has a > > >>>> Stream* and reports a Fatal error otherwise. The code then goes on to > > >>>> dereference the Stream pointer which will be a nullptr in the event of > > >>>> the Fatal error being triggered. > > >>>> > > >>>> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should > > >>>> attempt to continue or report the error up the call-stack. > > >>>> > > >>>> Make Camera::configure() return an error in the event that the Stream* > > >>>> is not valid. This will cause the configure operation to fail and the > > >>>> application will be notified just as other errors in the > > >>>> Camera::configure() operation do. > > >>>> > > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>>> --- > > >>>> src/libcamera/camera.cpp | 4 +++- > > >>>> 1 file changed, 3 insertions(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > >>>> index 76c737cb9381..7ad9e0e48686 100644 > > >>>> --- a/src/libcamera/camera.cpp > > >>>> +++ b/src/libcamera/camera.cpp > > >>>> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config) > > >>>> activeStreams_.clear(); > > >>>> for (const StreamConfiguration &cfg : *config) { > > >>>> Stream *stream = cfg.stream(); > > >>>> - if (!stream) > > >>>> + if (!stream) { > > >>>> LOG(Camera, Fatal) > > >>>> << "Pipeline handler failed to update stream configuration"; > > >>>> + return -EINVAL; > > >>> > > >>> Overall this makes sense, but I wonder if -EINVAL is the appropriate > > >>> error code. The issue at hand is that the pipeline handler is buggy. A > > >>> specific error code to denote errors that are not supposed to happen > > >>> ever would be best I think (and bonus points if we could use that error > > >>> code consistently through libcamera). > > >> > > >> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't > > >> have a stream pointer - as long as that doesn't get too confusing > > >> against other types of pipe :) > > >> > > >> Or we could use: "ESTRPIPE 86 Streams pipe error"? > > >> > > >> But then again for a generic 'Fatal' error code we could choose one of: > > >> > > >> EOWNERDEAD 130 Owner died > > >> ENOTRECOVERABLE 131 State not recoverable > > >> > > >> I quite like both of those for a 'fatal' error. > > I quite like ENOTRECOVERABLE too. > > > > Is this not an academic question? As LOG(..., Fatal) will never return > > > and terminate the application? > > > > Currently, you are correct - but while discussing with Laurent on > > another patch somewhere, or perhaps it was IRC - I can't recall, we > > realised that we might have to make LOG(x, Fatal) *non-fatal* on release > > builds. > > > > (Just as assertions are compiled out in release builds) > > > > It might be that we leave this to a packaging policy as well. > > > > Anyway, this is the only part of the code that in the event of the > > failure being detected would then go on to do a null-dereference - so I > > thought the small fix up was worth it. > > > > Perhaps I should add a comment before the return explaining that this > > code is unreachable in debug builds, but could be enabled in future > > release builds? > > I think mentioning it in the commit log is enough, there's no need to > sprinkle copies of the information through the code. I'm not sure I like the idea of allowing continued execution after a LOG(..., Fatal) in release builds. It feels a bit like Java, whenever one looks at a process logs it's sprinkled with unhandled expectation log messages and trying to narrow down a problem is impossible so one just gives up and once prejudice against the language is reinforced ;-) I'm not against changing/removing LOG(..., Fatal) so that it always allow continued execution after it's been called. But I think it should be the same behavior always, we will have enough trouble as it is to test all different error paths :-) ASSERT() for me is different, it's usually at the start of functions and could be in hot paths and do not alter the execution order of functions so it make sens to disable them in release builds as it decreases the execution time. While I think of LOG(..., Fatal) as BUG() if it happens we are in deep shit and there is no point in trying to recover from it. > > > > If we need a return statement to keep analysers happy add one in the LOG > > > macro. The return code could be EXIT_FAILURE. > > > > Eeeep - no - I really don't want to have a macro which returns implicitly. > > > > I would really prefer to keep return paths explicit. > > I also dislike hiding return statements in macros, even if it could help > standardising the return value. Let's also note that LOG(..., Fatal) can > be used in functions returning any type, so we can't simply return > -EWHATEVER there. > > > >>>> + } > > >>>> > > >>>> stream->configuration_ = cfg; > > >>>> activeStreams_.insert(stream); > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Tue, Jul 30, 2019 at 07:37:15AM +0200, Niklas Söderlund wrote: > On 2019-07-30 07:09:05 +0300, Laurent Pinchart wrote: > > On Mon, Jul 22, 2019 at 10:54:26AM +0100, Kieran Bingham wrote: > >> On 20/07/2019 14:22, Niklas Söderlund wrote: > >>> On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote: > >>>> On 18/07/2019 14:56, Laurent Pinchart wrote: > >>>>> On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote: > >>>>>> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing > >>>>>> without crashing. > >>>>>> > >>>>>> Camera::configure() validates that each StreamConfiguration has a > >>>>>> Stream* and reports a Fatal error otherwise. The code then goes on to > >>>>>> dereference the Stream pointer which will be a nullptr in the event of > >>>>>> the Fatal error being triggered. > >>>>>> > >>>>>> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should > >>>>>> attempt to continue or report the error up the call-stack. > >>>>>> > >>>>>> Make Camera::configure() return an error in the event that the Stream* > >>>>>> is not valid. This will cause the configure operation to fail and the > >>>>>> application will be notified just as other errors in the > >>>>>> Camera::configure() operation do. > >>>>>> > >>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>>>> --- > >>>>>> src/libcamera/camera.cpp | 4 +++- > >>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > >>>>>> index 76c737cb9381..7ad9e0e48686 100644 > >>>>>> --- a/src/libcamera/camera.cpp > >>>>>> +++ b/src/libcamera/camera.cpp > >>>>>> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config) > >>>>>> activeStreams_.clear(); > >>>>>> for (const StreamConfiguration &cfg : *config) { > >>>>>> Stream *stream = cfg.stream(); > >>>>>> - if (!stream) > >>>>>> + if (!stream) { > >>>>>> LOG(Camera, Fatal) > >>>>>> << "Pipeline handler failed to update stream configuration"; > >>>>>> + return -EINVAL; > >>>>> > >>>>> Overall this makes sense, but I wonder if -EINVAL is the appropriate > >>>>> error code. The issue at hand is that the pipeline handler is buggy. A > >>>>> specific error code to denote errors that are not supposed to happen > >>>>> ever would be best I think (and bonus points if we could use that error > >>>>> code consistently through libcamera). > >>>> > >>>> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't > >>>> have a stream pointer - as long as that doesn't get too confusing > >>>> against other types of pipe :) > >>>> > >>>> Or we could use: "ESTRPIPE 86 Streams pipe error"? > >>>> > >>>> But then again for a generic 'Fatal' error code we could choose one of: > >>>> > >>>> EOWNERDEAD 130 Owner died > >>>> ENOTRECOVERABLE 131 State not recoverable > >>>> > >>>> I quite like both of those for a 'fatal' error. > > > > I quite like ENOTRECOVERABLE too. > > > >>> Is this not an academic question? As LOG(..., Fatal) will never return > >>> and terminate the application? > >> > >> Currently, you are correct - but while discussing with Laurent on > >> another patch somewhere, or perhaps it was IRC - I can't recall, we > >> realised that we might have to make LOG(x, Fatal) *non-fatal* on release > >> builds. > >> > >> (Just as assertions are compiled out in release builds) > >> > >> It might be that we leave this to a packaging policy as well. > >> > >> Anyway, this is the only part of the code that in the event of the > >> failure being detected would then go on to do a null-dereference - so I > >> thought the small fix up was worth it. > >> > >> Perhaps I should add a comment before the return explaining that this > >> code is unreachable in debug builds, but could be enabled in future > >> release builds? > > > > I think mentioning it in the commit log is enough, there's no need to > > sprinkle copies of the information through the code. > > I'm not sure I like the idea of allowing continued execution after a > LOG(..., Fatal) in release builds. It feels a bit like Java, whenever > one looks at a process logs it's sprinkled with unhandled expectation > log messages and trying to narrow down a problem is impossible so one > just gives up and once prejudice against the language is reinforced ;-) > > I'm not against changing/removing LOG(..., Fatal) so that it always > allow continued execution after it's been called. But I think it should > be the same behavior always, we will have enough trouble as it is to > test all different error paths :-) > > ASSERT() for me is different, it's usually at the start of functions and > could be in hot paths and do not alter the execution order of functions > so it make sens to disable them in release builds as it decreases the > execution time. While I think of LOG(..., Fatal) as BUG() if it happens > we are in deep shit and there is no point in trying to recover from it. >From a libcamera point of view that's true, but from an application point of view there may be a use for closing gracefully and saving data. For me LOG(..., Fatal) is a LOG() + ASSERT(), but if we have to keep the same behaviour in both release and debug builds, I would prefer removing the std::abort() call completely from LOG(..., Fatal). > >>> If we need a return statement to keep analysers happy add one in the LOG > >>> macro. The return code could be EXIT_FAILURE. > >> > >> Eeeep - no - I really don't want to have a macro which returns implicitly. > >> > >> I would really prefer to keep return paths explicit. > > > > I also dislike hiding return statements in macros, even if it could help > > standardising the return value. Let's also note that LOG(..., Fatal) can > > be used in functions returning any type, so we can't simply return > > -EWHATEVER there. > > > >>>>>> + } > >>>>>> > >>>>>> stream->configuration_ = cfg; > >>>>>> activeStreams_.insert(stream);
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 76c737cb9381..7ad9e0e48686 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config) activeStreams_.clear(); for (const StreamConfiguration &cfg : *config) { Stream *stream = cfg.stream(); - if (!stream) + if (!stream) { LOG(Camera, Fatal) << "Pipeline handler failed to update stream configuration"; + return -EINVAL; + } stream->configuration_ = cfg; activeStreams_.insert(stream);
Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing without crashing. Camera::configure() validates that each StreamConfiguration has a Stream* and reports a Fatal error otherwise. The code then goes on to dereference the Stream pointer which will be a nullptr in the event of the Fatal error being triggered. In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should attempt to continue or report the error up the call-stack. Make Camera::configure() return an error in the event that the Stream* is not valid. This will cause the configure operation to fail and the application will be notified just as other errors in the Camera::configure() operation do. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/camera.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)