From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: add rsync baseuri-based batching
To: Job Snijders <job@bsd.nl>
Cc: tech@openbsd.org
Date: Tue, 7 Jul 2026 20:06:40 +0200

On Tue, Jul 07, 2026 at 03:41:15PM +0000, Job Snijders wrote:
> On Tue, Jul 07, 2026 at 12:09:12PM +0000, Job Snijders wrote:
> > Question: I'm not sure the newly added calls to rsync_base_uri()
> > could ever fail, because nca_history_load() will already have called
> > rsync_base_uri() via valid_uri() on those same C strings, so perhaps
> > instead of 'continue' I should use an errx()? Or something else?
> 
> Here is a better version: no duplicate calls to rsync_base_uri() and
> more symmetry with rpkiNotify batching. Feedback? OK?
> 
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> diff -u -p -r1.287 extern.h
> --- extern.h	1 Jul 2026 11:09:12 -0000	1.287
> +++ extern.h	7 Jul 2026 15:40:47 -0000
> @@ -159,6 +159,7 @@ struct nca_hist {
>  	char			*ski;
>  	char			*location;
>  	char			*mfturi;
> +	char			*baseuri;
>  	char			*notify;
>  	time_t			 since;
>  	time_t			 last_attempt;
> Index: nca.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/nca.c,v
> diff -u -p -r1.7 nca.c
> --- nca.c	1 Jul 2026 11:09:12 -0000	1.7
> +++ nca.c	7 Jul 2026 15:40:47 -0000
> @@ -31,6 +31,8 @@
>  
>  #include "extern.h"
>  
> +extern int rrdpon;
> +
>  /*
>   * Add a given CA cert into the non-functional CA tree.
>   * Return 1 if a synchronization attempt is to be made, 0 otherwise.
> @@ -123,7 +125,7 @@ certidcmp(const struct nonfunc_ca *a, co
>  
>  RB_GENERATE(nca_tree, nonfunc_ca, entry, certidcmp);
>  
> -static LIST_HEAD(, fqdnlistentry) notifys = LIST_HEAD_INITIALIZER(notifys);
> +static LIST_HEAD(, fqdnlistentry) batchlist = LIST_HEAD_INITIALIZER(batchlist);

Make this static struct fqdns please.

>  
>  static RB_HEAD(nca_hist_tree, nca_hist) ncas_hist = RB_INITIALIZER(&ncas_hist);
>  
> @@ -220,6 +222,37 @@ nca_decide_retry(const struct nca_hist *
>  	return 0;
>  }
>  
> +static void
> +insert_in_batchlist(const char *uri)
> +{
> +	struct fqdnlistentry *fle;
> +
> +	if ((fle = malloc(sizeof(*fle))) == NULL)
> +		err(1, NULL);
> +
> +	if ((fle->fqdn = strdup(uri)) == NULL)
> +		err(1, NULL);
> +
> +	LIST_INSERT_HEAD(&batchlist, fle, entry);
> +}
> +
> +static int
> +find_in_batchlist(const char *uri)
> +{
> +	struct fqdnlistentry *fle;
> +	size_t uri_len;
> +
> +	uri_len = strlen(uri);
> +	LIST_FOREACH(fle, &batchlist, entry) {
> +		if (strlen(fle->fqdn) == uri_len &&
> +		    strncasecmp(uri, fle->fqdn, uri_len) == 0) {
> +			return 1;
> +		}
> +	}

Why not simply use:
	LIST_FOREACH(fle, &batchlist, entry) {
		if (strcasecmp(uri, fle->fqdn,fle->fqdn) == 0)
			return 1;
	}
	return 0;

I don't understand why the strlen are needed here.

> +
> +	return 0;
> +}
> +
>  /*
>   * Determine which non-functioncal CAs are eligible for retry.
>   * If multiple NCAs point to the same RRDP repo and at least one NCA is eligible
> @@ -230,7 +263,6 @@ ncas_plan_retries(void)
>  {
>  	struct nca_hist *nca_hist;
>  	struct fqdnlistentry *fle, *fle_tmp;
> -	size_t notify_len;
>  
>  	RB_FOREACH(nca_hist, nca_hist_tree, &ncas_hist) {
>  		if (nca_decide_retry(nca_hist) == 0) {
> @@ -238,35 +270,24 @@ ncas_plan_retries(void)
>  			continue;
>  		}
>  
> -		if (nca_hist->notify == NULL)
> -			continue;
> -
> -		if ((fle = malloc(sizeof(*fle))) == NULL)
> -			err(1, NULL);
> -
> -		if ((fle->fqdn = strdup(nca_hist->notify)) == NULL)
> -			err(1, NULL);
> +		insert_in_batchlist(nca_hist->baseuri);
>  
> -		LIST_INSERT_HEAD(&notifys, fle, entry);
> +		if (nca_hist->notify != NULL && rrdpon)
> +			insert_in_batchlist(nca_hist->notify);
>  	}
>  
>  	RB_FOREACH(nca_hist, nca_hist_tree, &ncas_hist) {
> -		if (nca_hist->notify == NULL)
> -			continue;
> +		if (find_in_batchlist(nca_hist->baseuri))
> +			nca_hist->defer = 0;
>  
> -		notify_len = strlen(nca_hist->notify);
> +		if (nca_hist->notify == NULL || !rrdpon || !nca_hist->defer)
> +			continue;
>  
> -		LIST_FOREACH(fle, &notifys, entry) {
> -			if (strlen(fle->fqdn) == notify_len &&
> -			    strncasecmp(nca_hist->notify, fle->fqdn,
> -			    notify_len) == 0) {
> -				nca_hist->defer = 0;
> -				break;
> -			}
> -		}
> +		if (find_in_batchlist(nca_hist->notify))
> +			nca_hist->defer = 0;
>  	}
>  
> -	LIST_FOREACH_SAFE(fle, &notifys, entry, fle_tmp) {
> +	LIST_FOREACH_SAFE(fle, &batchlist, entry, fle_tmp) {
>  		LIST_REMOVE(fle, entry);
>  		free(fle->fqdn);
>  		free(fle);
> @@ -364,6 +385,8 @@ nca_history_load(void)
>  			goto err;
>  		if ((nca_hist->mfturi = strdup(mfturi)) == NULL)
>  			err(1, NULL);
> +		if (!rsync_base_uri(mfturi, &nca_hist->baseuri))
> +			goto err;

I would expect this to be done before the strdup(). Any reason why this
happens after?

>  
>  		notify = l;
>  		if (notify == NULL)
> 

-- 
:wq Claudio