Copilot commented on code in PR #12593:
URL: https://github.com/apache/trafficserver/pull/12593#discussion_r3660355654
##########
plugins/header_rewrite/operators.cc:
##########
@@ -1645,45 +1645,48 @@ void
OperatorSetNextHopStrategy::initialize(Parser &p)
{
Operator::initialize(p);
+ _stratname = p.get_arg();
- _value.set_value(p.get_arg(), this);
- Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy::initialie: %s",
_value.get_value().c_str());
+ if (_stratname.empty() || "null" == _stratname) {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy() 'clear'");
+ } else {
+ _strategy = TSRemapNextHopStrategyFind(_stratname.c_str());
+ if (nullptr == _strategy) {
+ TSError("[%s] Failed to get strategy '%s'", PLUGIN_NAME,
_stratname.c_str());
+ _apply = false;
+ } else {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy() '%s'", _stratname.c_str());
+ }
+ }
}
void
OperatorSetNextHopStrategy::initialize_hooks()
{
- add_allowed_hook(TS_HTTP_READ_REQUEST_HDR_HOOK);
add_allowed_hook(TS_REMAP_PSEUDO_HOOK);
}
bool
OperatorSetNextHopStrategy::exec(const Resources &res) const
{
- if (!res.state.txnp) {
- TSError("[%s] OperatorSetNextHopStrategy() failed. Transaction is null",
PLUGIN_NAME);
+ if (!_apply) {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy::exec: do nothing");
+ return true;
}
-
auto const txnp = res.state.txnp;
Review Comment:
This change removes per-transaction evaluation of the operator argument
(previously via `Value` expansion) and replaces it with a single, init-time
lookup. If `set-next-hop-strategy` previously supported variables/expansions
(e.g., based on headers, captures, or conditions), this is a behavior-breaking
change for existing configs. Suggested fix: preserve dynamic behavior by
keeping the `Value` field and, at exec time, resolving the computed name via
`TSHttpTxnNextHopStrategyFind(txnp, name)`; optionally keep the current
fast-path by pre-resolving only when the argument is a literal constant (no
expansions).
##########
plugins/header_rewrite/operators.cc:
##########
@@ -1645,45 +1645,48 @@ void
OperatorSetNextHopStrategy::initialize(Parser &p)
{
Operator::initialize(p);
+ _stratname = p.get_arg();
- _value.set_value(p.get_arg(), this);
- Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy::initialie: %s",
_value.get_value().c_str());
+ if (_stratname.empty() || "null" == _stratname) {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy() 'clear'");
+ } else {
+ _strategy = TSRemapNextHopStrategyFind(_stratname.c_str());
+ if (nullptr == _strategy) {
+ TSError("[%s] Failed to get strategy '%s'", PLUGIN_NAME,
_stratname.c_str());
+ _apply = false;
+ } else {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy() '%s'", _stratname.c_str());
+ }
+ }
}
Review Comment:
This change removes per-transaction evaluation of the operator argument
(previously via `Value` expansion) and replaces it with a single, init-time
lookup. If `set-next-hop-strategy` previously supported variables/expansions
(e.g., based on headers, captures, or conditions), this is a behavior-breaking
change for existing configs. Suggested fix: preserve dynamic behavior by
keeping the `Value` field and, at exec time, resolving the computed name via
`TSHttpTxnNextHopStrategyFind(txnp, name)`; optionally keep the current
fast-path by pre-resolving only when the argument is a literal constant (no
expansions).
##########
plugins/header_rewrite/operators.cc:
##########
@@ -1645,45 +1645,48 @@ void
OperatorSetNextHopStrategy::initialize(Parser &p)
{
Operator::initialize(p);
+ _stratname = p.get_arg();
- _value.set_value(p.get_arg(), this);
- Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy::initialie: %s",
_value.get_value().c_str());
+ if (_stratname.empty() || "null" == _stratname) {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy() 'clear'");
+ } else {
+ _strategy = TSRemapNextHopStrategyFind(_stratname.c_str());
+ if (nullptr == _strategy) {
+ TSError("[%s] Failed to get strategy '%s'", PLUGIN_NAME,
_stratname.c_str());
+ _apply = false;
+ } else {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy() '%s'", _stratname.c_str());
+ }
+ }
}
void
OperatorSetNextHopStrategy::initialize_hooks()
{
- add_allowed_hook(TS_HTTP_READ_REQUEST_HDR_HOOK);
add_allowed_hook(TS_REMAP_PSEUDO_HOOK);
}
bool
OperatorSetNextHopStrategy::exec(const Resources &res) const
{
- if (!res.state.txnp) {
- TSError("[%s] OperatorSetNextHopStrategy() failed. Transaction is null",
PLUGIN_NAME);
+ if (!_apply) {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy::exec: do nothing");
+ return true;
}
-
auto const txnp = res.state.txnp;
-
- std::string value;
- _value.append_value(value, res);
-
- // Setting an empty strategy clears it for either parent.config or remap to
- if ("null" == value || value.empty()) {
- Dbg(pi_dbg_ctl, "Clearing strategy");
- TSHttpTxnNextHopStrategySet(txnp, nullptr);
- return true;
+ if (!txnp) {
+ TSError("[%s] OperatorSetNextHopStrategy() failed. Transaction is null",
PLUGIN_NAME);
+ return false;
}
- void const *const stratptr = TSHttpTxnNextHopNamedStrategyGet(txnp,
value.c_str());
- if (nullptr == stratptr) {
- TSWarning("[%s] Failed to get strategy '%s'", PLUGIN_NAME, value.c_str());
+ if (nullptr == _strategy) {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy::exec: Clearing strategy");
} else {
- Dbg(pi_dbg_ctl, " Setting strategy '%s'", value.c_str());
- TSHttpTxnNextHopStrategySet(txnp, stratptr);
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy::exec: Setting strategy to
'%s'", _stratname.c_str());
}
+ TSHttpTxnNextHopStrategySet(txnp, _strategy);
+
return true;
}
Review Comment:
This change removes per-transaction evaluation of the operator argument
(previously via `Value` expansion) and replaces it with a single, init-time
lookup. If `set-next-hop-strategy` previously supported variables/expansions
(e.g., based on headers, captures, or conditions), this is a behavior-breaking
change for existing configs. Suggested fix: preserve dynamic behavior by
keeping the `Value` field and, at exec time, resolving the computed name via
`TSHttpTxnNextHopStrategyFind(txnp, name)`; optionally keep the current
fast-path by pre-resolving only when the argument is a literal constant (no
expansions).
##########
doc/developer-guide/api/functions/TSRemapNextHopStrategySet.en.rst:
##########
@@ -0,0 +1,56 @@
+.. Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed
+ with this work for additional information regarding copyright
+ ownership. The ASF licenses this file to you under the Apache
+ License, Version 2.0 (the "License"); you may not use this file
+ except in compliance with the License. You may obtain a copy of
+ the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ implied. See the License for the specific language governing
+ permissions and limitations under the License.
+
+.. include:: ../../../common.defs
+
+.. default-domain:: cpp
+
+TSRemapNextHopStrategySet
+*************************
+
+Synopsis
+========
+
+.. code-block:: cpp
+
+ #include <ts/ts.h>
+
+.. function:: void TSRemapNextHopStrategySet(TSStrategy strategy)
+
+Description
+===========
+
+Sets the next hop strategy for the currently loading remap rule.
+This :arg:`strategy` pointer must be a valid strategy and can be
+nullptr to indicate that parent.config will be used instead.
+
+Plugins can get a strategy by name by calling
+:func:`TSRemapNextHopStrategyGet` to get the current transaction's active
+strategy or :func:`TSRemapNextHopStrategyFind` to look up a strategy by
+name using the loading remap rule's pointer to the NextHopStrategyFactory
+strategy database.
Review Comment:
`TSRemapNextHopStrategyGet` does not return a transaction’s active strategy;
it returns the currently-loading remap rule’s assigned strategy (during
`TSRemapNewInstance`). Please reword this paragraph to avoid conflating
remap-instance APIs with per-transaction APIs (e.g., describe
`TSRemapNextHopStrategyGet` as returning the remap rule strategy being loaded,
and `TSHttpTxnNextHopStrategyGet` as the per-transaction getter).
##########
include/ts/ts.h:
##########
@@ -1682,32 +1694,55 @@ void TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, void
const *strategy);
@param txnp HTTP transaction whose next hop strategy to get.
*/
-void const *TSHttpTxnNextHopStrategyGet(TSHttpTxn txnp);
+TSStrategy TSHttpTxnNextHopStrategyGet(TSHttpTxn txnp);
/**
Returns either null pointer or null terminated pointer to name.
- DO NOT FREE.
+ DO NOT FREE.
This value may be a nullptr due to:
- parent proxying not enabled
- no parent selection strategy (using parent.config)
- @param txnp HTTP transaction whose next hop strategy to get.
+ @param pointer to the NextHopStrategy.
*/
-char const *TSHttpNextHopStrategyNameGet(void const *strategy);
+char const *TSNextHopStrategyNameGet(TSStrategy strategy);
Review Comment:
The comment says this may return `nullptr`, but the implementation returns a
non-null string literal (\"null\") for a null strategy (per the PR
description). Please align API contract and docs: either (a) update this
comment + the corresponding rst docs to state it returns a null-terminated name
and returns \"null\" when `strategy == nullptr`, or (b) change the
implementation to return `nullptr` for `nullptr` input and keep the current
documentation.
##########
src/api/InkAPI.cc:
##########
@@ -5023,57 +5023,92 @@ TSHttpTxnServerRequestBodySet(TSHttpTxn txnp, char
*buf, int64_t buflength)
s->internal_msg_buffer_fast_allocator_size = -1;
}
-void const *
+TSStrategy
TSHttpTxnNextHopStrategyGet(TSHttpTxn txnp)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
auto sm = reinterpret_cast<HttpSM const *>(txnp);
- return static_cast<void *>(sm->t_state.next_hop_strategy);
+ return reinterpret_cast<TSStrategy>(sm->t_state.next_hop_strategy);
+}
+
+TSStrategy
+TSHttpTxnNextHopStrategyFind(TSHttpTxn txnp, const char *name)
+{
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM const *>(txnp);
+
+ sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap->strategyFactory)
== TS_SUCCESS);
+
+ // HttpSM has a reference count handle to UrlRewrite which has a
+ // pointer to NextHopStrategyFactory
+ NextHopSelectionStrategy *const strategy =
sm->m_remap->strategyFactory->strategyInstance(name);
+
+ return reinterpret_cast<TSStrategy>(strategy);
}
void
-TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, void const *stratptr)
+TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, TSStrategy stratptr)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
// null strategy falls back to parent.config
- // sdk_assert(sdk_sanity_check_null_ptr(strategy) == TS_SUCCESS);
-
- auto sm = reinterpret_cast<HttpSM *>(txnp);
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const *>(stratptr);
+ // sdk_assert(sdk_sanity_check_null_ptr(stratptr) == TS_SUCCESS);
- sm->t_state.next_hop_strategy = const_cast<NextHopSelectionStrategy
*>(strategy);
+ auto sm = reinterpret_cast<HttpSM *>(txnp);
+ sm->t_state.next_hop_strategy = reinterpret_cast<NextHopSelectionStrategy
*>(stratptr);
}
-char const *
-TSHttpNextHopStrategyNameGet(void const *stratptr)
+TSStrategy
+TSRemapNextHopStrategyFind(const char *name)
{
- char const *name = nullptr;
- if (nullptr != stratptr) {
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const
*>(stratptr);
- name = strategy->strategy_name.c_str();
+ auto const um = url_mapping::instance;
+ sdk_assert(sdk_sanity_check_null_ptr((void *)um) == TS_SUCCESS);
+
+ NextHopSelectionStrategy *strategy = nullptr;
+
+ if (nullptr != um->strategyFactory) {
+ // HttpSM has a reference count handle to UrlRewrite which manages
+ // the NextHopStrategyFactory pointer.
+ strategy = um->strategyFactory->strategyInstance(name);
}
- return name;
+ return reinterpret_cast<TSStrategy>(strategy);
}
-void const *
-TSHttpTxnNextHopNamedStrategyGet(TSHttpTxn txnp, const char *name)
+void
+TSRemapNextHopStrategySet(TSStrategy strategy)
{
- sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
- sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
+ auto const um = url_mapping::instance;
+ sdk_assert(sdk_sanity_check_null_ptr((void *)um) == TS_SUCCESS);
+ // null strategy falls back to parent.config
+ // sdk_assert(sdk_sanity_check_null_ptr(stratptr) == TS_SUCCESS);
- auto sm = reinterpret_cast<HttpSM const *>(txnp);
+ um->strategy = reinterpret_cast<NextHopSelectionStrategy *>(strategy);
+}
Review Comment:
New public APIs are added/changed here (`TSHttpTxnNextHopStrategyFind`,
`TSRemapNextHopStrategyFind/Get/Set`, and the changed
`TSHttpTxnNextHopStrategySet/Get` signatures). `InkAPITest.cc` was updated for
the type change, but there doesn’t appear to be added coverage validating the
new behavior (e.g., `Find` returning nullptr for unknown names,
remap-new-instance functions only working during `TSRemapNewInstance`, and
`TSNextHopStrategyNameGet` null behavior). Please add regression/unit tests for
these new API entry points to prevent silent API contract breaks.
##########
src/api/InkAPI.cc:
##########
@@ -5023,57 +5023,92 @@ TSHttpTxnServerRequestBodySet(TSHttpTxn txnp, char
*buf, int64_t buflength)
s->internal_msg_buffer_fast_allocator_size = -1;
}
-void const *
+TSStrategy
TSHttpTxnNextHopStrategyGet(TSHttpTxn txnp)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
auto sm = reinterpret_cast<HttpSM const *>(txnp);
- return static_cast<void *>(sm->t_state.next_hop_strategy);
+ return reinterpret_cast<TSStrategy>(sm->t_state.next_hop_strategy);
+}
+
+TSStrategy
+TSHttpTxnNextHopStrategyFind(TSHttpTxn txnp, const char *name)
+{
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM const *>(txnp);
+
+ sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap->strategyFactory)
== TS_SUCCESS);
+
+ // HttpSM has a reference count handle to UrlRewrite which has a
+ // pointer to NextHopStrategyFactory
+ NextHopSelectionStrategy *const strategy =
sm->m_remap->strategyFactory->strategyInstance(name);
+
+ return reinterpret_cast<TSStrategy>(strategy);
}
void
-TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, void const *stratptr)
+TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, TSStrategy stratptr)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
// null strategy falls back to parent.config
- // sdk_assert(sdk_sanity_check_null_ptr(strategy) == TS_SUCCESS);
-
- auto sm = reinterpret_cast<HttpSM *>(txnp);
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const *>(stratptr);
+ // sdk_assert(sdk_sanity_check_null_ptr(stratptr) == TS_SUCCESS);
- sm->t_state.next_hop_strategy = const_cast<NextHopSelectionStrategy
*>(strategy);
+ auto sm = reinterpret_cast<HttpSM *>(txnp);
+ sm->t_state.next_hop_strategy = reinterpret_cast<NextHopSelectionStrategy
*>(stratptr);
}
-char const *
-TSHttpNextHopStrategyNameGet(void const *stratptr)
+TSStrategy
+TSRemapNextHopStrategyFind(const char *name)
{
- char const *name = nullptr;
- if (nullptr != stratptr) {
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const
*>(stratptr);
- name = strategy->strategy_name.c_str();
+ auto const um = url_mapping::instance;
+ sdk_assert(sdk_sanity_check_null_ptr((void *)um) == TS_SUCCESS);
+
+ NextHopSelectionStrategy *strategy = nullptr;
+
+ if (nullptr != um->strategyFactory) {
+ // HttpSM has a reference count handle to UrlRewrite which manages
+ // the NextHopStrategyFactory pointer.
+ strategy = um->strategyFactory->strategyInstance(name);
}
- return name;
+ return reinterpret_cast<TSStrategy>(strategy);
}
-void const *
-TSHttpTxnNextHopNamedStrategyGet(TSHttpTxn txnp, const char *name)
+void
+TSRemapNextHopStrategySet(TSStrategy strategy)
{
- sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
- sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
+ auto const um = url_mapping::instance;
+ sdk_assert(sdk_sanity_check_null_ptr((void *)um) == TS_SUCCESS);
+ // null strategy falls back to parent.config
+ // sdk_assert(sdk_sanity_check_null_ptr(stratptr) == TS_SUCCESS);
- auto sm = reinterpret_cast<HttpSM const *>(txnp);
+ um->strategy = reinterpret_cast<NextHopSelectionStrategy *>(strategy);
+}
- sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap) == TS_SUCCESS);
- sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap->strategyFactory)
== TS_SUCCESS);
+TSStrategy
+TSRemapNextHopStrategyGet()
+{
+ auto const um = url_mapping::instance;
+ sdk_assert(sdk_sanity_check_null_ptr((void *)um) == TS_SUCCESS);
+ return reinterpret_cast<TSStrategy>(um->strategy);
+}
- // HttpSM has a reference count handle to UrlRewrite which has a
- // pointer to NextHopStrategyFactory
- NextHopSelectionStrategy const *const strat =
sm->m_remap->strategyFactory->strategyInstance(name);
+char const *
+TSNextHopStrategyNameGet(TSStrategy stratptr)
+{
+ static char const *const nullname = "null";
+ char const *name = nullname;
+ if (nullptr != stratptr) {
+ auto strategy = reinterpret_cast<NextHopSelectionStrategy *>(stratptr);
+ name = strategy->strategy_name.c_str();
+ }
- return static_cast<void const *>(strat);
+ return name;
}
Review Comment:
This is a read-only accessor but casts `stratptr` to a non-const
`NextHopSelectionStrategy *`. Consider casting to `NextHopSelectionStrategy
const *` (or using `auto const *strategy = ...`) to avoid implying the API may
mutate the underlying strategy object.
##########
src/api/InkAPI.cc:
##########
@@ -5023,57 +5023,92 @@ TSHttpTxnServerRequestBodySet(TSHttpTxn txnp, char
*buf, int64_t buflength)
s->internal_msg_buffer_fast_allocator_size = -1;
}
-void const *
+TSStrategy
TSHttpTxnNextHopStrategyGet(TSHttpTxn txnp)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
auto sm = reinterpret_cast<HttpSM const *>(txnp);
- return static_cast<void *>(sm->t_state.next_hop_strategy);
+ return reinterpret_cast<TSStrategy>(sm->t_state.next_hop_strategy);
+}
+
+TSStrategy
+TSHttpTxnNextHopStrategyFind(TSHttpTxn txnp, const char *name)
+{
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM const *>(txnp);
+
+ sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap->strategyFactory)
== TS_SUCCESS);
+
+ // HttpSM has a reference count handle to UrlRewrite which has a
+ // pointer to NextHopStrategyFactory
+ NextHopSelectionStrategy *const strategy =
sm->m_remap->strategyFactory->strategyInstance(name);
+
+ return reinterpret_cast<TSStrategy>(strategy);
}
void
-TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, void const *stratptr)
+TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, TSStrategy stratptr)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
// null strategy falls back to parent.config
- // sdk_assert(sdk_sanity_check_null_ptr(strategy) == TS_SUCCESS);
-
- auto sm = reinterpret_cast<HttpSM *>(txnp);
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const *>(stratptr);
+ // sdk_assert(sdk_sanity_check_null_ptr(stratptr) == TS_SUCCESS);
- sm->t_state.next_hop_strategy = const_cast<NextHopSelectionStrategy
*>(strategy);
+ auto sm = reinterpret_cast<HttpSM *>(txnp);
+ sm->t_state.next_hop_strategy = reinterpret_cast<NextHopSelectionStrategy
*>(stratptr);
}
-char const *
-TSHttpNextHopStrategyNameGet(void const *stratptr)
+TSStrategy
+TSRemapNextHopStrategyFind(const char *name)
{
- char const *name = nullptr;
- if (nullptr != stratptr) {
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const
*>(stratptr);
- name = strategy->strategy_name.c_str();
+ auto const um = url_mapping::instance;
+ sdk_assert(sdk_sanity_check_null_ptr((void *)um) == TS_SUCCESS);
+
+ NextHopSelectionStrategy *strategy = nullptr;
+
+ if (nullptr != um->strategyFactory) {
+ // HttpSM has a reference count handle to UrlRewrite which manages
+ // the NextHopStrategyFactory pointer.
+ strategy = um->strategyFactory->strategyInstance(name);
}
Review Comment:
`TSRemapNextHopStrategyFind` dereferences/forwards `name` without validating
it. The txn variant asserts `sdk_sanity_check_null_ptr((void *)name) ==
TS_SUCCESS`; the remap variant should do the same to avoid passing a null
pointer into `strategyInstance(name)`.
##########
plugins/header_rewrite/operators.cc:
##########
@@ -1645,45 +1645,48 @@ void
OperatorSetNextHopStrategy::initialize(Parser &p)
{
Operator::initialize(p);
+ _stratname = p.get_arg();
- _value.set_value(p.get_arg(), this);
- Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy::initialie: %s",
_value.get_value().c_str());
+ if (_stratname.empty() || "null" == _stratname) {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy() 'clear'");
+ } else {
+ _strategy = TSRemapNextHopStrategyFind(_stratname.c_str());
+ if (nullptr == _strategy) {
+ TSError("[%s] Failed to get strategy '%s'", PLUGIN_NAME,
_stratname.c_str());
+ _apply = false;
+ } else {
+ Dbg(pi_dbg_ctl, "OperatorSetNextHopStrategy() '%s'", _stratname.c_str());
+ }
+ }
}
void
OperatorSetNextHopStrategy::initialize_hooks()
{
- add_allowed_hook(TS_HTTP_READ_REQUEST_HDR_HOOK);
add_allowed_hook(TS_REMAP_PSEUDO_HOOK);
}
Review Comment:
This removes `TS_HTTP_READ_REQUEST_HDR_HOOK` from the allowed hooks for this
operator, which can break existing header_rewrite configs that used
`set-next-hop-strategy` outside remap. If `TSHttpTxnNextHopStrategySet` is
still valid at READ_REQUEST_HDR (it should be, as it only needs to run before
parent selection), consider keeping the previous allowed hook to maintain
backward compatibility; otherwise, this restriction should be explicitly
documented as a breaking change in the plugin/operator docs.
##########
src/api/InkAPI.cc:
##########
@@ -5023,57 +5023,92 @@ TSHttpTxnServerRequestBodySet(TSHttpTxn txnp, char
*buf, int64_t buflength)
s->internal_msg_buffer_fast_allocator_size = -1;
}
-void const *
+TSStrategy
TSHttpTxnNextHopStrategyGet(TSHttpTxn txnp)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
auto sm = reinterpret_cast<HttpSM const *>(txnp);
- return static_cast<void *>(sm->t_state.next_hop_strategy);
+ return reinterpret_cast<TSStrategy>(sm->t_state.next_hop_strategy);
+}
+
+TSStrategy
+TSHttpTxnNextHopStrategyFind(TSHttpTxn txnp, const char *name)
+{
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM const *>(txnp);
+
+ sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap->strategyFactory)
== TS_SUCCESS);
+
+ // HttpSM has a reference count handle to UrlRewrite which has a
+ // pointer to NextHopStrategyFactory
+ NextHopSelectionStrategy *const strategy =
sm->m_remap->strategyFactory->strategyInstance(name);
+
+ return reinterpret_cast<TSStrategy>(strategy);
}
Review Comment:
New public APIs are added/changed here (`TSHttpTxnNextHopStrategyFind`,
`TSRemapNextHopStrategyFind/Get/Set`, and the changed
`TSHttpTxnNextHopStrategySet/Get` signatures). `InkAPITest.cc` was updated for
the type change, but there doesn’t appear to be added coverage validating the
new behavior (e.g., `Find` returning nullptr for unknown names,
remap-new-instance functions only working during `TSRemapNewInstance`, and
`TSNextHopStrategyNameGet` null behavior). Please add regression/unit tests for
these new API entry points to prevent silent API contract breaks.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
