You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by danobi <gi...@git.apache.org> on 2015/12/04 18:33:12 UTC
[GitHub] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
GitHub user danobi opened a pull request:
https://github.com/apache/trafficserver/pull/363
TS-4054 Incorrect ink_assert behavior in Diags.cc
`ink_assert(diags_log == NULL)` is incorrect since there are times
when `blf` correctly has the value NULL. In that case, we would
like for the function to do nothing and return early versus crash
ATS via an assert. That is to say, semantically an early return is
more clear than an assert.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/danobi/trafficserver TS-4054
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/363.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 #363
----
commit 0191a4f367d868409b9b4f2935ecfdaa02414615
Author: Daniel Xu <dl...@yahoo.com>
Date: 2015-12-04T17:28:47Z
TS-4054 Incorrect ink_assert behavior in Diags.cc
`ink_assert(diags_log == NULL)` is incorrect since there are times
when `blf` correctly has the value NULL. In that case, we would
like for the function to do nothing and return early versus crash
ATS via an assert. That is to say, semantically an early return is
more clear than an assert.
----
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-162035450
It is marked private. Would your suggestion still be to nest everything inside the `if (blf != NULL)`? I don't see any reason to keep `ink_assert(diags_log == NULL)` since the condition is always true b/c in the constructor we explicitly set `diags_log` to NULL.
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-162033316
Yes. Looking at the code it seems the better choice would be to keep the assert and move it after the `NULL` check on `blf`. I would be tempted to, rather than return there, reverse the check ( `blf != NULL` ) and enclose the rest of the function in that clause, including the assert.
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-162063591
I'm just going to keep the early return for consistency since there's already an early return later.
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-162030676
It looks to me like the purpose of that assertion is to ensure that ```Diags::setup_diagslog``` is not called multiple times. If it was called multiple times, the global ```diags_log``` would be leaked.
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/363
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-163487199
AFAICT the assertion in ```setup_diagslog``` is correct. I would even go so far as to make it an ```ink_release_assert``` because otherwise we would leak the global ```diags_log```. I can see a couple of places where ```diags_log``` is deleted before ```setup_diagslog``` is called.
The best way to fix this IMHO is to not deal with ```drags_log``` at all in ```setup_diagslog```. The pattern that this code should do is:
if (setup_diagslog(blf)) {
delete drags_log;
diags_log = blf;
}
```setup_diagslog``` can be changed from a member function to a local static function, and you can remove all mentions of ```diags_log``` from it.
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-162033850
Is `setup_diagslog` marked `private`? If not, it should be if it's called only from the constructor. Otherwise dropping the assert seems reasonable.
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-164196139
How does that look now?
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-162032775
`setup_diagslog` is only ever called within the `Diags.cc` constructor. If you look at the original change in commit b29149c2, you'll see that I actually made a mistake in transcribing over the if-statement to an assert.
---
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] trafficserver pull request: TS-4054 Incorrect ink_assert behavior ...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-162034159
So, something like
```
if (blf != NULL) {
if (blf->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
// ...
}
diags_log = blf;
// ...
} // end of method
```
---
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.
---