Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-157497990
- Remove _strlen and _memcpy usage (see TS-4029).
- In ConfigFileGroup(), use ats_strdup() rather than malloc+memcpy.
- In Rollback::createPathStr(), use the Layout::relative_to() that returns
an allocated string.
- Layout::relative_to, correctly deals with the file argument being
an absolute path. So in Rollback::createPathStr(), you don't need
to treat absolute paths specially.
- Rollback::isFileNameRooted should be a function, not a member variable.
bool isFileNameRooted() const {
return *fileName == '/';
}
- ConfigFileGroup should not be responsible for unmarshalling data.
The data marshalling format is a contract internal to the local
manager. ConfigFileGroup should just deal with paths.
- Why did you need to introduce ConfigFileGroup? An alternative
would be to organize Rollback objects in a tree. Then you don't
need to do all the name lookups; you can just keep a pointer from
child to parent.
- You can simplify the local manager protocol a lot. Rename
MGMT_SIGNAL_CONFIG_FILE_GROUP_UPDATE to MGMT_SIGNAL_CONFIG_FILE_CHILD.
The payload to this message is the parent config name and the file
file path, like this:
struct {
int options;
const char * parent; // config name
const char * child; // child path (absolute)
};
I'm not sure whether passing options is needed, but consider whether
Rollback versioning should make backup copies of SSL keys. Passing
options would allow you to deal with this.
You don't need to marshall this by hand, use the API in MgmtMarshall.h
to marshall the fields to a memory buffer. This marshalling should
be done in a helper method on ProcessManager() that has an API
similar to:
ProcessManager::signalConfigFileChild(/* int options? */ const char
* name, const char * path)
- This adds a binary dependency from SSL configuration on the local
manager that didn't exist before. Obviously I am OK with this, but
I wanted to make that explicit.
- Please sit with Brian and Thomas and make a TSQA test for this
(in a separate pull request is OK).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
