[bind10-dev] DataSrc::findClosestEnclosure() and empty Name

[JINMEI Tatuya / 神明達哉]

I've been thinking about the empty Name approach in the usage of
DataSrc::findClosestEnclosure() since we discussed this topic
yesterday.  I'm personally still not convinced about the need for it
in this context, so I'll provide some more thoughts on this and a
possible counter proposal.

[Background: the findClosestEnclosure() method is repeatedly calls
over a list of different data sources, beginning with an "empty name"
as an argument, and updating the name when it finds it has a better
matching name in its data]

First of all, why I explicitly prohibited the creation of an "empty
name" (by making the default constructor private) is because I wanted
to eliminate the possibility *at compilation time* that an empty name
is accidentally passed to a name class module when only a valid name
is expected.  Once we allow the existence of such an invalid state of
name, any application including the name class implementation itself
will have to do explicit check whether a name is valid.  This will
make the entire API fragile.  I believe we agreed on this principle as
a general matter.

So the question is whether the data source usage is sufficiently
compelling for opening this can of worms.  After thinking about it
more, I now more strongly believe the answer is no.  The main reason
is what's actually necessary is a state of "no name is found", and an
"empty name" is used just as an easy technique to represent a state of
"find" operation.  In general, such semantics overloading is error
prone (just like "-1" indicating an error when an non negative integer
is normally expected with specific semantics) and IMO should be
avoided as much as possible.

In this sense, the current prototype version in the branch is much
better, in that we pass the state of "whether a name has been found"
as an explicit separate variable (i.e., "found").  But, IMO, this is
still suboptimal because any data source implementation needs to add a
check to see the state and behaves differently (btw, this point also
applies to the approach with an "empty name").

Instead, I'd introduce a separate class encapsulating the name
matching logic, and move the state based decision logic inside the
separate class (see the "NameMatch" class definition copied below).

With this helper class, each findClosestEnclosure() could be much
simpler, eliminating any state-based if-else logic:

void
ConcreteDataSrc::findClosestEnclosure(NameMatch& match) const
{
    const Name& qname = match.getQname();

    // find the closest enclosure name.  assume a local variable 'found' is
    // true if found, and 'container' references the enclosure name.
    if (found) {
        match.update(*this, container);
    }
}

(and, as an additional bonus, since we also encapsulate the matched
datasource in the NameMatch class, the findClosestEnclosure() doesn't
have to return a pointer to the matched data source any more, avoiding
another semantics overloading)

Note: in the quick hack NameMatch class, I actually use "semantics
overloading": NULL means no match.  I believe it's okay since it's
only within this tiny (leaf-only) class, and if we can easily change
it if we really want to avoid that.

The application (e.g. auth name server) would simply construct a new
NameMatch object with the query name, and pass it to
findClosestEnclosure() of the meta data source:

    match = NameMatch(query->qname);
    meta_data_source.findClosestEnclosure(match);

and examine the result to decide next steps:
    if (match.getClosestEnclosure() == NULL) {
        return servfail;
    } else {
        //look up NS for match.getClosestEnclosure(), etc.
    }

The meta data source implementation would also become simpler, because
it doesn't have to care about the return value of each
findClosestEnclosure() call in its loop:

MetaDataSrc::findClosestEnclosure(NameMatch& match) const
{
    BOOST_FOREACH (DataSrc* ds, data_sources) {
        // omit the "RRclass" check for brevity
        ds->findClosestEnclosure(match);
    }
}

---
JINMEI, Tatuya

class NameMatch {
public:
    NameMatch(const Name& qname) : closest_name_(NULL), best_source_(NULL),
                                   qname_(qname)
    {}
    ~NameMatch()
    {
        // if we allocate memory for closest_name_ (see below), do this:
        //delete closest_name_;
    }
    void match(const DataSrc& new_source, const Name& container)
    {
        if (closest_name_ == NULL ||
            closest_name_->getLabelCount() < container.getLabelCount()) {
            // if we cannot simply store the pointer, do this:
            //closest_name_ = new Name(container);
            closest_name_ = &container;
            best_source_ = &new_source;
        }
    }
    const Name& getQname() { return (qname_); }
    const Name* getClosestEnclosure() { return (closest_name_); }
    const DataSrc* getBestDataSource() { return (best_source_); }
private:
    const Name* closest_name_;
    const DataSrc* best_source_;
    const Name& qname_;
};