Opened 7 years ago

Closed 7 years ago

#2292 closed defect (fixed)

eliminate const_cast from domaintree.h

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

Description

I don't know whether this was discussed in code review, but I don't
think we should have allowed this:

typename DomainTree<T>::Result
DomainTree<T>::find(const isc::dns::LabelSequence& target_labels_orig,
                    DomainTreeNode<T>** target,
                    DomainTreeNodeChain<T>& node_path,
                    bool (*callback)(const DomainTreeNode<T>&, CBARG),
                    CBARG callback_arg) const
{
[...]
    if (!node_path.isEmpty()) {
        // Get the top node in the node chain
        node = const_cast<DomainTreeNode<T>*>(node_path.top());

If we do const_cast, there should basically be something wrong. In
the previous version of the in-memory data source read-only and modify
cases are not cleanly separated and there could be a reason why we
cannot easily make things in the find() (read-only) path const. The
new version should be clean enough, and we should make sure we don't
need beasts const_cast.

From a quick check it seems we need to introduce some non trivial
adjustments, but I believe it should be possible and won't require
fundamental changes to the public interfaces.

Subtickets

Change History (15)

comment:1 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 4
  • Milestone changed from Next-Sprint-Proposed to Sprint-20121009

4 points estimated during sprint planning meeting.

comment:2 Changed 7 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:3 Changed 7 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Hello

The code changes are mostly small, even when it took some time to make it work
and modify all the templates on the way. I don't think it needs a changelog
entry, because it's an internal change only.

I also noticed some of the find variants are not used except for the tests.
Should we care about that?

comment:4 Changed 7 years ago by jinmei

I've not given a full review to yet, but it looks like somewhat
different from what I originally expected, so let me begin with
a higher level question.

For example, I thought we should be able to make node
const ZoneTableNode* in ZoneTable::findZone() because this node
should only be used in read-only context. To do this we'll have to
change the definition of ZoneTable::FindResult so the type of
zone_data will be const ZoneData*. This will probably require
some adjustments to other interfaces...and, repeating this type of
propagation of constness, I expected we should eventually be able to
eliminate the non cost version of DomainTree::find() like this:

    Result find(const isc::dns::Name& name,
                DomainTreeNode<T>** node) const {
        DomainTreeNodeChain<T> node_path;
        const isc::dns::LabelSequence ls(name);
        return (find<void*>(ls, node, node_path, NULL, NULL));
    }

And, as a result of that, I thought we should be able to kill the
const_cast naturally.

Wouldn't it be possible?

comment:5 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to vorner

comment:6 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

It doesn't seem to be possible. Sometimes, we need the nodes to be mutable:

  • The ZoneTable::setZoneData is quite natural place for a mutable node.
  • Even the mutability of the zone_data in the FindResult is used sometimes, for example here:
    if (fr.zone_data != NULL) {
        ZoneData::destroy(mem_sgmt_, fr.zone_data, rrclass_);
    }
    

So if we found a way to get rid of the mutability, I think it would feel very
unnatural and counter-intuitive.

What I'm thinking of is making the find variants that return mutable version as
non-const (so you couldn't call them on const domain tree). I think this would
need some changes in the tests (I tried something like that as an experiment
and it complained), but it should be possible and more or less logical.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by jinmei

Replying to vorner:

It doesn't seem to be possible. Sometimes, we need the nodes to be mutable:

  • The ZoneTable::setZoneData is quite natural place for a mutable node.
  • Even the mutability of the zone_data in the FindResult is used sometimes, for example here:
    if (fr.zone_data != NULL) {
        ZoneData::destroy(mem_sgmt_, fr.zone_data, rrclass_);
    }
    

Actually, I thought this was misuse of the read-only interface and
wanted to correct at this opportunity. If we want to possibly modify
a found node, we should use insert().

Regarding setZoneData() itself (which is only used within
InMemoryClientImpl::load()) I also thought it's badly designed and
used (probably as a result of being developed incrementally and by
multiple developers). What we should have really done (and will have
to do IMO) is to pass both zone name and zone data to addZone() and
let it insert the name and set the data (in the current implementation
addZone() creates the zone data itself, but with the latest code
logic it's mostly meaningless. I guess this is one of the things we
didn't do well in the incremental development). Then we won't even
need setZoneData() at all (at least for the current set of
features).

So if we found a way to get rid of the mutability, I think it would feel very
unnatural and counter-intuitive.

What I'm thinking of is making the find variants that return mutable
version as non-const (so you couldn't call them on const domain
tree). I think this would need some changes in the tests (I tried
something like that as an experiment and it complained), but it
should be possible and more or less logical.

If we inevitably need the immutable version at least in the current
code logic, I'm okay with that. But I'm not yet convinced about that;
I explained we don't need it for the zone table/data example above,
and in my initial investigation I thought we didn't see inevitable
need for it.

Adding a mutable version of things makes the implementation fragile. A
careless developer could use the mutable version without thinking
about the necessity, leaving source of bugs; if we only have an
immutable version we can enforce the safer behavior at compile time.

So, while I don't oppose to your approach as a general sense, in this
particular case I'm not yet convinced and would like to seek to begin
with const-only version. If future needs require real mutability,
then I'll accept the interface extension at that point.

If it's not convincing to you, can I try to see if I can offer a
counter proposal? Maybe I'm wrong and there's a real place that
cannot avoid mutability, but I'd like to be confident about that with
an actual attempt.

comment:8 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:9 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

Actually, I thought this was misuse of the read-only interface and
wanted to correct at this opportunity. If we want to possibly modify
a found node, we should use insert().

Actually, I didn't know such a wide scope was meant by the ticket (so I didn't
even look how much the method is used). It seems to be possible when changing
stuff that far away.

I hope the current changes are not problematic (I removed the add method of the
in-memory data source). Also, the history isn't really clean now (the template
parameters were introduced and later removed in this branch). Should I try to
rebuild the history in some cleaner way? It shouldn't be that much work I
think, if we care about how the history looks like.

With regards

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by jinmei

Replying to vorner:

Actually, I thought this was misuse of the read-only interface and
wanted to correct at this opportunity. If we want to possibly modify
a found node, we should use insert().

Actually, I didn't know such a wide scope was meant by the ticket (so I didn't
even look how much the method is used). It seems to be possible when changing
stuff that far away.

Yeah I admit the ticket description wasn't clear. My bad. But I like
the revised version very much, overall:-)

I hope the current changes are not problematic (I removed the add method of the
in-memory data source). Also, the history isn't really clean now (the template
parameters were introduced and later removed in this branch). Should I try to
rebuild the history in some cleaner way? It shouldn't be that much work I
think, if we care about how the history looks like.

I personally don't care about the history, since in the end the entire
diff for the branch isn't that big. Regarding the add() method I
guess we need some discussions. See below.

general

I made a few minor cleanups and pushed them.

zone_table.h/cc

  • ZoneTable::addZone(): I suggest passing ZoneData* instead of an object holder:
    ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class,
                       const Name& zone_name, ZoneData* zone_data)
    
    (InMemoryClientImpl::load() should release the zone data from the holder at the call to addZone()). I intended SegmentObjectHolder?() to be pretty local, like boost::scoped_ptr although it has release(), and didn't expect it to appear in inter-class interfaces. (maybe we should make it a derived class of boost::noncopyable). I also think it's clearer that the ownership of the zone data has been passed to the ZoneTable if we explicitly release() it at the time of call to addZone(). This means ZoneTable::addZone() stores the passed pointer to a separate holder, which could be considered a bit of waste, but I think the construction/destruction of the holder is negligible and acceptable.
  • ZoneTable::addZone(): we probably want to keep something like AddResult and return the previous zone data, if any. As we implement background zone loading, we'll need to separate the "build", "swap", and "release" steps of zone data so that only the "swap" step has to be in a critical section.

domaintree_unittest

  • This comment doesn't seem to be very correct:
        // the child/parent nodes shouldn't "inherit" the callback flag.
        // "cdtnode" may be invalid due to the insertion, so we need to re-find
        // it.
    
    The pair of "cdtnode" and "insertion" sounds awkward:-), and I think regarding the comment "dtnode" was actually correct. To clarify the point, we might say:
        // the child/parent nodes shouldn't "inherit" the callback flag.
        // "dtnode" may have been invalidated due to the insertion, so we
        // need to re-find it.
    
    ...Hmm, and, actually, our latest implementation should ensure "dtnode" should still be valid in this scenario. So I think it's okay to skip this find and just do
        EXPECT_TRUE(dtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK));
    
    or check it explicitly just in case:
        // the child/parent nodes shouldn't "inherit" the callback flag.
        // "dtnode" should still validly point to "callback.example", but we
        // explicitly confirm it.
        EXPECT_EQ(TestDomainTree::EXACTMATCH, dtree.find(Name("callback.example"),
                                                         &cdtnode));
        ASSERT_EQ(dtnode, cdtnode);
    

memory_client_unittest

I'm afraid we cannot simply remove all of these tests. Things like
loadRRSIGsRdataMixedCoveredTypes() or addOutOfZoneThrows() still seem
to be necessary (or at least they cannot be removed due to this
ticket). Also, we need *some* interface to add a particular RRset to
an existing zone anyway in order to support incremental updates after
ixfr or DDNS.

#2268 revised the load/add interface quite substantially, so maybe we
merge this branch with #2268, and update some of these tests using the
revised add interface while keeping the constness of the domain tree
interface?

comment:11 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:12 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

I fixed the three small issues. But about this one, I have a problem understanding how you mean it.
Replying to jinmei:

I'm afraid we cannot simply remove all of these tests. Things like
loadRRSIGsRdataMixedCoveredTypes() or addOutOfZoneThrows() still seem
to be necessary (or at least they cannot be removed due to this
ticket). Also, we need *some* interface to add a particular RRset to
an existing zone anyway in order to support incremental updates after
ixfr or DDNS.

#2268 revised the load/add interface quite substantially, so maybe we
merge this branch with #2268, and update some of these tests using the
revised add interface while keeping the constness of the domain tree
interface?

Keeping the add() method seems to be a mutually exclusive requirement for the
find to return immutable nodes, at least without cheating. If we have immutable
node, we get only immutable ZoneData? and therefore can't modify it to add
another RRset.

Also, I thought the incremental updates would happen inside the memory manager
thing, which probably will look at the data in a different way. Therefore it
might be premature to do some cheating because of add() that we may not need in
the end.

Maybe the tests could be reconstructed without using add at all, just by
providing the right iterator, or something.

Thank you

comment:13 in reply to: ↑ 12 Changed 7 years ago by jinmei

Replying to vorner:

I fixed the three small issues.

These look okay.

But about this one, I have a problem understanding how you mean it.

I'm afraid we cannot simply remove all of these tests. Things like
loadRRSIGsRdataMixedCoveredTypes() or addOutOfZoneThrows() still seem
to be necessary (or at least they cannot be removed due to this
ticket). Also, we need *some* interface to add a particular RRset to
an existing zone anyway in order to support incremental updates after
ixfr or DDNS.

#2268 revised the load/add interface quite substantially, so maybe we
merge this branch with #2268, and update some of these tests using the
revised add interface while keeping the constness of the domain tree
interface?

Keeping the add() method seems to be a mutually exclusive requirement for the
find to return immutable nodes, at least without cheating. If we have immutable
node, we get only immutable ZoneData? and therefore can't modify it to add
another RRset.

Right, so we'll still think about the interface for getting mutable
zone data from the zone table (eventually).

Also, I thought the incremental updates would happen inside the memory manager
thing, which probably will look at the data in a different way. Therefore it
might be premature to do some cheating because of add() that we may not need in
the end.

Hmm, unless we switch to the shared segment version soon and
completely drop the local segment mode, I guess we still need to
handle this operation.

Having said that,

Maybe the tests could be reconstructed without using add at all, just by
providing the right iterator, or something.

Looking into the latest status of #2268 and the test cases, it looks
like we can address this concern without InMemoryClient::add(). So,
I'd suggest just leaving these tests with '#if 0' and merge this
branch. I'll make sure it will be addressed at the final step of #2268.

comment:14 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:15 Changed 7 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 8.11

Thank you, closing.

Note: See TracTickets for help on using tickets.