Opened 10 years ago

Closed 9 years ago

#61 closed task (fixed)

review: dnstime

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

Description

Moved from ticket #50:

  • unit tests please

It's fully covered by the RRSIG unit test, but we can add another one just for dnstime if you think it's necessary.

As far as I can see there are only two dnstime relate tests in RRSIG, so I wouldn't call it 'fully covered'. Also, "ToText?" part isn't tested at all.

As for whether we need to create a separate test class for dnstime, yes, I think so. In general test cases of a particular module should be defined as close as to the module itself ("dnstime" in this case) rather than tests as part of a separate module.

for RRSIG, it should be sufficent to have one "bad from text" case that triggers InvalidTime?.

I've made changes according to the above policy. The added test cases uncovered some of bugs the dnstime implementations. Please fix them so that all of the tests will pass. Also, there should also be more tests. Please add them.

And one bikeshed point: public function names DNSSECTimeToText and DNSSECTimeFromText don't follow our coding guideline:

Class names are LikeThis?, methods are likeThis(), variables are like_this, and constants are LIKE_THIS.

Although there's one subtle point that in C++ global functions are not "methods".

Subtickets

Change History (17)

comment:1 in reply to: ↑ description ; follow-up: Changed 10 years ago by each

As far as I can see there are only two dnstime relate tests in RRSIG, so I wouldn't call it 'fully covered'. Also, "ToText?" part isn't tested at all.

I meant that lcov reported 100% coverage on that file. (And toText() is included in that; doesn't the RRSIG toText() function get tested? At least it was in my working tree; it's possible I neglected to commit something though.)

That said, I'm happy with adding an additional file for this purpose.

And one bikeshed point: public function names DNSSECTimeToText and DNSSECTimeFromText don't follow our coding guideline:

Class names are LikeThis?, methods are likeThis(), variables are like_this, and constants are LIKE_THIS.

True, but I couldn't really call it dNSSECTimeToText(). dnssecTimeToText(), perhaps?

comment:2 in reply to: ↑ 1 Changed 10 years ago by jinmei

Replying to each:

As far as I can see there are only two dnstime relate tests in RRSIG, so I wouldn't call it 'fully covered'. Also, "ToText?" part isn't tested at all.

I meant that lcov reported 100% coverage on that file. (And toText() is included in that; doesn't the RRSIG toText() function get tested? At least it was in my working tree; it's possible I neglected to commit something though.)

That said, I'm happy with adding an additional file for this purpose.

Okay, so that's a differnce of the definition of "full". Anyway, when I ask for "unit tests", I mean my definition of "full unit tests" (including various kinds of pathological cases). As I said before, such "full" tests normally uncover bugs and make the implementation robust, and even more important, save reviewr's time.

And one bikeshed point: public function names DNSSECTimeToText and DNSSECTimeFromText don't follow our coding guideline:

Class names are LikeThis?, methods are likeThis(), variables are like_this, and constants are LIKE_THIS.

True, but I couldn't really call it dNSSECTimeToText(). dnssecTimeToText(), perhaps?

Right, I couldn't come up with a counter proposal either...dnssecTimeToText() seems acceptable one to me.

btw this reminds me of one thing: I suggest renaming the file suffix from "dnstime" to "dnssectime" so that the file and moduile names are as consistent as possible. And then...if it's okay say this is something like "DNS time" rather than "DNSSEC time", maybe we should keep the file name and change the function names to "timeFromText()" and "timeToText()" ("DNS" is implicitly specified as part of the namespace).

comment:3 Changed 10 years ago by each

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

I've addressed these comments in r1342. When you have time, can you take a look and let me know if the changes are adequate?

comment:4 follow-up: Changed 10 years ago by jinmei

  • Owner changed from jinmei to each

I've made some minor changes (r1425), but for the purpose of year1 the current code is probably okay.

I have some additional questions, which may be post year1 fodder:

  • why did you use enum for DATE_LEN? it will work, but const size_t seems to be more natuaral choice here. it's a single constant value of type size_t, and its only usage is to compare itself against another size_t value. by using enum you implicitly rely on automatic type conversion from an enum typt to size_t.
  • this code doesn't support unformated integer seconds from epoch. Is that intentional? (this means we can't support this type of time representation in RRSIG, etc). If this intentional, the first comment of timeFromText() doesn't make sense: it says "first", which seems to me to indicate there's a "second" (which I'd expect to be the bare seconds representation).
  • regarding the year 10,000 problem, I think the right solution is to use 64-bit integer consistently as BIND 9 does. And, we should also consider the "year 2106 problme". But I understand it's not urgent.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 10 years ago by each

  • why did you use enum for DATE_LEN?

No reason at all, except that I thought we decided at the first face-to-face that that was the idiom we'd use for constants when in C we would've used #define. I agree that a "static const size_t" class variable makes more sense; let's do that.

  • this code doesn't support unformated integer seconds from epoch. Is that intentional?

Hmm. No, it isn't. An earlier revision did support that format, and I can't remember when or why I removed it. But I think for Y1 we should just note the oversight and move on; do you agree?

  • regarding the year 10,000 problem, I think the right solution is to use 64-bit integer consistently as BIND 9 does. And, we should also consider the "year 2106 problme". But I understand it's not urgent.

Right now we're using time_t to store time values, which I thought was already 64 bits on modern operating systems, but apparently I was mistaken about that. So yes, I agree that for Y2 it would be good to switch to a universal 64-bit time format.

(That's not the year 10,000 problem, though. The problem with y10k is that the RRSIG text format specifically says a year is four digits long--so we're going to have to reject time values higher than that, or we'll risk interoperability problems.)

comment:6 in reply to: ↑ 5 Changed 10 years ago by jinmei

Replying to each:

  • this code doesn't support unformated integer seconds from epoch. Is that intentional?

Hmm. No, it isn't. An earlier revision did support that format, and I can't remember when or why I removed it. But I think for Y1 we should just note the oversight and move on; do you agree?

Yes.

(That's not the year 10,000 problem, though. The problem with y10k is that the RRSIG text format specifically says a year is four digits long--so we're going to have to reject time values higher than that, or we'll risk interoperability problems.)

basically I agree. Technically, though, we could support years past 10K with the bare second format + wrap-arounding.

comment:7 Changed 10 years ago by shane

Bump.

comment:8 Changed 9 years ago by shane

  • Milestone changed from 05. 3rd Incremental Release: Serious Secondary to 06. 4th Incremental Release

What is this ticket waiting on?

comment:9 Changed 9 years ago by stephen

  • billable set to 0
  • Internal? unset
  • Milestone set to A-Team-Sprint-20110223

comment:10 Changed 9 years ago by jinmei

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

comment:11 Changed 9 years ago by jinmei

After looking at the code more closely, comapring it to the BIND 9
implementation, I propose:

  • provide 32-bit and 64-bit versions, instead of the single time_t (whose size may vary on different systems) version
  • (still) not support the unformatted integer second representation. BIND 9 doesn't support it either, and since we've not seen a problem until now, it's a no issue in practice for now. So keep it simpler until we see the need for it.

This should be a quite easy task. I'd give it an estimation of size 2.

comment:12 Changed 9 years ago by jinmei

  • Estimated Difficulty changed from 0.0 to 2.0

comment:13 Changed 9 years ago by jinmei

Branch trac61 is ready for review.

The code change itself should be pretty trivial. Test cases may be
subtle, but I tried to give detailed explanation about what's tested.
I also gave a detailed description to the functions (which was simply
missing so far).

In the code, one possibly controversial point would be that I replaced
the use of gmtime() to in-house conversion logic derived from BIND 9.
One reason for this is because (as commented in the code) gmtime()
may not work for 64bit integers (depending on systems). Another
undocumented reason is gmtime() is not thread safe, and b10-resolver
may want to have thread safety here (there's gmtime_r(), but I've seen
cases where _r() functions are not available).

This is the proposed changelog entry:

  173.?	[bug]*		jinmei
	src/lib/dns: revised dnssectime functions so that they don't rely
	on the time_t type (whose size varies on different systems, which
	can lead to subtle bugs like some form of "year 2038 problem").
	Also handled 32-bit wrap around issues more explicitly, with more
	detailed tests.  The function API has been changed, but the effect
	should be minimal because these functions are mostly private.
	(Trac #61, git TBD)

comment:14 Changed 9 years ago by jinmei

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

comment:15 follow-up: Changed 9 years ago by vorner

  • Owner changed from UnAssigned to jinmei

I think the code is clean and can be merged.

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

Replying to vorner:

I think the code is clean and can be merged.

Thanks for the prompt review (as usual:-), merged.

I've noticed build/test failures via buildbot, which I directly fixed
on master. It may be a bit beyond super trivial, but I thought it's
mostly obvious and skipped review cycle.

Closing this ticket.

comment:17 Changed 9 years ago by jinmei

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