Opened 9 years ago

Closed 9 years ago

#954 closed defect (fixed)

avoid using macro in crypto_unittests

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20110614
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 2.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Apparently my supplemental comment on #782 was too late, so I'm
opening a new ticket.

I propose revise test cases added to crypto_unittests.cc in #782 that
heavily relied on C++ macro. See this link for my argument:
http://bind10.isc.org/ticket/782#comment:21

I already have a complete alternative. So I'm attaching it and putting
this ticket to review queue (but not for this sprint).

Subtickets

Attachments (1)

avoid-macro.diff (19.1 KB) - added by jinmei 9 years ago.

Download all attachments as: .zip

Change History (11)

Changed 9 years ago by jinmei

comment:1 Changed 9 years ago by jinmei

  • Owner set to UnAssigned
  • Status changed from new to reviewing

comment:2 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:3 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

The patch does not apply cleanly to master (anymore?), but was quite easy to reproduce (i could upload a new one if you want). In order for the tests not to fail though, i had to remove a temporary bit of code from HMAC::verify() (the same return false i had to remove for #951).

Code looks much better, however, it is still a bit more brittle than i'd like to see; so i have one more suggestion the for-loop at the end of doRFC4231Tests() should have its conditional check on the lengths of all three vectors (or there should be a test just before it to check they are of equal size), otherwise one if someone was to remove one of the elements, it would segfault.

i only saw this was not for this sprint after i was half done with it, sorry for that, i'm reassigning it back to you anyway (you may ignore if you want to or we could cheat)

comment:4 Changed 9 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:5 Changed 9 years ago by shane

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:6 in reply to: ↑ 3 Changed 9 years ago by jinmei

Replying to jelte:

The patch does not apply cleanly to master (anymore?), but was quite easy to reproduce (i could upload a new one if you want). In order for the tests not to fail though, i had to remove a temporary bit of code from HMAC::verify() (the same return false i had to remove for #951).

Code looks much better, however, it is still a bit more brittle than i'd like to see; so i have one more suggestion the for-loop at the end of doRFC4231Tests() should have its conditional check on the lengths of all three vectors (or there should be a test just before it to check they are of equal size), otherwise one if someone was to remove one of the elements, it would segfault.

Thanks for the check.

I believe I've implemented the suggested idea (with trivial
adjustments to apply the diff). I've created a separate branch,
trac954.

As for the failed tests, as we discussed we need to skip the tests for
now (dependency on #920). I've disabled them for now (via a kind of
quick hack, hoping we address #920 in not so long future) and left
comments about the dependency both in the code and the ticket #920.

Is it okay now?

comment:7 Changed 9 years ago by jinmei

  • Milestone changed from Next-Sprint-Proposed to Sprint-20110614
  • Owner changed from jinmei to jelte

comment:8 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Looks good.

One thing; With the use of isc::util::encode, I needed to add libutil to the LDADD list in src/lib/cryptolink/tests/Makefile.am. I have taken the liberty to make this change and push it to trac954.

Please merge :)

comment:9 in reply to: ↑ 8 Changed 9 years ago by jinmei

Replying to jelte:

One thing; With the use of isc::util::encode, I needed to add libutil to the LDADD list in src/lib/cryptolink/tests/Makefile.am. I have taken the liberty to make this change and push it to trac954.

Please merge :)

Thanks, merge done. Closing ticket.

comment:10 Changed 9 years ago by jinmei

  • Estimated Difficulty changed from 0.0 to 2.0
  • Resolution set to fixed
  • Status changed from reviewing to closed

Oh, and I'm giving an estimate of 2 to this ticket.

Note: See TracTickets for help on using tickets.