masaori335 commented on code in PR #13887:
URL: https://github.com/apache/trafficserver/pull/13887#discussion_r4354633638
##########
src/cripts/Plugins.cc:
##########
@@ -39,16 +39,13 @@ Plugin::Remap::Create(const std::string &tag, const
std::string &plugin, const c
const cripts::string &to_url, const Plugin::Options
&options)
{
Plugin::Remap inst;
- int argc = options.size() + 1;
- const char **argv = new const char *[argc];
const swoc::file::path path(plugin);
// Remap plugins expect the first two arguments to be the from and to URLs.
- argv[0] = from_url.c_str();
- argv[1] = to_url.c_str();
+ std::vector<const char *> argv{from_url.c_str(), to_url.c_str()};
- for (unsigned i = 0; i < options.size(); ++i) {
- argv[i + 2] = options[i].c_str();
+ for (const auto &option : options) {
+ argv.push_back(option.c_str());
}
Review Comment:
The remap.config path passes its pparams the same way (`RemapConfig.cc`
const_casts them too), and no in-tree remap plugin writes into its argv
strings; getopt only permutes the pointer array, which is writable here. Not
changed in this PR.
##########
src/cripts/Plugins.cc:
##########
@@ -39,16 +39,13 @@ Plugin::Remap::Create(const std::string &tag, const
std::string &plugin, const c
const cripts::string &to_url, const Plugin::Options
&options)
{
Plugin::Remap inst;
- int argc = options.size() + 1;
- const char **argv = new const char *[argc];
const swoc::file::path path(plugin);
// Remap plugins expect the first two arguments to be the from and to URLs.
- argv[0] = from_url.c_str();
- argv[1] = to_url.c_str();
+ std::vector<const char *> argv{from_url.c_str(), to_url.c_str()};
- for (unsigned i = 0; i < options.size(); ++i) {
- argv[i + 2] = options[i].c_str();
+ for (const auto &option : options) {
+ argv.push_back(option.c_str());
}
Review Comment:
The remap plugin API is defined by `argc`: "The number of valid elements is
argc" (doc/developer-guide/plugins/remap-plugins.en.rst). Nothing in
`PluginFactory`/`RemapPluginInfo` reads past `argc`, so I'd rather not add a
sentinel that the API doesn't promise.
##########
src/cripts/Plugins.cc:
##########
@@ -60,11 +57,10 @@ Plugin::Remap::Create(const std::string &tag, const
std::string &plugin, const c
elevate_access =
RecGetRecordInt("proxy.config.plugin.load_elevated").value_or(0);
ElevateAccess access(elevate_access ? ElevateAccess::FILE_PRIVILEGE : 0);
- inst._plugin = gPluginFactory.getRemapPlugin(path, argc, const_cast<char
**>(argv), error, isPluginDynamicReloadEnabled());
+ inst._plugin = gPluginFactory.getRemapPlugin(path,
static_cast<int>(argv.size()), const_cast<char **>(argv.data()), error,
+
isPluginDynamicReloadEnabled());
Review Comment:
Same as
https://github.com/apache/trafficserver/pull/13887#discussion_r4354633638.
--
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]
