Opened 9 years ago

Closed 9 years ago

#905 closed enhancement (complete)

TSIG: complete python library update

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

Description

This is a continuation of #814.

#814 only contained a minimal feature of library so that we can start
the signing part of xfrin. We need to provide other features, including
the verify part once #893 is completed.

This ticket should go to the 20110517 sprint. I'm stealing an estimate
of 4 (out of 5) from the original ticket for this one.

Subtickets

Change History (13)

comment:1 Changed 9 years ago by jinmei

  • Milestone changed from Sprint-DHCP-20110517 to Sprint-20110517

comment:2 Changed 9 years ago by jinmei

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

comment:3 Changed 9 years ago by jinmei

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

comment:4 Changed 9 years ago by jinmei

Branch trac905 is ready for review.

This is the second set of changes for the TSIG python library.
As I noted in the daily status jabber room, I wanted to introduce some
cleanup with this change, and I decided to make an intermediate
subtask to keep the size of code to be reviewed.

The first cleanup point is to use separate .cc and .h files for python
class bindings rather than having all implementation files through
#include in the main pydnspp.cc. This approach requires careful
consideration of inclusion ordering, which is becoming a difficult
task as the binding gets bigger. It's also error prone because we
sometimes use 'using namespace' in .cc files, but with this approach
this means doing so before including some other header files, which is
generally considered a bad practice.

The second cleanup is to introduce helper utilities to use the python
C API more safely (and hopefully more easily). I've written my
motivation in lit/util/python/pycppwrapper_util.h in detail.

Another cleanup is to introduce a template of the python binding
code. I found many of the binding code has the same pattern, so
rather than copy-paste-editing an existing one I thought it would make
more sense to have a single real template and generate a more complete
initial file. They are in lib/util/python/, too.

Regarding the main topic of this task, I've only added one small C++
class, TSIGError, mainly to show how the cleanups work (for which I'll
create a yet another ticket). I'd like to get this set of changes
reviewed, while I work on developing the rest of the bindings.

comment:5 Changed 9 years ago by jinmei

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

comment:6 Changed 9 years ago by jinmei

  • Estimated Difficulty changed from 4.0 to 2.0

comment:7 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:8 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jinmei

Reviewing differences between commit e93f054a2d0cf1c21d00112ccaa93d5d2432a677 (where branch for #893 was merged immediately after creating the branch for #905) and commit 9eddc07cd32918f3b8e9ebd114d9c8f8f39a359b (latest commit).

src/lib/dns/python/tsigerror_python.cc
Would help if there was a little more documentation on the class; for example, it would be helpful to highlight the different constructors (one with an integer and the copy constructor).

src/lib/util/python/pycppwrapper_util.h
Very clever: it should simplify the creation of Python interfaces.

Other

This approach requires careful consideration of inclusion ordering, which is becoming a difficult task as the binding gets bigger.

Why not just include #ifdef sentinels in the .cc files and include them in one another as required? Providing there are no loops, the inclusion ordering won't matter.

It's also error prone because we sometimes use 'using namespace' in .cc files, but with this approach this means doing so before including some other header files, which is generally considered a bad practice.

Maybe, but as there are only a few different namespaces being "used" (I found six, including "std"), why not just put them at the head of the pydnspp.cc and be done with it.

An alternative would be to put the appropriate "using" declaration within some of the longer functions (so limiting the scope to just that function) and use the namespace in type declarations elsewhere.

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

Replying to stephen:

Again, thanks for the review!

Reviewing differences between commit e93f054a2d0cf1c21d00112ccaa93d5d2432a677 (where branch for #893 was merged immediately after creating the branch for #905) and commit 9eddc07cd32918f3b8e9ebd114d9c8f8f39a359b (latest commit).

Ah, yes, that's right. The merge commit was expected to be ignored.
I forgot to mention that in the review request.

src/lib/dns/python/tsigerror_python.cc

Would help if there was a little more documentation on the class; for example, it would be helpful to highlight the different constructors (one with an integer and the copy constructor).

(We don't have/need a python binding for the copy constructor, but
anyway) you mean documentation for the python lib embedded in the C++
code, right? If so, fair enough...and I've decided to use this
opportunity to address a long standing issue: reducing the overhead of
providing the python doc. Most of the doc should be able to be a copy
of the corresponding C++ doc, in theory it should be possible to share
a single source of doc for both C++ and python. I've written a python
script that parses XML output from doxygen and adjusts it for the
python library. There are still some non trivial points that cannot
automatically converted, so we still need to edit the output by hand,
but it should now be much easier to provide quite consistent set of
document for both C++ and python.

I've added a new file, tsigerror_python_inc.cc, which contains the
half-automated with minimally edited documentation.
tsigerror_python.cc includes it.

I plan to cleanup the script and include it (after review) in our
source tree, but that will be a separate task.

Other

This approach requires careful consideration of inclusion ordering, which is becoming a difficult task as the binding gets bigger.

Why not just include #ifdef sentinels in the .cc files and include them in one another as required? Providing there are no loops, the inclusion ordering won't matter.

Hmm, I didn't think about that possibility, but I suspect it's not
always that simple due to possible circular dependency (tsig could
have included message although I didn't choose that option, and
message would need to include tsig, etc). Unless we at least separate
header files (where we declare minimum things, some of which would be
just forward declaration) and implementation files, this approach
won't work. Having circular dependency itself often indicates a
design issue and (if we have it) may need to be addressed anyway, but
it would also be nicer if the python binding works regardless of the
existence of circular dependency.

If we separated headers and implementations, it might be a matter of
style whether or not we combine the implementation files into a single
.cc by #include. Although I personally think it's more natural to
keep different implementation files different translation units, I do
not necessarily object that if you strongly prefer combining them.

It's also error prone because we sometimes use 'using namespace' in .cc files, but with this approach this means doing so before including some other header files, which is generally considered a bad practice.

Maybe, but as there are only a few different namespaces being "used" (I found six, including "std"), why not just put them at the head of the pydnspp.cc and be done with it.

An alternative would be to put the appropriate "using" declaration within some of the longer functions (so limiting the scope to just that function) and use the namespace in type declarations elsewhere.

(I'm afraid I may not fully understand the first approach - it simply
seems to have the same problem, but in any case) such workaround may
work, but even so, the problem is that it's quite easy for us to
forget such unusual convention and break it. Again, while I don't
necessarily disagree with keeping combining them, I don't see much
benefit to do so despite the fragile workaround.

But, to be clear, if that's your strong preference, I don't
necessarily object and am okay with moving forward with it as long as
we separate header files and avoid 'using namespace' before including
header files.

comment:10 Changed 9 years ago by jinmei

  • Owner changed from jinmei to stephen

comment:11 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jinmei

(We don't have/need a python binding for the copy constructor, but anyway) you mean documentation for the python lib embedded in the C++ code, right? If so, fair enough...

Actually I meant commenting the calls to PyArg_ParseTuple() in TSIGError_init() - what constructor call do they correspond to?

and I've decided to use this opportunity to address a long standing issue: reducing the overhead of providing the python doc.

... but I'm pleased to see that it has triggered a documentation improvement elsewhere.

I suspect it's not always that simple due to possible circular dependency.

Nothing ever is simple :-) But if that is the case, you would probably have a problem with inclusion order anyway.

But, to be clear, if that's your strong preference, I don't necessarily object and am okay with moving forward with it as long as we separate header files and avoid 'using namespace' before including header files.

I don't have a preference, they were just some suggestions for tackling the problem; use whatever you thing best. But having said that, I do think the suggestion of using scope-limited "using namespace" declarations is the most viable.

Changes are OK, please merge.

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

Replying to stephen:

(We don't have/need a python binding for the copy constructor, but anyway) you mean documentation for the python lib embedded in the C++ code, right? If so, fair enough...

Actually I meant commenting the calls to PyArg_ParseTuple() in TSIGError_init() - what constructor call do they correspond to?

Ah...okay. Updated the comment.

and I've decided to use this opportunity to address a long standing issue: reducing the overhead of providing the python doc.

... but I'm pleased to see that it has triggered a documentation improvement elsewhere.

I had a feeling that I may have misunderstood you, but due to the time
difference I couldn't check that timely...anyway, this (half)
automation has been needed, and it's a sooner-is-better thing, so this
is probably a good misunderstanding.

I suspect it's not always that simple due to possible circular dependency.

Nothing ever is simple :-) But if that is the case, you would probably have a problem with inclusion order anyway.

In fact, that's one major motivation of my proposed change. As we
extend libdns++ and pydnspp, it will be more probable that we actually
have this trouble.

But, to be clear, if that's your strong preference, I don't necessarily object and am okay with moving forward with it as long as we separate header files and avoid 'using namespace' before including header files.

I don't have a preference, they were just some suggestions for tackling the problem; use whatever you thing best. But having said that, I do think the suggestion of using scope-limited "using namespace" declarations is the most viable.

Okay, I'm not arguing separating .cc files are *the best*, but for now
I'll keep the current style. If someone finds a stronger need for
re-combining them with limited scope using directives, that's their
decision.

Changes are OK, please merge.

Okay, thanks, merged. Closing ticket.

comment:13 Changed 9 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.