On 2015-06-10 19:42, Richard Hansen wrote: > Looks good to go (with the fixup squashed). Thanks! > > Reviewed-by: Richard Hansen <rhan...@bbn.com>
Thanks. Merged and pushed. > > > 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 -- David Eric Mandelberg / dseomn http://david.mandelberg.org/ ------------------------------------------------------------------------------ _______________________________________________ rpstir-devel mailing list rpstir-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rpstir-devel
