Skip to content

Conversation

@airween
Copy link
Member

@airween airween commented May 28, 2018

When I compiled ModSecurity with LMDB support, the related regression tests had failed, eg:

72 collection-resource-simple.json        Testing collection :: RESOURCE (2/2)           failed!
73 collection-resource.json               Testing collection :: RESOURCE (1/2)           failed!
74 collection-resource.json               Testing collection :: RESOURCE (2/2)           failed!

Looks like the LMDB backend isn't complete, but may be I'm wrong :).

Before you merge it, please review the comments here:
https://github.com/airween/ModSecurity/blob/908782a06af9d26962e244745bfa46d659d91828/src/collection/backend/lmdb.cc#L496
https://github.com/airween/ModSecurity/blob/908782a06af9d26962e244745bfa46d659d91828/src/collection/backend/lmdb.cc#L509
https://github.com/airween/ModSecurity/blob/908782a06af9d26962e244745bfa46d659d91828/src/collection/backend/lmdb.cc#L518

I don't understand some things, please help me to clarify them.

The patch fixed the regression tests without touched them.

zimmerle pushed a commit that referenced this pull request Jun 12, 2018
@zimmerle
Copy link
Contributor

Hi @airween,

Indeed those are good questions. I've changed the implementation a little bit to address some of your concerns:

Most of the checks that you have highlighted was there due to historical reasons. In the past the collection key was named after a concatenation of the collection name and key (+ others factors such as the web app id, not so relevant for the explanation).
The fact is that naming police was changed for performance improvements and the changes were not back ported to the LMDB back-end.

I've changed yours implementation to reflect the changes on InMemoryPerProcess collection. The modifications are available here: e51297b.

If you are OK with the patch above, I will proceed with the merge. Thank you.

@zimmerle zimmerle self-assigned this Jun 12, 2018
@zimmerle zimmerle added the 3.x Related to ModSecurity version 3.x label Jun 12, 2018
@zimmerle zimmerle added this to the v3.0.3 milestone Jun 12, 2018
@airween
Copy link
Member Author

airween commented Jun 12, 2018

Hi Felipe,

I've grabbed the pull_1787 branch, merged to my lmdbfix, reviewed the modifications and tests. All tests had passed. The modifications looks like right:

https://github.com/SpiderLabs/ModSecurity/compare/v3/master...airween:v3/lmdbfix_1787?expand=1

I think you can merge it to v3/master. Thanks.

@airween airween closed this Jun 12, 2018
@airween airween reopened this Jun 12, 2018
zimmerle pushed a commit that referenced this pull request Jun 12, 2018
@zimmerle
Copy link
Contributor

Merged! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.x Related to ModSecurity version 3.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants