You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "zwoop (via GitHub)" <gi...@apache.org> on 2025/06/17 21:37:45 UTC
[PR] HRW: Add support for elif, in if-elif-else [trafficserver]
zwoop opened a new pull request, #12304:
URL: https://github.com/apache/trafficserver/pull/12304
This adds an elif clause to the conditions. E.g.
```
cond %{SEND_RESPONSE_HDR_HOOK} [AND]
cond %{CLIENT-HEADER:X-Foo} ="foo"
set-header X-Resp "foo"
elif
cond %{CLIENT-HEADER:X-Foo} ="bar"
set-header X-Resp "bar"
elif
cond %{CLIENT-HEADER:X-Foo} ="baz"
set-header X-Resp "baz"
else
set-header X-Resp "else"
```
In addition, this cleans up the handling of the existing "else" clause, and I ran clang-tidy over the two files I was editing.
Co-author: Miles L.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2160417343
##########
plugins/header_rewrite/ruleset.h:
##########
@@ -39,23 +39,44 @@ class RuleSet
{
public:
// Holding the IF and ELSE operators and mods, in two separate linked lists.
- struct OperatorPair {
- OperatorPair() = default;
+ struct OperatorAndMods {
+ OperatorAndMods() = default;
- OperatorPair(const OperatorPair &) = delete;
- OperatorPair &operator=(const OperatorPair &) = delete;
+ OperatorAndMods(const OperatorAndMods &) = delete;
+ OperatorAndMods &operator=(const OperatorAndMods &) = delete;
Operator *oper = nullptr;
OperModifiers oper_mods = OPER_NONE;
};
+ struct CondOpSection {
+ CondOpSection() = default;
+
+ ~CondOpSection()
+ {
+ delete ops.oper;
+ delete next;
Review Comment:
Consider potential risks of recursive deletion in the ~CondOpSection destructor. If many 'elif' sections are present, this recursive deletion might lead to a stack overflow in extreme cases; consider documenting the expected upper bounds or refactoring to an iterative deletion mechanism.
```suggestion
CondOpSection *current = this->next;
while (current) {
CondOpSection *next_section = current->next;
current->next = nullptr; // Avoid dangling pointers
delete current;
current = next_section;
}
```
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -115,28 +115,24 @@ class RulesConfig
bool parse_config(const std::string &fname, TSHttpHookID default_hook, char *from_url = nullptr, char *to_url = nullptr);
private:
- bool add_rule(RuleSet *rule);
+ void add_rule(std::unique_ptr<RuleSet> rule);
TSCont _cont;
RuleSet *_rules[TS_HTTP_LAST_HOOK + 1];
ResourceIDs _resids[TS_HTTP_LAST_HOOK + 1];
};
-// Helper function to add a rule to the rulesets
-bool
-RulesConfig::add_rule(RuleSet *rule)
+void
+RulesConfig::add_rule(std::unique_ptr<RuleSet> rule)
{
- if (rule && rule->has_operator()) {
- Dbg(dbg_ctl, " Adding rule to hook=%s", TSHttpHookNameLookup(rule->get_hook()));
- if (nullptr == _rules[rule->get_hook()]) {
- _rules[rule->get_hook()] = rule;
- } else {
- _rules[rule->get_hook()]->append(rule);
- }
- return true;
- }
+ auto raw = rule.release();
- return false;
+ Dbg(pi_dbg_ctl, " Adding rule to hook=%s", TSHttpHookNameLookup(raw->get_hook()));
+ if (nullptr == _rules[raw->get_hook()]) {
+ _rules[raw->get_hook()] = raw;
+ } else {
+ _rules[raw->get_hook()]->append(raw);
Review Comment:
Transferring ownership from a unique_ptr to a raw pointer via release() can be error-prone. Consider using smart pointers consistently for the _rules array to improve memory safety and ease maintenance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2164280885
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -304,8 +325,8 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
}
// Add the last rule (possibly the only rule)
- if (add_rule(rule.get())) {
- rule.release();
+ if (rule && rule->has_operator()) {
+ add_rule(std::move(rule)); // Transfer ownership
Review Comment:
I actually still see the commet in the github display of the patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#issuecomment-2998124620
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop merged PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2153304495
##########
doc/admin-guide/plugins/header_rewrite.en.rst:
##########
@@ -142,6 +142,23 @@ with the word ``else``. The following example illustrates this::
The ``else`` clause is not a condition, and does not take any flags, it is
of course optional, but when specified must be followed by at least one operator.
+You can also do an ``elif`` (else if) clause, which is specified by starting a new line
+with the word ``elif``. The following example illustrates this::
+
+ cond %{STATUS} >399 [AND]
+ cond %{STATUS} <500
+ set-status 404
+ elif
+ cond %{STATUS} =503
+ set-status 502
+ else
+ set-status 503
+
+Keep in mind that nesting the ``else`` and ``elif`` clauses is not allowed, but any
+number of ``elif`` clauses can be specified. We can consider these clauses are more
+powerful and flexible ``switch`` statement. In an ``if-elif-else`` rule, only one
+will be evaluate its operators.
Review Comment:
Added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "mlibbey (via GitHub)" <gi...@apache.org>.
mlibbey commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2153232893
##########
doc/admin-guide/plugins/header_rewrite.en.rst:
##########
@@ -142,6 +142,23 @@ with the word ``else``. The following example illustrates this::
The ``else`` clause is not a condition, and does not take any flags, it is
of course optional, but when specified must be followed by at least one operator.
+You can also do an ``elif`` (else if) clause, which is specified by starting a new line
+with the word ``elif``. The following example illustrates this::
+
+ cond %{STATUS} >399 [AND]
+ cond %{STATUS} <500
+ set-status 404
+ elif
+ cond %{STATUS} =503
+ set-status 502
+ else
+ set-status 503
+
+Keep in mind that nesting the ``else`` and ``elif`` clauses is not allowed, but any
+number of ``elif`` clauses can be specified. We can consider these clauses are more
+powerful and flexible ``switch`` statement. In an ``if-elif-else`` rule, only one
+will be evaluate its operators.
Review Comment:
How about adding...
```
Similarly, each ``else`` and ``elif`` have the same implied :ref:`Hook Condition <Hook Conditions>` as the initial condition. ```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "mlibbey (via GitHub)" <gi...@apache.org>.
mlibbey commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2153232893
##########
doc/admin-guide/plugins/header_rewrite.en.rst:
##########
@@ -142,6 +142,23 @@ with the word ``else``. The following example illustrates this::
The ``else`` clause is not a condition, and does not take any flags, it is
of course optional, but when specified must be followed by at least one operator.
+You can also do an ``elif`` (else if) clause, which is specified by starting a new line
+with the word ``elif``. The following example illustrates this::
+
+ cond %{STATUS} >399 [AND]
+ cond %{STATUS} <500
+ set-status 404
+ elif
+ cond %{STATUS} =503
+ set-status 502
+ else
+ set-status 503
+
+Keep in mind that nesting the ``else`` and ``elif`` clauses is not allowed, but any
+number of ``elif`` clauses can be specified. We can consider these clauses are more
+powerful and flexible ``switch`` statement. In an ``if-elif-else`` rule, only one
+will be evaluate its operators.
Review Comment:
How about adding...
Similarly, each ``else`` and ``elif`` have the same implied :ref:`Hook Condition <Hook Conditions>` as the initial condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#issuecomment-2984391165
[approve ci osx]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2161926795
##########
plugins/header_rewrite/ruleset.h:
##########
@@ -39,23 +39,44 @@ class RuleSet
{
public:
// Holding the IF and ELSE operators and mods, in two separate linked lists.
- struct OperatorPair {
- OperatorPair() = default;
+ struct OperatorAndMods {
+ OperatorAndMods() = default;
- OperatorPair(const OperatorPair &) = delete;
- OperatorPair &operator=(const OperatorPair &) = delete;
+ OperatorAndMods(const OperatorAndMods &) = delete;
+ OperatorAndMods &operator=(const OperatorAndMods &) = delete;
Operator *oper = nullptr;
OperModifiers oper_mods = OPER_NONE;
};
+ struct CondOpSection {
+ CondOpSection() = default;
+
+ ~CondOpSection()
+ {
+ delete ops.oper;
+ delete next;
Review Comment:
This is a non issue IMO, if you have this many elif's, you're just wrong :).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#issuecomment-2997643093
[approve ci autest 0]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2153221865
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -206,15 +202,43 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
continue;
}
- // If we are at the beginning of a new condition, save away the previous rule (but only if it has operators).
- if (p.is_cond() && add_rule(rule.get())) {
- rule.release();
+ TSHttpHookID hook = default_hook;
+ bool is_hook = p.cond_is_hook(hook);
+
+ // Deal with the elsif / else special keywords, these are neither conditions nor operators.
Review Comment:
`elsif` -> `elif`
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -304,8 +325,8 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
}
// Add the last rule (possibly the only rule)
- if (add_rule(rule.get())) {
- rule.release();
+ if (rule && rule->has_operator()) {
+ add_rule(std::move(rule)); // Transfer ownership
Review Comment:
No need for `// Transfer ownership` comment, that's clear from `std::move` on a `unique_ptr`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2153304669
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -304,8 +325,8 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
}
// Add the last rule (possibly the only rule)
- if (add_rule(rule.get())) {
- rule.release();
+ if (rule && rule->has_operator()) {
+ add_rule(std::move(rule)); // Transfer ownership
Review Comment:
Fixed
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -206,15 +202,43 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
continue;
}
- // If we are at the beginning of a new condition, save away the previous rule (but only if it has operators).
- if (p.is_cond() && add_rule(rule.get())) {
- rule.release();
+ TSHttpHookID hook = default_hook;
+ bool is_hook = p.cond_is_hook(hook);
+
+ // Deal with the elsif / else special keywords, these are neither conditions nor operators.
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Add support for elif, in if-elif-else [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2162170838
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -115,28 +115,24 @@ class RulesConfig
bool parse_config(const std::string &fname, TSHttpHookID default_hook, char *from_url = nullptr, char *to_url = nullptr);
private:
- bool add_rule(RuleSet *rule);
+ void add_rule(std::unique_ptr<RuleSet> rule);
TSCont _cont;
RuleSet *_rules[TS_HTTP_LAST_HOOK + 1];
ResourceIDs _resids[TS_HTTP_LAST_HOOK + 1];
};
-// Helper function to add a rule to the rulesets
-bool
-RulesConfig::add_rule(RuleSet *rule)
+void
+RulesConfig::add_rule(std::unique_ptr<RuleSet> rule)
{
- if (rule && rule->has_operator()) {
- Dbg(dbg_ctl, " Adding rule to hook=%s", TSHttpHookNameLookup(rule->get_hook()));
- if (nullptr == _rules[rule->get_hook()]) {
- _rules[rule->get_hook()] = rule;
- } else {
- _rules[rule->get_hook()]->append(rule);
- }
- return true;
- }
+ auto raw = rule.release();
- return false;
+ Dbg(pi_dbg_ctl, " Adding rule to hook=%s", TSHttpHookNameLookup(raw->get_hook()));
+ if (nullptr == _rules[raw->get_hook()]) {
+ _rules[raw->get_hook()] = raw;
+ } else {
+ _rules[raw->get_hook()]->append(raw);
Review Comment:
Changed to use std::unique_ptr consistently around the RuleSet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org