DNS over TCP clean-up and improvements #554
Conversation
|
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'. |
| if dataLength <= DecodeOffset { | ||
| logp.Debug("dns", EmptyMsg+" addresses %s", |
McStork
force-pushed
the
clean-dns-tcp
branch
from
ece2238
to
b0345cc
Compare
|
Will do more code refactoring |
McStork
force-pushed
the
clean-dns-tcp
branch
from
b0345cc
to
42321a1
Compare
McStork
changed the title
McStork
force-pushed
the
clean-dns-tcp
branch
from
e002af9
to
4766017
Compare
McStork
force-pushed
the
clean-dns-tcp
branch
from
eb164ec
to
703f730
Compare
McStork
force-pushed
the
clean-dns-tcp
branch
4 times, most recently
from
41bd034
to
b820a9e
Compare
|
Happy new year! May 2016 bring even more users to the Beat shippers 😃 . I finished with all the cleaning and fixes. @andrewkroh , your last comment went missing from GitHub 😞 but it isn't lost since this PR addresses it. The PR corrects the Stream management and improves the debug messages and the published error Notes. It is ready for review. |
* Use RFC 1035 'bytes offset' to decode DNS over TCP payloads * Correct Streams management * Improve error management (for Debug and published Notes) * Tests improvement * Split files of ```package dns``` Minor changes: * Change the name of dnsPrivateData to dnsConnectionData to reflect the naming used in other applayers * Split the ```Parse()``` method in multiple functions to comply more with the code convention used in other applayers implementation * Remove a PCAP file from the previous and first DNS over TCP pull request * Introduce a README.md file
|
Thanks @McStork, I will review this soon. |
| trans.Notes = append(trans.Notes, OrphanedResponseMsg) | ||
| logp.Debug("dns", OrphanedResponseMsg+" %s", tuple) | ||
| trans.Notes = append(trans.Notes, OrphanedResponse.Error()) | ||
| logp.Debug("dns", OrphanedResponse.Error()+" %s", tuple) |
There was a problem hiding this comment.
try to not pass a generated format string as first parameter to printf like functions. Better:
debug("%s %s", OrphanedResponse, tuple)
where
var debug = logp.MakeDebug("dns")
|
all in all LGTM |
* Use printf format in all debug messages * Rename err in handleDecode * Remove a useless pointer assignement and move another one into a if statement
|
I like all the test cases you added. 👍 |
| } | ||
|
|
||
| // Checks that PrepareNewMessage and Parse can manage two messages sharing one packet on the same stream | ||
| // It typically happens when a SOA is followed by AXFR |
There was a problem hiding this comment.
This comment misled me slightly. I thought it was going to be a test case for two DNS requests with unique IDs like you would see with a SOA and a AXFR being sent over one connection. That is a case that this package does not handle (yet).
There was a problem hiding this comment.
@andrewkroh Yes, the comment can be misleading. It would be better with two requests with unique ID.
But still, I think the test does the job with a short amount of lines of code. The test shows that two requests that share one connection (and one packet during the connection) will get parsed and create transactions. Right?
|
LGTM |
DNS over TCP clean-up and improvements
Refactor, clean-up and address reviews of the first DNS over TCP PR
package dnsMinor changes:
the naming used in other applayers
Parse()method in multiple functions to comply more with the code conventionused in other applayers implementation