Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#997 closed task (complete)

Create the ACL class

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

Description

This class will hold bunch of conditions (from ticket #977) and actions. When it is called, it will feed the conditions one by one and performing the first action that matches.

This is to be a template class, so we can use it with whatever „bag of information“ structure to be passed into it. Maybe even the list of possible actions might be template parameter.

Subtickets

Change History (15)

comment:1 Changed 9 years ago by vorner

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

comment:2 Changed 9 years ago by vorner

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

It is ready for review. It should be quite stright-forward and short, so I hope there are no bugs.

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

  • Owner changed from UnAssigned to jinmei

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

I did't see anything obviously wrong, but (as usual?) I have some
points to discuss in details.

First, I made some trivial editorial changes and pushed them.
Having/removing a white space, e.g. from "{ }" may be purely matter of
taste (and the coding guideline doesn't say anything about this
point), but it seems we don't have a space for these cases in the vast
majority of the code, so I made them consistent.

Action enum

  • It seems the ACL module does intentionally not define the semantics of these (and the caller application can use it for whatever purpose it wants). I think this is reasonable, but it would be better if the description clarifies that point.
  • Since "Action" is a template parameter, and an application may introduce its own set of actions, I'd name "Action" something more basic, e.g. "BaseAction?", "BasicAction?", "BuiltInAction?", etc. It's just a comment, though.

Acl class

  • maybe "ACL" instead of Acl is a better name considering it's an acronym. but this is just a comment; I don't insist on it.
  • why not using boost::noncopyable to prohibit copy? we agreed on using instead of in-house privatization of the copy constructor and operator= for that purpose.
  • maybe a matter of taste, but I'd call the parameter to the constructor "default_action" instead of "policy" (and would name the corresponding member variable accordingly) as the former seems to better describe what it is.
  • "policy_" doesn't seem to be intended to be changed after the construction of the Acl class. If that's the intent, I'd suggest making it const.
  • as you may have anticipated, I'd generally avoid using protected member variables. In this case, if (as documented) the only reason for making them protected is to refer to them from tests, I'd introduce accessor methods, and, desirably, have them return a const reference (or a copy of the object). That way we can be sure it's a bug within the main ACL implementation (not in the test code or possible in a non-compliant application) when we encounter a strange bug where these member variables have been magically changed. If you insist on keeping them member variables, perhaps for brevity in the test code, I don't agree with the brevity argument, but I could at least live with that for "policy_" as long as it's const (see the previous point). I'd still argue the access to "entries_" should be more restricted because it cannot be immutable, but I'd also note that at least for now it doesn't have to be protected (the test code doesn't use it).
  • CheckPtr?: shouldn't it (better) be a shared pointer of a "const" object? i.e.
        typedef boost::shared_ptr<const Check<Context> > CheckPtr;
    
    (and, perhaps, name it ConstCheckPtr? following our convention)
  • execute(): is it reasonable to return an Action "object"? This means it must be copyable, and, depending on the internal detail of the actual Action type, the copy could be very expensive. On a related note, maybe it would be useful to describe the minimal requirement (such as copyability) that the Action template parameter must meet, and, if it's basically intended to be integral types, mention that, too.
  • execute(): this type of for loop is a bit more expensive because it involves a call to entries_.end() in each iteration. I'd not necessarily request it be addressed right now, but since this could be a performance sensitive path, I believe we should be careful.

Test

The use of reference of pointer seems to be abuse in the apparent
sense of Check class:

class ConstCheck : public Check<Log*> {
public:
[...]
    typedef Log* LPtr;
    virtual bool matches(const LPtr& log) const {
        log->run[log_num_] = true;
        return (accepts_);
    }

That is, while matches() is basically expected to not alter the
context, this implementation bypasses the design intent by hiding the
real context in the pointer. Maybe this is only for tests, and if so,
I'd at least add comments about the intent and/or (pass a normal
reference and) explicitly use const_cast to break the constness to
highlight the intent. Or, maybe in some cases contexts are actually
expected to be modified within matches(). Although I'd avoid that if
we could do so by design, but if there are really cases we want to do
that, I'd introduce a non-const version of matches() and let the
application use that with an explicit care.

comment:5 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

First, I made some trivial editorial changes and pushed them.
Having/removing a white space, e.g. from "{ }" may be purely matter of
taste (and the coding guideline doesn't say anything about this
point), but it seems we don't have a space for these cases in the vast
majority of the code, so I made them consistent.

I noticed ‒ I thought we were using camelCase for C++ code and underscores in python. Was I mistaken?

  • why not using boost::noncopyable to prohibit copy? we agreed on using instead of in-house privatization of the copy constructor and operator= for that purpose.

I simply forgot about that one. In my own code I usually don't even bother to document such class is not supposed to be copied and rely on common sense O:-).

  • execute(): is it reasonable to return an Action "object"? This means it must be copyable, and, depending on the internal detail of the actual Action type, the copy could be very expensive. On a related note, maybe it would be useful to describe the minimal requirement (such as copyability) that the Action template parameter must meet, and, if it's basically intended to be integral types, mention that, too.

I'm returning reference to it now (I somehow implicitly expected to be something small, but you're right, when we allow it to be quite anything, it may be worth it). But it still needs to be copyable, since I need to place it into the vector.

  • execute(): this type of for loop is a bit more expensive because it involves a call to entries_.end() in each iteration. I'd not necessarily request it be addressed right now, but since this could be a performance sensitive path, I believe we should be careful.

Well, considering that the vector is template, therefore completely available to compiler and the function is inlined, if it is combined with the fact that vector guarantees being really an array and iterator being wrapped pointer, this should optimise to single pointer comparison with the end pointer being fetched from constant memory location. Would there be any way of avoiding even that?

The use of reference of pointer seems to be abuse in the apparent
sense of Check class:

Yes, it's abuse for tests (and now even documented). I don't expect the match to modify the context (maybe with some exception of a scratchpad area for optimising them, but that one would be mutable and not created by the user).

I used mutable in the test, as it sounds a lot cleaner than const-cast to me (knowing how many optimisations compiler can base on constness of something and mutable is only for the part of the object that changes, while const-cast is both hidden, therefore possibly unsafe, and on the whole object).

that, I'd introduce a non-const version of matches() and let the
application use that with an explicit care.

That wouldn't work, because we would need to have two separate hierarchies, one with const and one with non-const or every check would need to provide both or something.

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

  • Milestone changed from Sprint-20110628 to Sprint-20110614

Replying to vorner:

Replying to jinmei:

First, I made some trivial editorial changes and pushed them.
Having/removing a white space, e.g. from "{ }" may be purely matter of
taste (and the coding guideline doesn't say anything about this
point), but it seems we don't have a space for these cases in the vast
majority of the code, so I made them consistent.

I noticed ‒ I thought we were using camelCase for C++ code and underscores in python. Was I mistaken?

We use the camel for classes, but use (lower case+) underscores for
variables. See the "Naming" section of
http://bind10.isc.org/wiki/CodingGuidelines

  • execute(): is it reasonable to return an Action "object"? This means it must be copyable, and, depending on the internal detail of the actual Action type, the copy could be very expensive. On a related note, maybe it would be useful to describe the minimal requirement (such as copyability) that the Action template parameter must meet, and, if it's basically intended to be integral types, mention that, too.

I'm returning reference to it now (I somehow implicitly expected to be something small, but you're right, when we allow it to be quite anything, it may be worth it). But it still needs to be copyable, since I need to place it into the vector.

Okay.

  • execute(): this type of for loop is a bit more expensive because it involves a call to entries_.end() in each iteration. I'd not necessarily request it be addressed right now, but since this could be a performance sensitive path, I believe we should be careful.

Well, considering that the vector is template, therefore completely available to compiler and the function is inlined, if it is combined with the fact that vector guarantees being really an array and iterator being wrapped pointer, this should optimise to single pointer comparison with the end pointer being fetched from constant memory location. Would there be any way of avoiding even that?

I suspect it's not safe to rely on specific compiler optimization too
much (on the other hand, of course, it wouldn't be wise to introduce
micro optimization in the code level when compilers are normally
expected to do a better job. so it's a trade off issue, and in this
case I personally think it's better to be careful). I'd also note
that it's not guaranteed that iterators are implemented as a pointer.

There are several ways to avoid the "obvious waste", and one
straightforward way is to define the end iterator explicitly, outside
the scope of the loop

        typename Entries::const_iterator const i_end(entries_.end());
        for (typename Entries::const_iterator i(entries_.begin());
             i != i_end; ++i) {

When we can rely on smartness of the compiler, the resulting code
would be equivalent to the most efficient version of the result from
the original code; when we cannot and/or the iterator is not
implemented as a pointer, the resulting code of this version will
still avoid the redundant evaluation of end().

I suspect you were of course aware of it and didn't choose to do so
for some reason, however. Perhaps that was for brevity. I personally
think in this case the additional code doesn't damage the readability
too much, but, as I already said, I wouldn't insist it be included.

One other minor point(s): if we change "policy" to "default action" in
acl.h, it would be better to make it consistent in acl_test.c. For
example.

  • checkPolicy would be renamed to checkDefaultAction
  • emptyPolicy would be something like emptyRule
  • AclTest::policy would be something like noDefaultAction
  • overall comment update

Oh, and one really last, very minor thing, maybe you want to rename
AclTest? to ACLTest.

comment:8 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:9 Changed 9 years ago by stephen

  • Milestone changed from Sprint-20110614 to Sprint-20110628

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

  • Owner changed from vorner to jinmei

Replying to jinmei:

There are several ways to avoid the "obvious waste", and one
straightforward way is to define the end iterator explicitly, outside
the scope of the loop

Hmm, why I didn't think of this? Maybe it was so simple I missed it O:-).

Actually, this can help even if theoretically completely smart compiler, as there's no guarantee the end() pointer couldn't be changed from inside a call to some check. Thanks for the suggestion.

One other minor point(s): if we change "policy" to "default action" in
acl.h, it would be better to make it consistent in acl_test.c. For

Lack of concentration on my side I guess. There should be no more policies in the code now.

Is it better?

comment:11 Changed 9 years ago by vorner

Hmm, I just noticed I have wrong branch names in the [ ] brackets in log. I'll fix it at merge.

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

Replying to vorner:

One other minor point(s): if we change "policy" to "default action" in
acl.h, it would be better to make it consistent in acl_test.c. For

Lack of concentration on my side I guess. There should be no more policies in the code now.

Is it better?

Yes.

And I made a few more editorial cleanups. They include the change
from underscore to camel for a *method*.

One final thing is that I'd constify the end iterator:

        typename Entries::const_iterator const end(entries_.end());

but in this case the entire amount of code is very small, so it's
mostly a matter of taste. I'd leave it to you.

Please merge with or without it.

comment:13 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:14 Changed 9 years ago by vorner

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

Thanks, merged.

comment:15 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 2
Note: See TracTickets for help on using tickets.