Opened 7 years ago

Closed 7 years ago

#2087 closed task (fixed)

add LabelSequence::getOffsetData() method

Reported by: jinmei Owned by: muks
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: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by jinmei)

It's the offset version of getData().

This should be easy, so let's piggy back this: Extend
MessageRenderer::writeName so it can take LabelSequence. We should
be able to do this so we don't have to construct a name object from
in-memory encoded data.

Unlike getData(), however, there's one non trivial issue: the internal
offset values would have to be adjusted because stripRight/Left
(rightly) doesn't adjust them. In my experiment I passed a placeholder
and let the method fill in it

    const uint8_t* getOffsetData(size_t* len,
                                 uint8_t placeholder[Name::MAX_LABELS]) const;

but this may not be a cleanest way to do it. Please consider if
there's a better way. This method won't be used the most performance
sensitive path (like find() or rendering) so it could be relatively
less efficient.

Better to do this after #2086.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 7 years ago by shane

  • Estimated Difficulty changed from 0 to 4

comment:3 Changed 7 years ago by shane

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

comment:4 Changed 7 years ago by muks

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

Picking.

comment:5 Changed 7 years ago by muks

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

Up for review. It is in the trac2087 branch (which is based on the trac2086 branch as that is a dependency).

comment:6 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks

Sorry, taking back as there's more to do in the bug description.

comment:7 Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned

Up for review.

comment:8 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

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

  • Owner changed from jelte to muks

There is one thing that I noticed (not necessarily just because of this ticket, but the combination with #2086 does make it visible;

if you pull out data and offsets, you cannot use them to create a new labelsequence, or at least, not one with the same properties as the one you got the data from; the isAbsolute() property is 'reset' to true in the new object (because of the way that property is stored/checked). This could potentially also change comparison behaviour

This may actually be a feature, or a bug, depending on the intended use (I have not decided which I think it is yet). And it may be useful, or might not matter in practice.

It is fixable, but would require more info to go to the raw constructor i think. And if we're only going to need this for the rendering than we may actually want this property.

Anyway, I think the code looks OK, and assuming the above is not a problem, I think this can be merged.

comment:10 in reply to: ↑ 9 Changed 7 years ago by muks

Replying to jelte:

if you pull out data and offsets, you cannot use them to create a new labelsequence, or at least, not one with the same properties as the one you got the data from; the isAbsolute() property is 'reset' to true in the new object (because of the way that property is stored/checked). This could potentially also change comparison behaviour

The code that uses this data would have to be careful about it. We could add a constructor argument (bool not_absolute), something like this:

diff --git a/src/lib/dns/labelsequence.cc b/src/lib/dns/labelsequence.cc
index 7627571..31a7bd8 100644
--- a/src/lib/dns/labelsequence.cc
+++ b/src/lib/dns/labelsequence.cc
@@ -25,11 +25,13 @@ namespace dns {
 
 LabelSequence::LabelSequence(const uint8_t* data,
                              const uint8_t* offsets,
-                             size_t offsets_size) : data_(data),
+                             size_t offsets_size,
+                             bool not_absolute) :   data_(data),
                                                     offsets_(offsets),
                                                     offsets_size_(offsets_size),
                                                     first_label_(0),
-                                                    last_label_(offsets_size_)
+                                                    last_label_(offsets_size_),
+                                                    init_not_absolute_(not_absolute)
 {
     if (data == NULL || offsets == NULL) {
         isc_throw(BadValue, "Null pointer passed to LabelSequence constructor");
@@ -214,7 +216,7 @@ LabelSequence::stripRight(size_t i) {
 
 bool
 LabelSequence::isAbsolute() const {
-    return (last_label_ == offsets_size_);
+    return ((last_label_ == offsets_size_) && (!init_not_absolute_));
 }
 
 size_t
diff --git a/src/lib/dns/labelsequence.h b/src/lib/dns/labelsequence.h
index 48f3241..d5efd27 100644
--- a/src/lib/dns/labelsequence.h
+++ b/src/lib/dns/labelsequence.h
@@ -60,7 +60,8 @@ public:
                                      offsets_(ls.offsets_),
                                      offsets_size_(ls.offsets_size_),
                                      first_label_(ls.first_label_),
-                                     last_label_(ls.last_label_)
+                                     last_label_(ls.last_label_),
+                                     init_not_absolute_(ls.init_not_absolute_)
     {}
 
     /// \brief Constructs a LabelSequence for the given name
@@ -76,7 +77,8 @@ public:
                                      offsets_(&name.offsets_[0]),
                                      offsets_size_(name.offsets_.size()),
                                      first_label_(0),
-                                     last_label_(name.getLabelCount())
+                                     last_label_(name.getLabelCount()),
+                                     init_not_absolute_(false)
     {}
 
     /// \brief Constructs a LabelSequence for the given data
@@ -96,7 +98,8 @@ public:
     /// \param offsets_size The size of the offsets data
     LabelSequence(const uint8_t* data,
                   const uint8_t* offsets,
-                  size_t offsets_size);
+                  size_t offsets_size,
+                  bool not_absolute = false);
 
     /// \brief Return the wire-format data for this LabelSequence
     ///
@@ -257,6 +260,7 @@ private:
     size_t offsets_size_;
     size_t first_label_;
     size_t last_label_;
+    const bool init_not_absolute_;
 };

Let's discuss and if we want this, we'll implement it in a separate ticket.

comment:11 Changed 7 years ago by muks

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

Merged to master in commit 49ad6346f574d00cfbd1d12905915fd0dd6a0bac:

* a1068f5 [2087] Rewrite code for better readability
* a16d268 [2087] Add MessageRenderer::writeName() variant for LabelSequence
* 5f6ead5 [2087] Change alignment of test data
* 99f765a [2087] Add the LabelSequence::getOffsetData() method

Resolving as fixed. Thank you for the reviews Jelte.

Note: See TracTickets for help on using tickets.