Opened 8 years ago

Closed 8 years ago

#2086 closed task (complete)

allow construct LabelSequence from "raw data"

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20120717
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: scalable inmemory
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 8 Internal?: no

Description (last modified by jinmei)

i.e. sets of const uint8_t* data (for name data and offsets).

We first need to make this class free from Name object (so directly
holding a uint8_t* of name data and offsets). Also make sure this
version of constructor "explicit" (implicit conversion from Name to
LabelSequence can be really confusing).

Then add another constructor to make this ticket possible.

One open question is how much we should validate its input. basically
the data should originally come from another LabelSequence? and should
be valid. But it's not guaranteed by the interface, so it should be
generally validated. But it could be expensive in the intended usage
of it - we should consider the tradeoff.

Subtickets

Change History (14)

comment:1 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 8 years ago by shane

  • Estimated Difficulty changed from 0 to 6

comment:3 Changed 8 years ago by shane

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120717

comment:4 Changed 8 years ago by jelte

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

comment:5 Changed 8 years ago by jelte

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

Ready for review.

Didn't know exactly how this is intended to be used, but depending on that, we could consider not using the real raw data for the offsets, but a reference to the vector as constructed in Name; would shave off one more member (currently it also needs the size of that data).

I moved the comparison code from Name to LabelSequence?, similar to how toText already worked (i.e. Name::compare() now constructs labelsequences instead of doing comparison itself). It does have a few changes (an extra isAbsolute() check, and removed a few checks that are now no longer necessary).

comment:6 Changed 8 years ago by muks

  • Owner changed from UnAssigned to muks

Picking for review.

comment:7 Changed 8 years ago by muks

  • Owner changed from muks to jelte

Hi Jelte

  • In LabelSequence::getData(size_t *len), this would need brackets around the return in our coding style:
    +    return &(data_[offsets_[first_label_]]);
    
  • This is the behavior we want, right (there's no constructor that makes a copy of the data)?
    +    /// \note The associated data MUST remain in scope during the lifetime
    +    /// of this LabelSequence, since only the pointers are copied.
    
  • For COMMONANCESTOR tests, please check the returned # of common labels and order too.

The compare() moved from Name looks ok, modifications to toText(), etc. are fine and the constructors and tests look fine too. make check passes, so you have a successful review. Please go ahead and merge after you look at the points above. :)

comment:8 follow-up: Changed 8 years ago by muks

Jelte: One more thing, sorry: What happens if the data passed to the raw data constructor exceeds MAX_LABELLEN and MAX_WIRE? How do we handle these? There should also be tests to test these conditions.

comment:9 Changed 8 years ago by muks

Jelte: Another "one more thing" :) Please go through the doc comments for LabelSequence? class and update them where Name is referenced.

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

Replying to muks:

Jelte: One more thing, sorry: What happens if the data passed to the raw data constructor exceeds MAX_LABELLEN and MAX_WIRE? How do we handle these? There should also be tests to test these conditions.

As noted in the ticket description, it's indeed worth considering how
much we should validate the input. While it could be relatively
expensive for input that is basically assumed to be safe, I guess it
may be still better to make sure at the constructor that the resulting
sequence is really valid. We can subsequently check the performance
impact (after completing everything) and revisit how much we should
skip the check for higher performance.

comment:11 Changed 8 years ago by jelte

  • Owner changed from jelte to muks

OK, addressed initial comments, and added a number of basic checks in the raw constructor.

I made all of the errors BadValue?, even though we have separate exceptions for two of the error conditions, but it seems more usable to only have to catch one exception here.

comment:12 Changed 8 years ago by jelte

note, in case you happen to have already pulled, just pushed a trivial change (removed a code comment that was of no use)

comment:13 Changed 8 years ago by muks

  • Owner changed from muks to jelte

It is fine now. Please go ahead and merge.

comment:14 Changed 8 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 8

Thanks, merged, closing ticket

Note: See TracTickets for help on using tickets.