Opened 8 years ago

Closed 7 years ago

#1086 closed enhancement (complete)

Ensure message consistency when outputting address+port or name+type+class

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

Description

Some logging messages output an endpoint (address + port) in the form "address#port" and others "address(port)". "address#port" is BIND9 usage, so messages should be changed to use that.

Similarly, although BIND9 is not always consistent, specifying a name, type and class is usually done using "name/type/class" (or "name/type" if class is not present). BIND 10 messages should be reviewed for adherence to that standard.

Subtickets

Change History (19)

comment:1 Changed 8 years ago by jelte

Where did that # originate? Doesn't every other piece of software use [address]:port (with square brackets to get rid of ambiguity introduced by :-separated v6 addresses)?

comment:2 Changed 8 years ago by shane

  • Milestone changed from New Tasks to Next-Sprint-Proposed
  • Type changed from defect to enhancement

comment:3 Changed 8 years ago by shane

  • Priority changed from very low to low

Note that we have standardized this:

CodingGuidelines#IPaddressandportformatting

So this work is to audit and fix all of these messages.

comment:4 Changed 7 years ago by shane

  • Milestone set to Next-Sprint-Proposed
  • Priority changed from low to medium

comment:5 Changed 7 years ago by shane

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

comment:6 Changed 7 years ago by shane

I did a few quick greps.

First, lets see if we used any printf-style output with #port:

$ find . -type f | xargs grep "#%d"
Binary file ./.git/objects/pack/pack-fad569f01b61428e6f9164903d4f47580abef7d7.pack matches

Nope. What about ostream-style with a character '#':

$ find . -type f | xargs grep "#' *<<"
./src/lib/server_common/client.cc:       << '#' << impl_->request_.getRemoteEndpoint().getPort();

One occurrence.

And ostream-style with a string "#":

$ find . -type f | xargs grep '#" *<<'
./examples/host/host.cc:        //cout << "Address: " << address << "\n" ; // "#" << port << "\n";
./src/lib/dhcp/option_custom.cc:            tmp << "#" << std::distance(fields.begin(), field) << " "
./src/lib/dhcp/option_custom.cc:            tmp << "#" << i << " "
./src/lib/acl/tests/logcheck.h:                EXPECT_TRUE(run[i]) << "Check #" << i << " did not run.";
./src/lib/acl/tests/logcheck.h:                EXPECT_FALSE(run[i]) << "Check #" << i << "did run.";

Looks like there is a commented-out version in the host examples that we need to get rid of.

And lets see if we used a string concatenate to build something:

$ find . -type f | xargs grep '#" *+'
$ find . -type f | xargs grep "#' *+"
Binary file ./.git/objects/pack/pack-f976871e7ebb588ae03fbf78678f7738fca7c796.pack matches
Binary file ./.git/objects/pack/pack-fad569f01b61428e6f9164903d4f47580abef7d7.pack matches
Binary file ./.git/objects/pack/pack-fad569f01b61428e6f9164903d4f47580abef7d7.idx matches

Doesn't look like it.

So, I may have missed some possible patterns, but I think that this ticket basically involves changing one line and deleting a commented-out line.

comment:7 Changed 7 years ago by shane

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

comment:8 Changed 7 years ago by jelte

  • Estimated Difficulty changed from 5 to 2
  • Milestone changed from Next-Sprint-Proposed to Sprint-20130402

As discussed in planning meeting, I am reducing the estimated time to 2

comment:9 Changed 7 years ago by jelte

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

comment:10 Changed 7 years ago by jelte

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

You missed the logging output, which were quite a few more :p

Anyway, ready for review;

In the python code, I added an AddressFormatter? class, similar (but not equal) to the ClientFormatter? in ddns.logger; we *could* consider basing the one in ddns on this new one but it takes very different input and has different extra option, so I left it alone for now.

For the few cases in C++ i did not create such a class as yet, these are outside of critical paths and had different inputs anyway. But I guess we could consider doing something similar.

comment:11 follow-up: Changed 7 years ago by jelte

ohyeah proposed changelog:
[func?]
Address + port output and logs is now consistent according to our coding guidelines, e.g. <address>:<port> in the case of IPv4, and [<address>]:port in the case of IPv6, instead of <address>#<port>

comment:12 in reply to: ↑ 11 Changed 7 years ago by jreed

Replying to jelte:

Address + port output and logs is now consistent according to our coding guidelines, e.g. <address>:<port> in the case of IPv4, and [<address>]:port in the case of IPv6, instead of <address>#<port>

"and [<address>]:<port> in" (add brackets around port too)

comment:13 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:14 follow-up: Changed 7 years ago by vorner

Hello

What is meant with the replacable address with the AddressFormatter?? Does it make sense, in python? Can that be a copy-paste of C++ code by accident?

Do the formatter take address-family or address-port?

            logger.info(BIND10_STARTING_PROCESS_PORT_ADDRESS,
                        self.curproc, AddressFormatter(address, port))

comment:15 Changed 7 years ago by vorner

  • Owner changed from vorner to jelte

comment:16 in reply to: ↑ 14 Changed 7 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

Hello

What is meant with the replacable address with the AddressFormatter?? Does it make sense, in python? Can that be a copy-paste of C++ code by accident?

The 'replacable address' bit was taken from the other (python) implementation, from what I can tell the idea would be to save a couple of AddressFormatter? instantiations by adding a setAddress() (which would only internally update PyObject?* pointers). I don't think it'd be very useful, but IMO the comment mostly shows that there are other approaches that could be considered :) I wouldn't be very much opposed to removing the consideration here altogether.

Do the formatter take address-family or address-port?

            logger.info(BIND10_STARTING_PROCESS_PORT_ADDRESS,
                        self.curproc, AddressFormatter(address, port))

Woops, that would error. Added test for this failure and fixed the call.

comment:17 Changed 7 years ago by vorner

  • Owner changed from vorner to jelte

It looks OK now. Please merge.

comment:18 Changed 7 years ago by vorner

  • Total Hours changed from 0 to 1.07

comment:19 Changed 7 years ago by jelte

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

Thanks!

Merged, closing ticket.

Note: See TracTickets for help on using tickets.