| Message ID | 20221016210202.1107-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, thanks for the summary On Mon, Oct 17, 2022 at 12:02:00AM +0300, Laurent Pinchart via libcamera-devel wrote: > Hello, > > This small patch series stems from my review of "[PATCH 13/14] > libcamera: pipeline: rkisp1: Factorize errors handling path on start()". > While I agreed that the current error handling pattern in > PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage > of goto statements to fix it. This is an alternative proposal. > > The ScopeExitActions class introduced in patch 1/2 took multiple forms > before reaching this one. I first experimented with a class that handled > a single action and thus needed to be instantiated once per action. To > achieve the equivalent of the ScopeExitActions::release() call I had to > either add an 'if (ret)' statement to the lambda functions, or pass the > ret variable to the constructor of the ScopeExitActions. I found ideas > for syntactic sugar in https://github.com/yuri-kilochek/boost.scope_guard, > but in the end, neither option made me very happy. > > Handling multiple actions with one ScopeExitActions instance makes the > user code (in 2/2) simpler. The main drawback is the use of std::vector > to store the actions, which requires dynamic allocation (and > reallocation). I tried to find a different solution. One interesting > approach used a ScopeExitGroup class instantiated at the beginning of > the function, and a ScopeExit class that handled a single action, > instantiated multiple times. The ScopeExit class was constructed with a > pointer to the ScopeExitGroup, and only called the action in its > destructor if the ScopeExitGroup hadn't been release()d. Unfortunately > this didn't work well for patch 2/2, as the > PipelineHandlerRkISP1::start() function needs to register an action in a > sub-scope (within a if (...) { ... }). The corresponding ScopeExit > instance thus got destroyed when exiting the sub-scope, not at the end > of the function. > > Another completely different approach to error handling would be to make > all the cleanup functions in PipelineHandlerRkISP1 idempotent, safe to > be called when the corresponding operation hasn't been performed (e.g. > streamOff() without a previous streamOn()). This idiom may result to > better error handling through libcamera. If anyone thinks it could be > better than a ScopeExitActions class, that would be enough to convince > me to give it a try and burry ScopeExitActions. > That would a solution limited to the RkISP1 pipeline handler, it won't solve the issue generically as this class might > In v1 of the series the class had a defer() function (instead of an > operator+=()), whose name was influenced by an interesting C extension > proposal (https://hal.inria.fr/hal-03059076/document). It uses the name > "guard" for the conceptual equivalent of the ScopeExitActions class, but > that didn't convince me. > > I still wish the release() call wouldn't be needed, but I'm not as > unhappy with it than in v1. This is error-prone as it could be > forgotten, but the other options I can think of are worse; > > - Switch to the opposite, with cleanups disabled by default and an > explicit call in the error paths. That could also be forgotten, > resulting in missing cleanups in some error paths. The result would > likely be worse, as error paths are less tested. > > - Require explicit calls in both the success and error paths, and > complain loudly in the destructor if none of those functions have been > called. That's equally error-prone due to undertesting of error paths. However, this would help catch errors that might not be trivial to debug. I'm not sure it's worth the extra annoyance of calling an explicit action in error paths though. It would help detecting missing calls to "cleanup()" but won't help validate that the correct cleanup-actions have been added, something for which we shall rely on reviews anyway.. > > - Tying the explicit calls with the return statements, for instance by > making the user return a Result type (any rust developer here ?), and > adding two functions to ScopeExitActions, success and error, that > return a Result. While the Result idiom may be interesting, it would > be a much broader change. I'm just a little annoyed by the dynamic allocation and the usage of a plain vector. I presume you have considered the even less nice alternative to pre-allocating a fixed number of cleanup actions. Or, we can possibily .reserve() the vector at construction time, assuming a reasonable number of cleanup actions. > > Laurent Pinchart (2): > libcamera: utils: Add ScopeExitActions class > pipeline: rkisp1: Use ScopeExitActions to simplify error handling in > start > > include/libcamera/base/utils.h | 13 +++ > src/libcamera/base/utils.cpp | 132 +++++++++++++++++++++++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++---- > 3 files changed, 156 insertions(+), 20 deletions(-) > > > base-commit: f4201e9636f4460ad0eacdf32aac43c70c6bf95c > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Mon, Oct 17, 2022 at 10:31:56AM +0200, Jacopo Mondi wrote: > Hi Laurent, > thanks for the summary > > On Mon, Oct 17, 2022 at 12:02:00AM +0300, Laurent Pinchart via libcamera-devel wrote: > > Hello, > > > > This small patch series stems from my review of "[PATCH 13/14] > > libcamera: pipeline: rkisp1: Factorize errors handling path on start()". > > While I agreed that the current error handling pattern in > > PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage > > of goto statements to fix it. This is an alternative proposal. > > > > The ScopeExitActions class introduced in patch 1/2 took multiple forms > > before reaching this one. I first experimented with a class that handled > > a single action and thus needed to be instantiated once per action. To > > achieve the equivalent of the ScopeExitActions::release() call I had to > > either add an 'if (ret)' statement to the lambda functions, or pass the > > ret variable to the constructor of the ScopeExitActions. I found ideas > > for syntactic sugar in https://github.com/yuri-kilochek/boost.scope_guard, > > but in the end, neither option made me very happy. > > > > Handling multiple actions with one ScopeExitActions instance makes the > > user code (in 2/2) simpler. The main drawback is the use of std::vector > > to store the actions, which requires dynamic allocation (and > > reallocation). I tried to find a different solution. One interesting > > approach used a ScopeExitGroup class instantiated at the beginning of > > the function, and a ScopeExit class that handled a single action, > > instantiated multiple times. The ScopeExit class was constructed with a > > pointer to the ScopeExitGroup, and only called the action in its > > destructor if the ScopeExitGroup hadn't been release()d. Unfortunately > > this didn't work well for patch 2/2, as the > > PipelineHandlerRkISP1::start() function needs to register an action in a > > sub-scope (within a if (...) { ... }). The corresponding ScopeExit > > instance thus got destroyed when exiting the sub-scope, not at the end > > of the function. > > > > Another completely different approach to error handling would be to make > > all the cleanup functions in PipelineHandlerRkISP1 idempotent, safe to > > be called when the corresponding operation hasn't been performed (e.g. > > streamOff() without a previous streamOn()). This idiom may result to > > better error handling through libcamera. If anyone thinks it could be > > better than a ScopeExitActions class, that would be enough to convince > > me to give it a try and burry ScopeExitActions. > > That would a solution limited to the RkISP1 pipeline handler, it won't > solve the issue generically as this class might That's partly right. If you look at the Raspberry Pi or IPU3 pipeline handlers, they assume some sort of idempotency for the "stop" operations in their start() implementation. That assumption is not always correct, and I'm quite sure some of the corresponding error paths in start() are thus broken. We just haven't noticed it, as they have never been exercised (we'll need error injection at some point). I have found very little use for the ScopeExitActions class in other parts of the code. There's one similar construct in thread.cpp that would be better off with a ScopeExit (single action, constructed on the stack, no dynamic allocation) instead of ScopeExitActions. There may be some other places that have complex error handling, but I haven't seen any after a casual look. > > In v1 of the series the class had a defer() function (instead of an > > operator+=()), whose name was influenced by an interesting C extension > > proposal (https://hal.inria.fr/hal-03059076/document). It uses the name > > "guard" for the conceptual equivalent of the ScopeExitActions class, but > > that didn't convince me. > > > > I still wish the release() call wouldn't be needed, but I'm not as > > unhappy with it than in v1. This is error-prone as it could be > > forgotten, but the other options I can think of are worse; > > > > - Switch to the opposite, with cleanups disabled by default and an > > explicit call in the error paths. That could also be forgotten, > > resulting in missing cleanups in some error paths. The result would > > likely be worse, as error paths are less tested. > > > > - Require explicit calls in both the success and error paths, and > > complain loudly in the destructor if none of those functions have been > > called. That's equally error-prone due to undertesting of error paths. > > However, this would help catch errors that might not be trivial to > debug. How so ? There will be no compile-time error that something is missing, we would only get a runtime error, and only when the corresponding error path is followed, which is very rarely, if ever. > I'm not sure it's worth the extra annoyance of calling an > explicit action in error paths though. It would help detecting missing > calls to "cleanup()" but won't help validate that the correct cleanup-actions > have been added, something for which we shall rely on reviews anyway.. > > > - Tying the explicit calls with the return statements, for instance by > > making the user return a Result type (any rust developer here ?), and > > adding two functions to ScopeExitActions, success and error, that > > return a Result. While the Result idiom may be interesting, it would > > be a much broader change. > > I'm just a little annoyed by the dynamic allocation and the usage of a > plain vector. I presume you have considered the even less nice > alternative to pre-allocating a fixed number of cleanup actions. Yes, and I concluded we would get it wrong. I'd rather not introducing a new type of possible bugs with this helper class that is meant to reduce the number of bugs :-) > Or, we can possibily .reserve() the vector at construction time, assuming > a reasonable number of cleanup actions. That's quite a bit of a hack for such a generic helper. And there would still be one dynamic allocation. > > Laurent Pinchart (2): > > libcamera: utils: Add ScopeExitActions class > > pipeline: rkisp1: Use ScopeExitActions to simplify error handling in > > start > > > > include/libcamera/base/utils.h | 13 +++ > > src/libcamera/base/utils.cpp | 132 +++++++++++++++++++++++ > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++---- > > 3 files changed, 156 insertions(+), 20 deletions(-) > > > > > > base-commit: f4201e9636f4460ad0eacdf32aac43c70c6bf95c
Hello, This small patch series stems from my review of "[PATCH 13/14] libcamera: pipeline: rkisp1: Factorize errors handling path on start()". While I agreed that the current error handling pattern in PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage of goto statements to fix it. This is an alternative proposal. The ScopeExitActions class introduced in patch 1/2 took multiple forms before reaching this one. I first experimented with a class that handled a single action and thus needed to be instantiated once per action. To achieve the equivalent of the ScopeExitActions::release() call I had to either add an 'if (ret)' statement to the lambda functions, or pass the ret variable to the constructor of the ScopeExitActions. I found ideas for syntactic sugar in https://github.com/yuri-kilochek/boost.scope_guard, but in the end, neither option made me very happy. Handling multiple actions with one ScopeExitActions instance makes the user code (in 2/2) simpler. The main drawback is the use of std::vector to store the actions, which requires dynamic allocation (and reallocation). I tried to find a different solution. One interesting approach used a ScopeExitGroup class instantiated at the beginning of the function, and a ScopeExit class that handled a single action, instantiated multiple times. The ScopeExit class was constructed with a pointer to the ScopeExitGroup, and only called the action in its destructor if the ScopeExitGroup hadn't been release()d. Unfortunately this didn't work well for patch 2/2, as the PipelineHandlerRkISP1::start() function needs to register an action in a sub-scope (within a if (...) { ... }). The corresponding ScopeExit instance thus got destroyed when exiting the sub-scope, not at the end of the function. Another completely different approach to error handling would be to make all the cleanup functions in PipelineHandlerRkISP1 idempotent, safe to be called when the corresponding operation hasn't been performed (e.g. streamOff() without a previous streamOn()). This idiom may result to better error handling through libcamera. If anyone thinks it could be better than a ScopeExitActions class, that would be enough to convince me to give it a try and burry ScopeExitActions. In v1 of the series the class had a defer() function (instead of an operator+=()), whose name was influenced by an interesting C extension proposal (https://hal.inria.fr/hal-03059076/document). It uses the name "guard" for the conceptual equivalent of the ScopeExitActions class, but that didn't convince me. I still wish the release() call wouldn't be needed, but I'm not as unhappy with it than in v1. This is error-prone as it could be forgotten, but the other options I can think of are worse; - Switch to the opposite, with cleanups disabled by default and an explicit call in the error paths. That could also be forgotten, resulting in missing cleanups in some error paths. The result would likely be worse, as error paths are less tested. - Require explicit calls in both the success and error paths, and complain loudly in the destructor if none of those functions have been called. That's equally error-prone due to undertesting of error paths. - Tying the explicit calls with the return statements, for instance by making the user return a Result type (any rust developer here ?), and adding two functions to ScopeExitActions, success and error, that return a Result. While the Result idiom may be interesting, it would be a much broader change. Laurent Pinchart (2): libcamera: utils: Add ScopeExitActions class pipeline: rkisp1: Use ScopeExitActions to simplify error handling in start include/libcamera/base/utils.h | 13 +++ src/libcamera/base/utils.cpp | 132 +++++++++++++++++++++++ src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++---- 3 files changed, 156 insertions(+), 20 deletions(-) base-commit: f4201e9636f4460ad0eacdf32aac43c70c6bf95c