Github user dcristoloveanu commented on the pull request:
https://github.com/apache/qpid-proton/pull/39#issuecomment-116826581
Hi Andrew,
Thanks for the review, it's much appreciated.
There are several good points here that deserve a more in depth discussion,
so I will attempt to address them one by one:
1. "Using LINE as an error is not acceptable"
I typically use __LINE__ as return code when it does not really matter why
the function failed, and the caller does not really need to check which error
case what hit. I can understand that's not the case for Proton, and I can
change this to using an existing error code. I simply underestimated that this
is a big deal, so I'll switch to your suggestion.
2. "The Proton code does not subscribe to the practice of forcing a single
return to a routine."
I can understand the choice, and I shall respect that, living by the "when
in Rome" rule.
On the other hand, as an FYI, MISRA for example has an advisory rule (15.5)
that states "A function should have a single point of exit at the end". One of
the rationale is that if a function has exit points interspersed with
statements that produce persistent side effects, it is not easy to determine
which side effects will occur when the function is executed.
Anyhow, this is like I said just an FYI remark, I do suspect that you've
already considered the plus/minuses for single vs. multiple return point and
this can quickly turn into a not so productive discussion.
3. "On the whole I feel that this kind of error situation should be dealt
with using an exception like mechanism. "
On this one I respectfully disagree. I do not think it is for Proton to
decide whether the system should be brought down or not. It is at system level
that such a decision is made. For example one could have on an embedded device
a special memory management implementation that would decide that if malloc
fails a reboot function should be invoked (or any other action taken). But
Proton has no business of taking such a decision for an application, since
Proton is merely a protocol library. Should for example openssl crash the
system if it cannot allocate a block? I believe not, since it's not openssl's
responsibility either to decide that.
So it follows that the ideal action for a library like openssl, proton,
etc. in case an error happened is to report the errors to the user in a way
that the user can act on them, without leaving the library in an unusable state
for the next operation, if a next operation would be possible.
An exception-like mechanism would require the state to be unwound, which is
OK in other languages, but C's longjmp is not really good for that. So the only
solution that I believe is left is to report the errors as return codes until
the point they are surfaces to the consuming application (as return codes or
tracker status), then the application which can decide for itself whether to
bring down the system, print an error or set the building on fire :-).
I don't think ignoring the errors is acceptable if Proton is to considered
for usage in many embedded environments (like automotive, industrial, etc. -
MISRA is a requirement in many such cases), that's why I submitted this PR.
I'll wait for your thoughts before submitting an update to the PR.
Also, as a side point, I could go and fix this throughout the code base,
but I fear there would be a lot of changes, thus I opted for fixing things one
by one so it gets a little better with each PR. If you think we should do this
in a different way, let me know, any guidance is appreciated.
Thanks,
/Dan
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
