[PATCH] resolv_wrapper - support multiple URI answers
Matt Rogers
mrogers at redhat.com
Mon Aug 29 16:26:04 UTC 2016
On Mon, 2016-08-29 at 09:56 +0200, Jakub Hrozek wrote:
> On Fri, Aug 26, 2016 at 10:30:21AM -0400, Matt Rogers wrote:
> >
> > On Fri, 2016-08-26 at 08:23 +0200, Andreas Schneider wrote:
> > >
> > > On Wednesday, 24 August 2016 10:53:17 CEST Matt Rogers wrote:
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > Another issue is that it can only return a single answer
> > > > > > for a
> > > > > > URI
> > > > > > query, since it reaches the first matching URI key in the
> > > > > > host
> > > > > > file
> > > > > > and
> > > > > > stops. The krb5 URI lookup collects all answers from a
> > > > > > query
> > > > > > andres
> > > > > > parses
> > > > > > the URIs, sorting the results accordingly, so it would be
> > > > > > great
> > > > > > to
> > > > > > be
> > > > > > able to have a fake hosts file with all of the test URIs
> > > > > > listed
> > > > > > and
> > > > > > not
> > > > > > have to test them one by one.
> > > > >
> > > > > We are happy to accept patches to allow to return more than
> > > > > one
> > > > > URI
> > > > >
> > > > > :)
> > > >
> > > > Good, I'll be working on that :)
> > >
> >
> > The patches I submitted for this before are in the wrong patch
> > format,
> > so here are the new ones.
> >
> > Regards,
> > Matt
>
> Hi Matt,
>
> the patches work OK, but I wonder if we can make them even better by
> simplifying the code a bit. Please note I haven't actually tried
> that,
> just an idea based on reading the code..
>
> >
> > From 803d9eec1bc731000c65f3a93cc62ba096be3d5d Mon Sep 17 00:00:00
> > 2001
> > From: Matt Rogers <mrogers at redhat.com>
> > Date: Thu, 25 Aug 2016 12:57:09 -0400
> > Subject: [PATCH 1/2] Support multiple URI answers
> >
> > Add URI specific recursion to rwrap_get_record(), for collecting
> > multiple URI answers under a single query. Add RWRAP_MAX_RECORDS
> > to
> > increase the number of allowed answers and to seperate it from the
> > SRV
> > and CNAME recursion limit.
> >
> > Signed-off-by: Matt Rogers <mrogers at redhat.com>
> > ---
> > src/resolv_wrapper.c | 74
> > +++++++++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 59 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/resolv_wrapper.c b/src/resolv_wrapper.c
> > index b3a3e15..6698f33 100644
> > --- a/src/resolv_wrapper.c
> > +++ b/src/resolv_wrapper.c
> > @@ -152,6 +152,7 @@ static void rwrap_log(enum rwrap_dbglvl_e
> > dbglvl,
> > } while(0);
> >
> > #define RWRAP_MAX_RECURSION 5
> > +#define RWRAP_MAX_RECORDS 64
>
> If you needed two variables because MAX_RECURSION is much lower than
> MAX_RECORDS, then I guess it would be fine to define MAX_RECURSION to
> the same value as MAX_RECORDS, the idea behind MAX_RECURSION was to
> avoid
> depleting the stack..and MAX_RECORDS is still quite small, so
> recursing
> up to that value would be OK I think.
>
>
> [...]
>
> >
> > static int rwrap_get_record(const char *hostfile, unsigned
> > recursion,
> > - const char *query, int type,
> > - struct rwrap_fake_rr *rr)
> > + unsigned uri_recurse, const char
> > *query, int type,
> > + struct rwrap_fake_rr *rr)
>
> I wonder if we need the separate uri_recurse variable? Wouldn't it be
> enough to use the recursion variable for tracking the nesting level
> and
> special-case URI recursion by examining the type variable value?
>
> >
> > {
> > FILE *fp = NULL;
> > char buf[BUFSIZ];
> > char *key = NULL;
> > char *value = NULL;
> > int rc = ENOENT;
> > + unsigned uris = 0;
> >
> > if (recursion >= RWRAP_MAX_RECURSION) {
> > RWRAP_LOG(RWRAP_LOG_ERROR, "Recursed too
> > deep!\n");
> > return -1;
> > }
> >
> > + if (uri_recurse >= RWRAP_MAX_RECORDS) {
> > + return -1;
> > + }
> > +
>
> ..then maybe we could drop this line altogether
>
> >
> > RWRAP_LOG(RWRAP_LOG_TRACE,
> > "Searching in fake hosts file %s for %s:%d\n",
> > hostfile,
> > query, type);
> > @@ -829,6 +846,18 @@ static int rwrap_get_record(const char
> > *hostfile, unsigned recursion,
> > }
> > q[0] = '\0';
> >
> > + if (uri_recurse != 0) {
>
> ..and perhaps we could use to check if type == ns_t_uri && recurse >
> 0
>
> >
> > + /* Skip non-URI records. */
> > + if (!TYPE_MATCH(type, ns_t_uri, rec_type,
> > "URI", key, query)) {
> > + continue;
> > + }
> > + /* Skip previous records based on the
> > recurse depth. */
> > + uris++;
> > + if (uris <= uri_recurse) {
> > + continue;
> > + }
> > + }
> > +
> > if (TYPE_MATCH(type, ns_t_a, rec_type, "A", key,
> > query)) {
> > rc = rwrap_create_fake_a_rr(key, value,
> > rr);
> > break;
> > @@ -852,6 +881,10 @@ static int rwrap_get_record(const char
> > *hostfile, unsigned recursion,
> > } else if (TYPE_MATCH(type, ns_t_uri,
> > rec_type, "URI", key,
> > query)) {
> > rc = rwrap_create_fake_uri_rr(key, value,
> > rr);
> > + if (rc == 0) {
> > + /* Recurse to collect multiple URI
> > answers under a single key. */
> > + rc = rwrap_uri_recurse(hostfile,
> > uri_recurse + 1, key, rr + 1);
>
> ..and similarly here just use recurse instead of uri_recurse.
Thanks for the feedback! I did have the impression from MAX_RECURSION
that it was specific to an allowed amount of CNAME recursion, and the
URIs needed more than that, so I thought to keep them seperate. But if
that's not an issue then I'm good with simplifying things like you
suggest. I've attached the revised patch and verified it with the
tests.
One side question, the krb5 test suite will need to run its URI tests
based on having the resolv_wrapper version with these patches, and no
earlier. The library only seems to have __res_search() available for an
autoconf version check; Is there some other way that we can reliably
check for the version?
Regards,
Matt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-multiple-URI-answers.patch
Type: text/x-patch
Size: 3321 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160829/9d626e30/0001-Support-multiple-URI-answers.bin>
More information about the samba-technical
mailing list