Opened 9 years ago

Closed 9 years ago

#745 closed enhancement (complete)

Conversion of nsas library to use the new logging interface

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

Description

Similar to ticket #738, but applied to the nsas library.

Subtickets

Change History (12)

comment:1 Changed 9 years ago by shane

  • Estimated Difficulty changed from 0.0 to 4
  • Milestone changed from Year 3 Task Backlog to Sprint-20110419
  • Priority changed from major to minor

comment:2 Changed 9 years ago by shane

  • Defect Severity set to N/A
  • Feature Depending on Ticket set to logging
  • Sub-Project set to DNS

comment:3 Changed 9 years ago by stephen

  • Owner set to stephen
  • Status changed from new to assigned

comment:4 Changed 9 years ago by stephen

  • Owner changed from stephen to UnAssigned
  • Status changed from assigned to reviewing

I've put in a few logging statements, but in a module such as this, buried deep in the internals of the resolver, it's difficult to add anything other than debug statements.

Three debug levels have been used: one for tracing "normal" operation (requests from the rest of the resolver), one to report the results of those requests, and one to report RTT results. Only a few messages have been used - as we get more experience, we may want to add more.

comment:5 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:6 Changed 9 years ago by vorner

  • Owner changed from vorner to stephen

Hello

I removed one + that was left in the message definitions.

I vaguely remember there was a compiler that had problems with const ints defined and initialized in header files and you use them for the debug levels. Maybe using them as static/in anonymous namespace or as an enum? But I don't remember which compiler it was, mine doesn't complain.

Also, while working on the datasrc library, I discovered the message compiler doesn't find the definitions when run from make distcheck, because the compilation happens in a different directory, so I had to modify the makefile and the compiler slightly. How is it possible it finds your message definitions and everything works?

Thanks

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

  • Owner changed from stephen to vorner

I removed one + that was left in the message definitions.

Thanks.

I vaguely remember there was a compiler that had problems with const ints defined and initialized in header files and you use them for the debug levels. Maybe using them as static/in anonymous namespace or as an enum? But I don't remember which compiler it was, mine doesn't complain.

There shouldn't be a problem: "in C, constant values default to external linkage, so they can appear only in source files. In C++, constant values default to internal linkage, which allows them to appear in header files." (From http://msdn.microsoft.com/en-us/library/357syhfh%28v=vs.71%29.aspx)

Also, while working on the datasrc library, I discovered the message compiler doesn't find the definitions when run from make distcheck, because the compilation happens in a different directory, so I had to modify the makefile and the compiler slightly. How is it possible it finds your message definitions and everything works?

Magic! :-) Actually, I think that there are several causes. My guess is that in this ticket, it worked because:

  1. You did a build before doing a "make distcheck".
  2. The Makefile.am in this ticket was incomplete - it did not include the created files on the CLEANFILES line. (In fact it also missed a couple of other lines as well - an update to this branch corrects that.)

Due to (a) the nsasdef.{cc,h} files were created in the source directory and because of (b) weren't tidied up when you did "make clean". As a result, "make distcheck" was able to find them when it created the distribution.

As to #744: Automake has to copy the generated files when creating the distribution because they are listed in the <library>_SOURCES variable. As they don't exist, it applies the dependency rule and tries to create them from the message file using the message compiler. Unfortunately at this stage the message compiler does not exist, hence the error. The solution is not to distribute the generated files, which can be done by putting them in the noinst_<library>_SOURCES variable instead.

What puzzles me though is why updating the message compiler fixed it in ticket #744 - my guess is that this was coincidental. (More on the message compiler in the review of that ticket.)

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

  • Owner changed from vorner to stephen

Hello

Replying to stephen:

  1. You did a build before doing a "make distcheck".
  2. The Makefile.am in this ticket was incomplete - it did not include the created files on the CLEANFILES line. (In fact it also missed a couple of other lines as well - an update to this branch corrects that.)

Ah, I see. And I saw this branch fail for me as well now.

As to #744: Automake has to copy the generated files when creating the distribution because they are listed in the <library>_SOURCES variable. As they don't exist, it applies the dependency rule and tries to create them from the message file using the message compiler. Unfortunately at this stage the message compiler does not exist, hence the error. The solution is not to distribute the generated files, which can be done by putting them in the noinst_<library>_SOURCES variable instead.

Hmm, noinst sounds to me like they shouldn't be installed by make install. Shouldn't that be some kind of nodist_?

Anyway, my failure wasn't at that point at all. It was after unpacking the package, when compiling. At first it was that it didn't find the file, then after including the full path, the message compiler complained it can't output the result because of permissions ‒ it wanted to output to the srcdir.

What puzzles me though is why updating the message compiler fixed it in ticket #744 - my guess is that this was coincidental. (More on the message compiler in the review of that ticket.)

That's the second part ‒ I need it to read from srcdir and output to builddir. That seems to work correctly. But yes, I'll include the makefile fixes from here to #744 as well.

Anyway, I believe this branch needs the compiler fix as well. It fails the same way now:

make[5]: Entering directory `/home/vorner/work/bind10/bind10-devel-20110322/_build/src/lib/nsas'
../../../src/lib/log/compiler/message ../../../../src/lib/nsas/nsasdef.mes
../../../src/lib/log/compiler/message ../../../../src/lib/nsas/nsasdef.mes
OPNMSGOUT, unable to open ../../../../src/lib/nsas/nsasdef.h for output: Permission denied
make[5]: *** [nsasdef.cc] Error 1
make[5]: *** Waiting for unfinished jobs....
OPNMSGOUT, unable to open ../../../../src/lib/nsas/nsasdef.h for output: Permission denied
make[5]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110322/_build/src/lib/nsas'

But I think that, as you suggested in email, we wait here for #901 and that one already includes the message compiler fix as well.

comment:9 Changed 9 years ago by stephen

  • Owner changed from stephen to UnAssigned

Hmm, noinst sounds to me like they shouldn't be installed by make install. Shouldn't that be some kind of nodist_?

Yes, that should have been nodist_. Sorry.

Anyway, I believe this branch needs the compiler fix as well.

It does. That's been included by incorporating a merge from the current version of master.

This just needs a quick review of the changes made as a result of #900 and #901.

comment:10 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:11 Changed 9 years ago by vorner

  • Owner changed from vorner to stephen

Hello

Just two minor points:

  • I'm little bit surprised by your indentation/line breaking. Having each .arg on a new line even when more of them would fit into the same line looks little bit wasting of space to me. But I guess nothing in our style guidelines would be against that, so if you like it this way, it's OK.
  • RRtype doesn't need .toText() within the .arg call, it is converted automatically, so the .arg(type_.toText()) calls can be simplified.

I think this is simple enough so it won't need another review cycle. Just merge after this.

Thanks

comment:12 Changed 9 years ago by stephen

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

RRtype doesn't need .toText() within the .arg call, it is converted automatically, so the .arg(type_.toText()) calls can be simplified.

Done.

I'm little bit surprised by your indentation/line breaking. Having each .arg on a new line even when more of them would fit into the same line looks little bit wasting of space to me. But I guess nothing in our style guidelines would be against that, so if you like it this way, it's OK.

That was in part to break up what would have been a very long string of text. Removing the .toText() has shortened the arguments, so I've combined some .arg() calls on the same line.

Committed as b653f950b906ecf4194d2a2cd07fa92ae47dd546

Note: See TracTickets for help on using tickets.