Opened 9 years ago

Closed 9 years ago

#977 closed task (complete)

ACL base class

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

Description

We want to have various ACL checks, so we need an abstract base class for a ACL property check. For start, there must be a virtual function which will be called on the message and context (the IP address it was received from, etc) and returns boolean.

It is simple task, but multiple other tasks depend on it, so it is a separate ticket.

Subtickets

Change History (12)

comment:1 Changed 9 years ago by stephen

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

comment:2 Changed 9 years ago by vorner

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

I'm going to take this despite the discussion and the fact we'll probably change the syntax. It seems to me we'll need a base class anyway whenever there's a possibility to create custom checks (which is, as I understand it, one of the goals of BIND 10).

comment:3 Changed 9 years ago by vorner

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

I implemented it, with some virtual functions to aid future optimisations and some of convenience default implementations of them.

Most of it are comments, so hopefully this will be trivial.

I think the ACLs should go to separate library. We could separate the DNS based checks to some libdnsacl and have the „generic“ part in libacl, but we probably don't want to worry about it yet.

No changelog is provided, as this code is not used anywhere.

comment:4 Changed 9 years ago by jinmei

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

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

General

I was not sure if this design is good or not just from this set of
definitions, including

  • whether this style of mixture of inheritance and template is the best or quite better abstraction of what we'd like to achieve
  • the fact that having both Check and CompoundCheck? are a base class indicates we'll have quite deep levels of inheritance hierarchy. without a care such a design can be difficult to manage.

But, not to waste time, I'd like to be clear that these are not
objections. After all, we won't be able to be sure about these points
until we implement more specific cases. So, I'm okay with moving
forward with it for now. You may or may not want to answer these
points at this stage, but I don't request resolving the discussion as
a prerequisite for merge.

Specific comments

  • I made some trivial editorial changes and pushed them directly. Please check.
  • To describe template parameters for Check and CompoundCheck?, I'd use typename (instead of class) if types that are not a "pure" class can be used as "Context" as is the case in the test. This is basically a matter of taste, however, and it's up to you.
  • maybe it's better to make the constructor of the base class protected.
  • s/subexpressions()/getSubexpressions()/ according to our naming guideline.
  • I'd like to have descriptions about possible exceptions, at least for some important methods such as matches().
  • is it intentional that Check::toText() is *not* virtual? If it's intentional, the fact that this class has a pure virtual method will cause an awkward effect, i.e., we'll have to provide the implementation of the pure virtual method, although it's not prohibited.
  • I'd avoid using hardcode for 10000. I'd instead define a class static constant and refer to it anywhere else.

comment:6 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner
  • Status changed from accepted to assigned

comment:7 Changed 9 years ago by jinmei

  • Status changed from assigned to reviewing

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

  • Owner changed from vorner to jinmei

Replying to jinmei:

  • whether this style of mixture of inheritance and template is the best or quite better abstraction of what we'd like to achieve

Well, having this kind of hierarchy for single context (if we want different kinds of checks) looks like textbook example for polymorphism.

And while I believe we want to share code with different contexts where it makes sense, parts of the hierarchy will be completely different. And it would be problematic to write some generic base class for context, check every time it is the correct child, typecast it… So this looks like a textbook example for templates. So I just put them together into one class.

  • the fact that having both Check and CompoundCheck? are a base class indicates we'll have quite deep levels of inheritance hierarchy. without a care such a design can be difficult to manage.

Why so? And is 3 so deep? Most GUI components in whatever library has depths around 15.

  • maybe it's better to make the constructor of the base class protected.

What is the rationale behind this? There's no constructor, so nothing to make hidden really. And the compiler won't let to create this class directly anyway, as it has pure virtual methods, so the goal to stop people from creating it by accident is fulfilled automatically as well. Is there any other reason I don't see?

  • I'd like to have descriptions about possible exceptions, at least for some important methods such as matches().

Like this?

  • is it intentional that Check::toText() is *not* virtual? If it's intentional, the fact that this class has a pure virtual method will cause an awkward effect, i.e., we'll have to provide the implementation of the pure virtual method, although it's not prohibited.

This was an error, thanks for noticing. I wrote the header and comment only, but forgot even the implementation.

comment:9 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 4

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

Replying to vorner:

Well, having this kind of hierarchy for single context (if we want different kinds of checks) looks like textbook example for polymorphism.

And while I believe we want to share code with different contexts where it makes sense, parts of the hierarchy will be completely different. And it would be problematic to write some generic base class for context, check every time it is the correct child, typecast it... So this looks like a textbook example for templates. So I just put them together into one class.

While I'm not yet sure whether this is the best, I don't object to
this design policy at the moment (I'm just uncertain, not necessarily
doubting it).

  • the fact that having both Check and CompoundCheck? are a base class indicates we'll have quite deep levels of inheritance hierarchy. without a care such a design can be difficult to manage.

Why so? And is 3 so deep? Most GUI components in whatever library
has depths around 15.

Depth itself is not immediately a problem, but when we (heavily) try
to represent different behaviors by defining derived classes
recursively, it will easily lead to an explosion of classes. If we
talk about textbooks, that's a common lesson of design pattern
textbooks. But, after all, it depends on specific cases, so this
concern may or may not be applicable to our case. As I said in the
previous bullet, I'm simply not certain as I've only seen a piece of
the entire design with actual code and practices.

This is not a showstopper comment anyway, and I think we now should
move forward. I'd suggest closing this discussion, completing this
branch and allowing other tasks to start.

  • maybe it's better to make the constructor of the base class protected.

What is the rationale behind this? There's no constructor, so nothing to make hidden really. And the compiler won't let to create this class directly anyway, as it has pure virtual methods, so the goal to stop people from creating it by accident is fulfilled automatically as well. Is there any other reason I don't see?

Hmm, my reason was to prevent an accidental instantiation of the base
class, but you're right that it's not possible due to the existence of
a pure virtual (with no definition) method. I still think it's a good
practice so that it will be robust against a possible future change
like introducing the default implementation of these methods (while we
still want to make the base class abstract), but I'd leave the
decision to you.

  • I'd like to have descriptions about possible exceptions, at least for some important methods such as matches().

Like this?

Looks okay.

  • is it intentional that Check::toText() is *not* virtual? If it's intentional, the fact that this class has a pure virtual method will cause an awkward effect, i.e., we'll have to provide the implementation of the pure virtual method, although it's not prohibited.

This was an error, thanks for noticing. I wrote the header and comment only, but forgot even the implementation.

The revised code looks okay. (Actually I was a bit confused myself
and was wrong about saying we'll have to provide the implementation,
but in any case my another point held; I thought it was intended to be
virtual, and the revised code looks okay in that sense).

comment:11 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:12 Changed 9 years ago by vorner

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

OK, I added the constructor, just in case and merged.

Thanks

Note: See TracTickets for help on using tickets.