[PATCH] resolv_wrapper - support multiple URI answers

[Jakub Hrozek]

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.