You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by dcristoloveanu <gi...@git.apache.org> on 2015/06/20 01:14:59 UTC
[GitHub] qpid-proton pull request: Fix 2 Code Analysis warnings and a reall...
GitHub user dcristoloveanu opened a pull request:
https://github.com/apache/qpid-proton/pull/39
Fix 2 Code Analysis warnings and a realloc leak/result not checked
-Fix 2 Code Analysis warnings in iocp.c
-Fix one realloc leak and the fact that the realloc result was not checked. This rippled through codec.c as more functions needed to have error checking. Also, more fixes regarding return values being checked need to be done throughout message.c, transport.c, etc., but the amount of changes is significant, so it needs to be done in several chunks.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dcristoloveanu/qpid-proton FixCodeAnalysis
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-proton/pull/39.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #39
----
commit 7d7ac1be633cc8cfd3a69cbf36112a97a9b836e2
Author: dcristoloveanu <dc...@microsoft.com>
Date: 2015-06-19T22:46:09Z
-Fix 2 Code Analysis warnings in iocp.c
-Fix one realloc leak and the fact that the realloc result was not checked. This rippled through codec.c as more functions needed to have error checking. Also, more fixes regarding return values being checked need to be done throughout message.c, transport.c, etc., but the amount of changes is significant, so it needs to be done in several chunks.
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: Fix 2 Code Analysis warnings and a reall...
Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on the pull request:
https://github.com/apache/qpid-proton/pull/39#issuecomment-119227983
@dcristoloveanu - Sorry not to reply to your comments - for some reason I got no email to tell me you had commented.
* If you change the PR I'll be happy to merge it.
* I was peripherally aware that some coding standards require single return points. And if you'd pressed me I might have guessed the MISRA standard. I do agree with the rationale, however I think it is just guidance, and as professionals we are supposed to use our best judgement. In this and similar cases I think that the single return actually makes the code harder to read and therefore maintain. So even in coding to this standard I'd argue that in these cases early exit is better.
* I think you may be misinterpreting the idea of exception like semantics. I'm not suggesting that the entire program abort (I'm not sure why exception would indicate that). In all the language that I'm familiar with exception indicates a non-local flow of control which avoids having to deal with bad consequences in a place that probably doesn't have the global knowledge to do this. Think of C++, Java, Python, C#, F#, etc. exceptions.
However it is most likely true that if you run out of memory in the context of Proton then the current connection can not continue.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: Fix 2 Code Analysis warnings and a reall...
Posted by dcristoloveanu <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: Fix 2 Code Analysis warnings and a reall...
Posted by dcristoloveanu <gi...@git.apache.org>.
Github user dcristoloveanu closed the pull request at:
https://github.com/apache/qpid-proton/pull/39
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: Fix 2 Code Analysis warnings and a reall...
Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on the pull request:
https://github.com/apache/qpid-proton/pull/39#issuecomment-119228335
@dcristoloveanu actually given that a corrected change would be entirely different from this change I'd actually prefer a new branch/PR to merge, which would make the history neater.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: Fix 2 Code Analysis warnings and a reall...
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on the pull request:
https://github.com/apache/qpid-proton/pull/39#issuecomment-118902728
I'd like to see this pull request move forward. On Dan's points above:
1) I agree with Andrew that using __LINE__ as an error code should be changed to simply returning an eror code (or a null result, whichever is appropriate).
2) I agree with Andrew here as well. I think it's an acceptable pattern to check inputs and exit early if there is an error condition.
3) I agree with Dan that the library should simply report the failure to allocate and not take any action on behalf of the process using it. There are a number of cases where Proton makes relatively large allocations the failure of which does not necessarily mean that the process can't recover.
Taking a piecemeal approach to cleaning up all the ignored allocations is probably a good idea. If there are any API changes that will result, we should try to get those all into a single release.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: Fix 2 Code Analysis warnings and a reall...
Posted by dcristoloveanu <gi...@git.apache.org>.
Github user dcristoloveanu commented on the pull request:
https://github.com/apache/qpid-proton/pull/39#issuecomment-114940533
Ping ...
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---