Re: rpki-client: add rsync baseuri-based batching

On Wed, Jul 08, 2026 at 10:23:16AM +0000, Job Snijders wrote:
> On Wed, Jul 08, 2026 at 11:57:13AM +0200, Theo Buehler wrote:
> > > Here is a refactor that introduces helpers for shortlist & skiplist. I
> > > don't think a 'len' member is needed if the needles used for search are
> > > right-sized.
> > 
> > I would prefer not strdup-ing and keeping the existing logic to be
> > honest. I also don't think this requires yet another file.
> 
> how about this? Pass a length into the search function.

I like this better. That more or less matches what I had in mind and I
think it will work for rsync as well.

Comments inline.

> 
> 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	8 Jul 2026 10:20:43 -0000
> @@ -29,11 +29,16 @@
>  
>  #define MAX_MSG_SIZE	(50 * 1024 * 1024)
>  
> -struct fqdnlistentry {
> -	LIST_ENTRY(fqdnlistentry)	entry;
> -	char				*fqdn;
> +struct strlistentry {
> +	LIST_ENTRY(strlistentry) entry;
> +	char *str;
> +	size_t str_len;
>  };
> -LIST_HEAD(fqdns, fqdnlistentry);
> +LIST_HEAD(strlist_head, strlistentry);

I would prefer omitting the _head here. I find such long struct names
hard to read and in this case it's rather unhelpful.

> +
> +void strlist_insert(struct strlist_head *, const char *);
> +int strlist_find(const struct strlist_head *, const char *, size_t);
> +void strlist_free(struct strlist_head *);

Do you anticipate using the strlist_free more than once? I think we can
do without this abstraction.

>  
>  enum cert_as_type {
>  	CERT_AS_ID, /* single identifier */
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> diff -u -p -r1.309 main.c
> --- main.c	24 Jun 2026 09:06:20 -0000	1.309
> +++ main.c	8 Jul 2026 10:20:44 -0000
> @@ -85,8 +85,49 @@ int64_t  evaluation_time = X509_TIME_MIN
>  
>  struct stats	 stats;
>  
> -struct fqdns shortlist = LIST_HEAD_INITIALIZER(fqdns);
> -struct fqdns skiplist = LIST_HEAD_INITIALIZER(fqdns);
> +static struct strlist_head shortlist = LIST_HEAD_INITIALIZER(shortlist);
> +static struct strlist_head skiplist = LIST_HEAD_INITIALIZER(skiplist);
> +
> +void
> +strlist_insert(struct strlist_head *strlist, const char *str)
> +{
> +	struct strlistentry *sle;
> +
> +	if ((sle = malloc(sizeof(*sle))) == NULL)

I would prefer calloc(), but others might disagre :)

> +		err(1, NULL);
> +
> +	if ((sle->str = strdup(str)) == NULL)
> +		err(1, NULL);
> +
> +	sle->str_len = strlen(sle->str);
> +
> +	LIST_INSERT_HEAD(strlist, sle, entry);
> +}
> +
> +int
> +strlist_find(const struct strlist_head *strlist, const char *str, size_t len)
> +{
> +	struct strlistentry *sle;
> +
> +	LIST_FOREACH(sle, strlist, entry) {
> +		if (sle->str_len == len && strncasecmp(str, sle->str, len) == 0)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +void
> +strlist_free(struct strlist_head *strlist)
> +{
> +	struct strlistentry *sle, *sle_tmp;
> +
> +	LIST_FOREACH_SAFE(sle, strlist, entry, sle_tmp) {
> +		LIST_REMOVE(sle, entry);
> +		free(sle->str);
> +		free(sle);
> +	}
> +}
>  
>  /*
>   * Log a message to stderr if and only if "verbose" is non-zero.
> @@ -509,7 +550,6 @@ static void
>  queue_add_from_cert(const struct cert *cert, struct nca_tree *ncas)
>  {
>  	struct repo		*repo;
> -	struct fqdnlistentry	*le;
>  	char			*nfile, *npath, *host;
>  	const char		*uri, *repouri, *file;
>  	size_t			 hostsz, repourisz;
> @@ -520,21 +560,13 @@ queue_add_from_cert(const struct cert *c
>  	host = cert->repo + RSYNC_PROTO_LEN;
>  	hostsz = strcspn(host, "/");
>  
> -	LIST_FOREACH(le, &skiplist, entry) {
> -		if (strlen(le->fqdn) == hostsz &&
> -		    strncasecmp(host, le->fqdn, hostsz) == 0) {
> -			warnx("skipping %s (listed in skiplist)", cert->repo);
> -			return;
> -		}
> +	if (strlist_find(&skiplist, host, hostsz)) {
> +		warnx("skipping %s (listed in skiplist)", cert->repo);
> +		return;
>  	}
>  
> -	LIST_FOREACH(le, &shortlist, entry) {
> -		if (strlen(le->fqdn) == hostsz &&
> -		    strncasecmp(host, le->fqdn, hostsz) == 0) {
> -			shortlisted = 1;
> -			break;
> -		}
> -	}
> +	shortlisted = strlist_find(&shortlist, host, hostsz);
> +

I think we can drop shortlisted and do:

  	if (shortlistmode && !strlist_find(&shortlist, host, hostsz)) {

>  	if (shortlistmode && shortlisted == 0) {
>  		if (verbose)
>  			warnx("skipping %s (not shortlisted)", cert->repo);
> @@ -881,7 +913,6 @@ tal_load_default(void)
>  static void
>  load_skiplist(const char *slf)
>  {
> -	struct fqdnlistentry	*le;
>  	FILE			*fp;
>  	char			*line = NULL;
>  	size_t			 linesize = 0, linelen;
> @@ -910,12 +941,8 @@ load_skiplist(const char *slf)
>  		if (!valid_uri(line, linelen, NULL))
>  			errx(1, "invalid entry in skiplist: %s", line);
>  
> -		if ((le = malloc(sizeof(struct fqdnlistentry))) == NULL)
> -			err(1, NULL);
> -		if ((le->fqdn = strdup(line)) == NULL)
> -			err(1, NULL);
> +		strlist_insert(&skiplist, line);
>  

I'd drop the extra empty line.

> -		LIST_INSERT_HEAD(&skiplist, le, entry);
>  		stats.skiplistentries++;
>  	}
>  	if (ferror(fp))
> @@ -931,18 +958,11 @@ load_skiplist(const char *slf)
>  static void
>  load_shortlist(const char *fqdn)
>  {
> -	struct fqdnlistentry	*le;
> -
>  	if (!valid_uri(fqdn, strlen(fqdn), NULL))
>  		errx(1, "invalid fqdn passed to -q: %s", fqdn);
>  
> -	if ((le = malloc(sizeof(struct fqdnlistentry))) == NULL)
> -		err(1, NULL);
> -
> -	if ((le->fqdn = strdup(fqdn)) == NULL)
> -		err(1, NULL);
> +	strlist_insert(&shortlist, fqdn);
>  

zap empty line

> -	LIST_INSERT_HEAD(&shortlist, le, entry);
>  }
>  
>  static void
> Index: nca.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/nca.c,v
> diff -u -p -r1.8 nca.c
> --- nca.c	8 Jul 2026 05:33:28 -0000	1.8
> +++ nca.c	8 Jul 2026 10:20:44 -0000
> @@ -123,7 +123,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 struct strlist_head batchlist = LIST_HEAD_INITIALIZER(batchlist);
>  
>  static RB_HEAD(nca_hist_tree, nca_hist) ncas_hist = RB_INITIALIZER(&ncas_hist);
>  
> @@ -229,8 +229,6 @@ static void
>  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) {
> @@ -241,36 +239,21 @@ ncas_plan_retries(void)
>  		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);
> -
> -		LIST_INSERT_HEAD(&notifys, fle, entry);
> +		strlist_insert(&batchlist, nca_hist->notify);

Fine to leave it as-is, but I wonder if the logic should not be inverted
to be the shorter:

		if (nca_hist->notify != NULL)
			strlist_insert(&batchlist, nca_hist->notify);

Perhaps reconsider this once the rsync batching tweak has taken proper
shape.

>  	}
>  
>  	RB_FOREACH(nca_hist, nca_hist_tree, &ncas_hist) {
>  		if (nca_hist->notify == NULL)
>  			continue;
>  
> -		notify_len = strlen(nca_hist->notify);
> -
> -		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 (strlist_find(&batchlist, nca_hist->notify,
> +		    strlen(nca_hist->notify))) {
> +			nca_hist->defer = 0;
> +			break;
>  		}
>  	}
>  
> -	LIST_FOREACH_SAFE(fle, &notifys, entry, fle_tmp) {
> -		LIST_REMOVE(fle, entry);
> -		free(fle->fqdn);
> -		free(fle);
> -	}
> +	strlist_free(&batchlist);

As noted towards the top, I don't really see much of a win here.

>  }
>  
>  void