From: Theo Buehler Subject: Re: rpki-client: add rsync baseuri-based batching To: Job Snijders Cc: tech@openbsd.org Date: Wed, 8 Jul 2026 14:26:41 +0200 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(¬ifys, 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, ¬ifys, 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, ¬ifys, 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