Change HttpSM.cc from using Debug() to new interface, Dbg(). #8809
Conversation
proxy/http/HttpSM.cc
Outdated
| } | ||
|
|
||
| #define SMDebug(tag, fmt, ...) SpecificDebug(debug_on, tag, "[%" PRId64 "] " fmt, sm_id, ##__VA_ARGS__) | ||
| #if defined(Dbg) // Optimized debug available. |
There was a problem hiding this comment.
Use of conditional compilation here means that we could backport this PR to a 9.x release without having to also backport #8581 . Could be useful if we need to backport later PRs that also change HttpSM.cc extensively.
proxy/http/HttpSM.cc
Outdated
|
|
||
| #else | ||
|
|
||
| #define DebugHttpRange(fmt, ...) Debug("htto_range", fmt, ##__VA_ARGS__) |
proxy/http/HttpSM.cc
Outdated
| { \ | ||
| /*ink_assert (magic == HTTP_SM_MAGIC_ALIVE); */ REMEMBER(event, reentrancy_count); \ | ||
| SMDebug("http", "[%s, %s]", #state_name, HttpDebugNames::get_event_name(event)); \ | ||
| SMDebugHttp("[%s, %s]", #state_name, HttpDebugNames::get_event_name(event)); \ |
There was a problem hiding this comment.
It's sad that I can't see the actual tag name, although that might have already been discussed when the new interface was proposed. This one is ok, but "ip-allow" is tricky because it uses "-" where most other tags use "_".
There was a problem hiding this comment.
@SolidWallOfCode did you pick the tag name "ip-allow"? Do you care if I change it to "ip_allow"?
I could also change to a naming pattern where, for example, SMDebug_http_trans (rather than SMDebugHttpTrans) corresponds to the tag "http_trans".
There was a problem hiding this comment.
If all debug tags were valid C++ identifiers, then I wouldn't need the SMDebugXxx() macros. This:
SMDebug("http_trans", "Unable to accelerate range request, fallback to transform");
would instead become this:
SMDebug(http_trans, "Unable to accelerate range request, fallback to transform");
The implementation would use either the concatenation or stringization macro operators:
#if defined(Dbg)
namespace
{
DbgCtl dbgCtl_http_trans;
// ...
}
#define SMDebug(tag, fmt, ...) SpecificDbg(debug_on, dbgCtl_ # tag, "[%" PRId64 "] " fmt, sm_id, ##__VA_ARGS__)
#else
#define SMDebug(tag, fmt, ...) SpecificDbg(debug_on, #tag, "[%" PRId64 "] " fmt, sm_id, ##__VA_ARGS__)
#endif
There was a problem hiding this comment.
If all debug tags were valid C++ identifiers...
I like the concatenation/stringization macro idea. It would be confusing a little since old Debug() requires quotations though, I prefer it rather than other naming patterns.
Also, somebody told me that he prefer to have "Debug" at the end of macro names because that enables you to find (grep) Debug lines easily, when I added macros for HTTP/2 or 3.
|
[approve ci ubuntu] |
|
I marked this for backport to 9.2, since it seems likely there will be later changes to HttpSM.cc that will have to be backported to 9.2. |
|
If we change "ip-allow" to "ip_allow", we should change ones in IPAllow.cc, HttpTransact.cc, and ip_allow.test.py as well.
I understand that would make backporting other changes easy but isn't the change for "ip-allow" technically an incompatible change? I personally don't mind changing debug tag names on a minor release though. In either case, we should probably make the change for "ip-allow" separately on another PR for change logs and release notes. We can discuss about the incompatibility on the PR, and that would tell whether we can backport this to 9.x. |
|
#8830 should be merged prior to merging this PR. |
|
The Au test run finished successfully, even though it is reported as "pending": https://ci.trafficserver.apache.org/job/Github_Builds/job/autest/968/consoleFull |
Oh, interesting. I haven't seen that before. Seems like the jenkins message about the passed autest didn't make it back to github. I'll kick off autest again. |
|
I think we should land this on 10-Dev branch if I understand why the branch exists correctly. |
|
I thought that 10-Dev was for certain select large features. |
|
It isn't limited to large features. Incompatible changes should not be merged on master. I thought we agreed that the ip_allow change is incompatible because you unmarked 9.2. |
|
There would probably be many merge conflicts in HttpSM.cc every time we merged master to 10-Dev. In any case, more urgent work has come up, so I must put this aside for now. |
|
The practical way to do this is to merge it on master right after release 9.2 comes out (and make sure it's the first commit in any hypothetical 9.3). |
|
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community. |
4 participants
No description provided.