add a function to parse a srv reply with ttl #393
Conversation
This adds a new private srv record struct with a ttl field. It also adds public accessor functions to set all of the fields. Finally, it replicates the ares_parse_srv_reply function to use the new struct and accessors.
|
I made this to confirm whether this was how you wanted to approach #387. If it is, then I will implement the parsing for the other RR records on this PR too, so no need to merge it yet. I also need to add more tests. Thanks. |
|
Looks like its going in the right direction. I would think though that we should make ares_parse_srv_reply() call ares_parse_srv_reply_ext() so we don't have 2 basically identical code paths. Basically make the old one a legacy wrapper. Infact, I'd almost opt for naming ares_parse_srv_reply_ext() -> cares_parse_srv_reply() ... and start making a new parallel API that is more modern and extensible prefixed with cares_ ... afterall, this is a fork of the original ares project. |
|
That makes sense to me, but the old one would still return a linked list of the old ares_srv_reply structs right? We don't want to return the new one with the TTL since that would break ABI? Also, for the life of me I can't figure out why the NMAKE build is failing. Do you have any idea there? |
|
anytime you're dealing with message parsing itself, its always better not to do it too often ... most bugs tend to be in that sort of code. So making the legacy function a wrapper is preferred, even if its still tedious and a good amount of code. It also makes the legacy function easier to maintain in the long run. Regarding the windows build failure, my guess is you have some additional whitespace in Makefile.inc that is hard to see. Nmake is very white-space sensitive. It sort of looks like you used spaces between the ares_reply_ext.c and the \ terminating the line, but everything else is using tabs. |
|
Thanks for your help; I made ares_parse_srv_reply a wrapper for cares_parse_srv_reply. Is this on the right track for what you envisioned? I want to make sure before I start working on other parse functions. |
|
Note that two sanitizer builds fail tests now. |
|
Sorry for the delay, been slammed lately. I think the setters should be private, and only the getters be public. The setters would be tested already in the internal implementation, and the getters would be tested by the legacy function wrappers when those are called for the most part. And yes, I think everything in one large PR would be best as it may uncover things that hadn't been thought about previously. |
CAA MX NAPTR NS PTR SOA TXT
update branch
| aptr += rr_len; | ||
| if (aptr > abuf + alen) | ||
| { /* LCOV_EXCL_START: already checked above */ | ||
| status = ARES_EBADRESP; | ||
| break; | ||
| } /* LCOV_EXCL_STOP */ |
There was a problem hiding this comment.
Some parse functions have this check after moving aptr, and some don't. Should all of them have it?
| const char* cares_srv_reply_get_host(const cares_srv_reply* srv_reply) | ||
| { | ||
| return srv_reply->host; | ||
| } |
There was a problem hiding this comment.
Should we check for null on the input variables in these getters? Like on srv_reply here? It will be UB if a caller passes a null pointer, right?
|
I think this is getting pretty close, let me know what you think. I left some questions above, and I also want to point out that I made new types for cares_ns_reply and cares_ptr_reply. The parse functions return a linked list of those types rather than a hostent. I hope that's okay; it felt more consistent to me. |
| break; | ||
| cares_srv_reply_set_host(srv_curr, srv_host); | ||
| } | ||
| else if (rr_type != T_CNAME) |
There was a problem hiding this comment.
Could you please explain why T_CNAME is excluded? A short comment in the code would be great.
(Same for CAA, MX, NAPTR, NS, PTR, SRV, TXT)
|
@bradh352 I've reworked the srv replies to use an array and an iterator. Let me know if you like that pattern better or if you like the old linked list pattern. |
|
Hi @kylebevans sorry for the late reply here, it may be a couple of weeks before I have time to review this. Thanks for doing this though, I'm sure we'll be pulling this in as I think it greatly modernizes the c-ares API... it just might take a while to get through since the patch set is absolutely huge :) |
|
That's fine! I appreciate all your help. |
|
Putting a note on this to attempt to resuscitate the pending |
|
@bradh352 are you still interested in this PR? I could try to get it to a finish if there is still a desire for it. I think the current question is whether to rewrite all the replies with the array/iterator pattern like the srv reply is now, or to keep the current api pattern. |
|
thanks for your work on this, but i ended up going an easier to maintain way in c-ares 1.21.0 with ares_dns_record.h. Probably in c-ares 1.22 I'll make these interfaces public. It would be nice to have feedback on this. |
This PR is in response to issue #387.
This adds a new private srv record struct with a ttl field.
It also adds public accessor functions to set all of the
fields. Finally, it replicates the ares_parse_srv_reply
function to use the new struct and accessors.
So far, I just added one test as a POC. If this is the right direction, I can add more tests as well as implement the other RR parsing functions.