Hello Henri, I am impressed. Really nice job.
It was easy to build the redis version. I was able to run unauthenticated and authenticated scans. I observed no difference in terms of performance and only marginal differences regarding the results (that may have occured due to other side effects during my testing). I have not done large-scale scans yet though. What I observed while setting up was that INSTALL is not yet updated with some guide to get the redis properly configured and started. I used an older config I had for redis 2.6 (self compiled) which then all worked. I tried Debian 7 stock redis-server (2.4.14) and had to drop quite a lot of config options to get it to start. However, when stating a scan, I got this in openvassd.dump and the scan stopped: (openvassd:30647): lib kb_redis-ERROR **: Unexpected reply length (0) I assume it might work better when using a conf file derived from debian stock 2.4 redis-server. In the file doc/redis_config.txt you mention the number of databases. Though usually surely less than 1000, this can be pretty large numbers if you think of network phase scans where many thousands of hosts are already port-scanned. Will redis get troubled by this? Is it expensive to just in case set a pretty high max by default? The patch reads pretty good. Only few unrelated elements as far as I can tell (e.g. gpg-error in a CMakeLists.txt). I assume writing the ChangeLog will mean quite some work but also will help understanding this very comprehensive patch (and iron out last unintended or unrelated changes). I am very happy that this way we could get rid of so much old and ugly code. All in all it is definitely time to start the vote on CR#62 (http://www.openvas.org/openvas-cr-62.html). Perhaps mention there the compatible minimum version of redis-server. More reviews surely help! Best Jan Am Montag, 24. Februar 2014, 18:10:56 schrieb Henri Doreau: > this new revision fixes all known issues. It was sometimes a bit of a > struggle against monsters from the past but all my test cases now run > smoothly and I think the patch generally improves scanner's code > quality. > > I am pleased to see how the code behaves under load and > misconfigurations and how easy it is to maintain it in comparison to > the older one. As an example, I have updated the patch to make > assignments of a given (key,value) pair idempotent, like it currently > is. IOW: assigning 1 or 100 times the same (k,v) tuple makes no > difference. I find this behavior buggy but had to preserve it to avoid > annoying side effects. As a result, OpenVAS now uses redis sets > instead of lists. Switching from one to the other is a matter of > five-ish lines of code. > > I have tested many different configurations and have a zero-delta with > trunk. > > Oh, and I'm very happy with the patch statistics: 25 files changed, > 1548 insertions(+), 2016 deletions(-). -- Dr. Jan-Oliver Wagner | ++49-541-335084-0 | http://www.greenbone.net/ Greenbone Networks GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 202460 Geschäftsführer: Lukas Grunwald, Dr. Jan-Oliver Wagner _______________________________________________ Openvas-devel mailing list [email protected] https://lists.wald.intevation.org/cgi-bin/mailman/listinfo/openvas-devel
