On 2015-06-05 01:13, Richard Hansen wrote: > Hi David, > > Thanks for the changes. Everything looks like it's in working order, > except for one error handling bug in commit #4 (see my reply for commit > #4, which I will send shortly).
Assuming you're referring to your comment "this should be 'return false' (-1 is true)", fixed. > There are a bunch of issues/bugs that shouldn't affect proper operation, > but would be cleaned up in an ideal world (read: if the time it would > take to clean them up was worthwhile). Here is a summary of the bigger > items: > > * These changes introduce a LOT of duplicated code. When I originally wrote it, I spent a little time trying to find a way around this, but I couldn't think of anything. At some point, somebody should try again. I filed a ticket internally. > * Most of the commits should be split into multiple separate > commits. This is especially true for commit #4. That would be ideal, but we're a bit tight on time at the moment. I don't think this is worth the amount of time it would take for these commits. > * Type mismatches for database API (uint32_t when it should be > unsigned int, uint64_t when it should be unsigned long long, > uint8_t when it should be unsigned char, etc.). Fixed. > * Potential alignment/padding/offset issues in fillPduIpPrefix() Fixed. > * The code makes an assumption that you can always distinguish > address families via address length. As long as we have only IPv4 and IPv6, this is true. I don't think it's worth spending the time now to remove this assumption, but I filed an internal ticket to fix this in the future. > * Lots of verbiage could be eliminated via initializer lists. True, but I don't think it's worth spending the time now to change. I filed an internal ticket. > I'll reply in-line to each patch pointing out specific issues, but most > of the issues are minor enough to not be worth fixing now. (This is > especially true given that we want to get a release out very soon.) We > can clean things up later (after these commits are merged and we've cut > a new release), though some of the issues may not ever be worth fixing. > I'll leave it to your judgment to decide which are worth fixing and > which are not. > > A comment about my review style: When I ask a question like: > > What about foo? > > I'm mostly trying to convey: > > Is this a bug or intentional? If intentional, someone reading this > code might wonder to themselves, 'What about foo?', so there should > be a supporting comment. > > In other words, don't tell me the answer to the question, tell all of > the people that could potentially read the code. Thanks for your detailed review. I think I've addressed all the issues via a combination of code fixes, discussion, and new tickets. David Mandelberg (7): improve the schema for prefixes use new schema when inserting ROAs use new schema when querying ROAs partially rewrite bin/rpki-rtr/rtr-update.c use the new schema when querying rpki-rtr data update rpki-rtr tests based on recent changes make rpki-rtr database error messages more useful Richard Hansen (1): improvements to shell logging and error handling functions bin/rpki-rtr/rtr-update.c | 467 +- bin/rpki/query.c | 4 +- bin/rpki/upgrade.in | 88 +- lib/db/clients/rtr.c | 1046 ++- lib/db/clients/rtr.h | 134 + lib/db/prep-stmt.c | 110 +- lib/db/prep-stmt.h | 14 + lib/db/util.c | 89 + lib/db/util.h | 88 + lib/rpki-rtr/pdu.h | 2 + lib/rpki/cms/roa_general.c | 195 +- lib/rpki/cms/roa_utils.h | 33 +- lib/rpki/err.c | 1 + lib/rpki/err.h | 3 +- lib/rpki/querySupport.c | 250 +- lib/rpki/querySupport.h | 4 +- lib/rpki/scmf.h | 4 +- lib/rpki/scmmain.h | 54 +- lib/rpki/sqcon.c | 4 +- lib/rpki/sqhl.c | 206 +- lib/util/macros.h | 8 + lib/util/shell_utils | 74 +- mk/libdb.mk | 1 + mk/rpki-rtr.mk | 65 +- .../rtr/response.bad_pdu_usage.log.correct | 4 +- .../rtr/response.many_prefixes.log.correct | 8204 ++++++++++++++++++++ .../rtr/response.reset_query_first.log.correct | 48 +- .../rtr/response.reset_query_last.log.correct | 28 +- .../rtr/response.serial_notify.log.correct | 12 +- .../rtr/response.serial_queries.log.correct | 340 +- tests/subsystem/rtr/test.sh.in | 11 +- 31 files changed, 10689 insertions(+), 902 deletions(-) create mode 100644 tests/subsystem/rtr/response.many_prefixes.log.correct -- 1.9.1 ------------------------------------------------------------------------------ _______________________________________________ rpstir-devel mailing list rpstir-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rpstir-devel
