[PATCH] resolv_wrapper - support multiple URI answers
Jakub Hrozek
jakub.hrozek at posteo.se
Mon Aug 29 19:12:00 UTC 2016
On Mon, Aug 29, 2016 at 12:26:04PM -0400, Matt Rogers wrote:
> 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.
Thank you, the patches now look good to me!
>
> 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?
resolv_wrapper ships a pkg-config file that contains the version. Could
you run 'pkg-config --atleast-version=1.1.5 resolv_wrapper' to find if
the version is the one you'd expect? (I know, checking for versions is
frowned upon..)
More information about the samba-technical
mailing list