Opened 9 years ago

Closed 9 years ago

#583 closed defect (complete)

random Qid

Reported by: jreed Owned by: jelte
Priority: medium Milestone: R-Team-Sprint-20110308
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 1.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

See src/lib/asiolink/udp_query.cc

Make sure we have a random Qid.

Subtickets

Change History (8)

comment:1 Changed 9 years ago by stephen

  • Component changed from Unclassified to resolver
  • Estimated Difficulty changed from 0.0 to 1
  • Milestone set to R-Team-Sprint-20110308

comment:2 Changed 9 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:3 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review;

Simple uniform random number generator using boost.

At this moment, it is in asiolink, because that is where it is used. It should be moved to a lib/util (or something), together with the non-uniform nsas rng.

Also note that IOFetch now *sets* a random qid, but does not actually check it when it receives an answer.

(just had a short discussion about that, we agreed that that would be a separate task)

commit b723719cdad0da4db9b2099596c09f407dbdb135
(oh and ff55e63140852f4c7cf8a0404dde36442c45ff4a, i had forgotten to remove the TODO line)

comment:4 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

src/lib/asiolink/qid_gen.h
The "#ifdef" sentinel is __QID_GEN_: for consistency with the other header files is should be __QID_GEN_H.

Convention is to have private members suffixed with an underscore.

A little documentation as to what the mt19937 class is would be useful.

src/lib/asiolink/qid_gen.cc
Does the QidGenerator have to to be allocated dynamically? Why not make it a static object and call seed() in the constructor? That way there will be no complaints from memory checkers.

return values are not enclosed in brackets.

src/lib/asiolink/tests/qid_gen.cc
OK, will accept the argument for now. However, apart from the fact that I think that the Dilbert cartoon in http://www.random.org/analysis might indicate problems with this approach :-) I have added a task (#668) to the general backlog concerning the need to do proper randomness tests.

Other: Ticket #669 has been created for checking the QID in the response.

comment:6 in reply to: ↑ 5 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Replying to stephen:

src/lib/asiolink/qid_gen.h
The "#ifdef" sentinel is __QID_GEN_: for consistency with the other header files is should be __QID_GEN_H.

Convention is to have private members suffixed with an underscore.

fixed

A little documentation as to what the mt19937 class is would be useful.

added

src/lib/asiolink/qid_gen.cc
Does the QidGenerator have to to be allocated dynamically? Why not make it a static object and call seed() in the constructor? That way there will be no complaints from memory checkers.

Right. Originally i made it dynamic so we could enforce the use of the singleton instance; we could make the constructor private. But thinking of this again, I agree that having it automatically cleaned up is more important.

return values are not enclosed in brackets.

doh. fixed

src/lib/asiolink/tests/qid_gen.cc
OK, will accept the argument for now. However, apart from the fact that I think that the Dilbert cartoon in http://www.random.org/analysis might indicate problems with this approach :-) I have added a task (#668) to the general backlog concerning the need to do proper randomness tests.

Right.

Other: Ticket #669 has been created for checking the QID in the response.

cool :)

Changes in commit 058f40e75eb59aea1d3e18ffcdfdacc8386aafa3

comment:7 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

All OK, please merge.

comment:8 Changed 9 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged, closing ticket.

Note: See TracTickets for help on using tickets.