Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a function to parse a srv reply with ttl #393

Closed
wants to merge 51 commits into from

Conversation

kylebevans
Copy link

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.

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.
@kylebevans
Copy link
Author

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.

@coveralls
Copy link

coveralls commented Jan 25, 2021

Coverage Status

Coverage decreased (-0.3%) to 88.371% when pulling 7eae6d8 on kylebevans:kylebevans-ttl into fd890e9 on c-ares:master.

@bradh352
Copy link
Member

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.

@kylebevans
Copy link
Author

kylebevans commented Jan 29, 2021

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?

@bradh352
Copy link
Member

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.

src/lib/cares_reply.c Outdated Show resolved Hide resolved
@kylebevans
Copy link
Author

kylebevans commented Jan 30, 2021

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.

@bagder
Copy link
Member

bagder commented Jan 30, 2021

Note that two sanitizer builds fail tests now.

src/lib/cares_reply.c Outdated Show resolved Hide resolved
@bradh352
Copy link
Member

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.

Comment on lines +163 to +168
aptr += rr_len;
if (aptr > abuf + alen)
{ /* LCOV_EXCL_START: already checked above */
status = ARES_EBADRESP;
break;
} /* LCOV_EXCL_STOP */
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some parse functions have this check after moving aptr, and some don't. Should all of them have it?

Comment on lines +26 to +29
const char* cares_srv_reply_get_host(const cares_srv_reply* srv_reply)
{
return srv_reply->host;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@kylebevans
Copy link
Author

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@kylebevans kylebevans requested a review from bradh352 May 7, 2021 19:47
@kylebevans
Copy link
Author

@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.

@bradh352
Copy link
Member

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 :)

@kylebevans
Copy link
Author

That's fine! I appreciate all your help.

@bassmandja
Copy link

Putting a note on this to attempt to resuscitate the pending srv_dns extension cluster in Envoy

@kylebevans
Copy link
Author

@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.

@bradh352
Copy link
Member

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.

@bradh352 bradh352 closed this Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants