Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#253 closed defect (fixed)

xfrout should use the case-sensitive compress mode.

Reported by: jinmei Owned by: zzchen_pku
Priority: low Milestone: A-Team-Sprint-20110223
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.5 Add Hours to Ticket: 0
Total Hours: 1.0 Internal?: no

Description

This is a spec requirement for AXFR.

See also Trac #142.

I'm putting this ticket on the backlog.

Subtickets

Change History (16)

comment:1 follow-up: Changed 50 years ago by zzchen_pku

  • Add Hours to Ticket changed from 0.0 to 1.0
  • Total Hours changed from 0.0 to 1.0

comment:1 follow-up: Changed 9 years ago by zhanglikun

Is there one RFC talking about case-sensitive compress mode. I have read RFC4343, and didn't find the word "MUST" to the use of case-sensitive compress mode for AXFR, or it's suggested in "DNS Zone Transfer Protocol (AXFR) draft-ietf-dnsext-axfr-clarify-14"?

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

Replying to zhanglikun:

Is there one RFC talking about case-sensitive compress mode. I have read RFC4343, and didn't find the word "MUST" to the use of case-sensitive compress mode for AXFR, or it's suggested in "DNS Zone Transfer Protocol (AXFR) draft-ietf-dnsext-axfr-clarify-14"?

It's in axfr-clarify:

   Hence, name compression in an AXFR message SHOULD be performed in a
   case-preserving manner, unlike how it is done for 'normal' DNS
   responses.  That is, although when comparing a domain name for

(in 3.4. Name Compression)

but my understanding is that this is a "clarification" document, and just reflects the common understanding of the original spec (RFC1034/1035).

p.s. when you work on it, please don't forget writing tests first:-)

comment:3 Changed 9 years ago by stephen

  • billable set to 0
  • Internal? unset
  • Milestone changed from feature backlog item to A-Team-Sprint-20110223
  • Priority changed from major to minor

comment:4 Changed 9 years ago by zzchen_pku

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

comment:5 in reply to: ↑ 2 Changed 9 years ago by zzchen_pku

Replying to jinmei:

It's in axfr-clarify:

   Hence, name compression in an AXFR message SHOULD be performed in a
   case-preserving manner, unlike how it is done for 'normal' DNS
   responses.  That is, although when comparing a domain name for

(in 3.4. Name Compression)

but my understanding is that this is a "clarification" document, and just reflects the common understanding of the original spec (RFC1034/1035).

Now we have rfc5936 for reference:-)

comment:6 Changed 9 years ago by zzchen_pku

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

It's ready for review now.

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

  • Owner changed from UnAssigned to zzchen_pku

Hello

Few minor points:

  • messagerenderer_python.cc:
    // TODO: set/get compressmode
    
    Can this be removed?
  • MessageRenderer_setCompressMode:
    PyErr_SetString(PyExc_TypeError,
                    "Message mode must be Message.PARSE or Message.RENDER")
    
    The error message looks wrong (it might be a copy-paste from somewhere).
  • The test_messagerenderer_set_compress_mode tries to set to case sensitive mode. It would be helpful to test setting it back to case insensitive as well.
  • Is it possible to test if the _send_message actually sends correct data, eg correctly compressed?

Otherwise it seems OK.

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

  • Owner changed from zzchen_pku to vorner

Replying to vorner:

  • messagerenderer_python.cc:
    // TODO: set/get compressmode
    
    Can this be removed?
  • MessageRenderer_setCompressMode:
    PyErr_SetString(PyExc_TypeError,
                    "Message mode must be Message.PARSE or Message.RENDER")
    
    The error message looks wrong (it might be a copy-paste from somewhere).
  • The test_messagerenderer_set_compress_mode tries to set to case sensitive mode. It would be helpful to test setting it back to case insensitive as well.

Well,good catch,updated.

  • Is it possible to test if the _send_message actually sends correct data, eg correctly compressed?

I don't intend to do it, because the underlying library unittest can ensure the correctness, it's may be a duplicate in some sense. Is that okay?

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

  • Owner changed from vorner to zzchen_pku

Replying to zzchen_pku:

Is it possible to test if the _send_message actually sends correct data, eg correctly compressed?

I don't intend to do it, because the underlying library unittest can ensure the correctness, it's may be a duplicate in some sense. Is that okay?

But the new line:

render.set_compress_mode(MessageRenderer.CASE_SENSITIVE)

There's no test to check if it is there. If someone refactored the code and forgot to set the compress mode to case sensitive, we wouldn't know it at all. In that sense the code isn't tested. I don't say it must be tested by reading the wire data, but at last some kind of test to check it was set correctly should be there.

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

  • Owner changed from zzchen_pku to vorner

Replying to vorner:

render.set_compress_mode(MessageRenderer.CASE_SENSITIVE)

There's no test to check if it is there. If someone refactored the code and forgot to set the compress mode to case sensitive, we wouldn't know it at all. In that sense the code isn't tested. I don't say it must be tested by reading the wire data, but at last some kind of test to check it was set correctly should be there.

Got it, thanks for your clarification.
I have updated the unittest, please check.

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

  • Owner changed from vorner to zzchen_pku

It seems OK. Just that you expect that the OS buffer is long enough to fit into the pipe (or unix socket or whatever it is). It is probably fine with such a small message in a test, though.

Anyway, I noticed that there's no changelog entry proposed. Would you merge, write one and I'll just look into the changelog after that?

Thanks

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

  • Owner changed from zzchen_pku to vorner

Replying to vorner:

It seems OK. Just that you expect that the OS buffer is long enough to fit into the pipe (or unix socket or whatever it is). It is probably fine with such a small message in a test, though.

Yeah, because the message is quite small.

Anyway, I noticed that there's no changelog entry proposed. Would you merge, write one and I'll just look into the changelog after that?

Sorry, forgot it:-)
Proposed changeLog entry:

 175.  [bug]       jerry
   src/bin/xfrout: xfrout use the case-sensitive mode to compress names in an
   AXFR message.
   (Trac #253, git TBD)
Last edited 9 years ago by zzchen_pku (previous) (diff)

comment:13 Changed 9 years ago by vorner

  • Owner changed from vorner to zzchen_pku

OK, so go ahead and merge. Thanks.

comment:14 Changed 9 years ago by zzchen_pku

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

Merged,closing it. Thanks.

comment:15 Changed 9 years ago by zzchen_pku

  • Add Hours to Ticket set to 1
  • Estimated Difficulty changed from 0.0 to 0.5
Note: See TracTickets for help on using tickets.