Skip to content

Conversation

@ngara
Copy link
Contributor

@ngara ngara commented Sep 2, 2016

TS-4815 - CID 1267839 dead code in /mgmt/api/CfgContextManager.cc: return TS_ERR_PARAMS;

@zwoop
Copy link
Contributor

zwoop commented Sep 2, 2016

[approve ci]

@zwoop zwoop added this to the 7.0.0 milestone Sep 2, 2016
@zwoop zwoop self-assigned this Sep 2, 2016
@zwoop
Copy link
Contributor

zwoop commented Sep 2, 2016

Should we change this to an ink_relase_assert() or an ink_abort() ?

@atsci
Copy link

atsci commented Sep 2, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/688/ for details.

@atsci
Copy link

atsci commented Sep 2, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/584/ for details.

@shinrich
Copy link
Member

shinrich commented Sep 7, 2016

Yes, it seems like the null check is only dead if the assert always occurred. Otherwise running without debug enabled will not catch the issue and cause a more mysterious crash later.

@ngara ngara force-pushed the TS-4815 branch 2 times, most recently from 7687c89 to 8947c56 Compare September 8, 2016 02:14
@ngara
Copy link
Contributor Author

ngara commented Sep 8, 2016

Thanks for the feedback. Updated to use ink_release_assert which will always execute, and ready to review again.

@ngara ngara closed this Sep 13, 2016
@ngara ngara reopened this Sep 13, 2016
@ngara
Copy link
Contributor Author

ngara commented Sep 13, 2016

This pull request should not be merged in. Handling it with TS-4860 instead.

@atsci
Copy link

atsci commented Sep 13, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/793/ for details.

@atsci
Copy link

atsci commented Sep 13, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/689/ for details.

@zwoop zwoop modified the milestones: 7.1.0, 7.0.0 Sep 15, 2016
@zwoop
Copy link
Contributor

zwoop commented Sep 21, 2016

@ngara Maybe I'm reading this wrong, but it looks like we still have 3 commits in this PR. If that's the case, can you please squash that down into a single commit? No reason to have the 2 previous versions in the git commit history.

@ngara
Copy link
Contributor Author

ngara commented Sep 21, 2016

Alright, I cleaned up my mess. Here's just the single commit.

@zwoop zwoop merged commit 33cd156 into apache:master Oct 27, 2016
JosiahWI pushed a commit to JosiahWI/trafficserver that referenced this pull request Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants