Avoid leaking init options in context. #198
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
| return RMW_RET_OK; | ||
| return ret; | ||
| fail: | ||
| if (RMW_RET_OK != rmw_init_options_fini(&context->options)) { |
There was a problem hiding this comment.
Is it safe to call fini if init failed? Just checking ...
There was a problem hiding this comment.
good point, I think it's not.
@hidmic maybe just returning an error in line 1132.
There was a problem hiding this comment.
Well, arguably it should not and both rmw_init_options_init() or rmw_init_options_copy() should guarantee output options remain uninitialized. Reality is that such guarantee is not documented in rmw API and most implementations do not really check. rmw_cyclonedds_cpp isn't the exception. Thus the extra fini, which judging by the code isn't unsafe (though again, not necessarily correct nor ideal).
Alternatively, we can establish that guarantee and change implementations to abide to it. Thoughts?
There was a problem hiding this comment.
Alternatively, we can establish that guarantee and change implementations to abide to it. Thoughts?
👍
If I call *_init() or *_copy() and the function fails, I expect it to cleanup all the resources it created (and not having to do it manually).
In the case of *_copy(src, dst), I also expect the function to no modify *dst if it failed to do the copy (if this cannot be guaranteed, it should be documented).
It's the more usual pattern in C code IMO.
|
This has been superseded by #202. |
Spin off from #196.