Looks good to go (with the fixup squashed). Thanks! Reviewed-by: Richard Hansen <rhan...@bbn.com>
On 2015-06-09 17:07, David Mandelberg wrote: > 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 > ------------------------------------------------------------------------------ _______________________________________________ rpstir-devel mailing list rpstir-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rpstir-devel
