On Thu, Nov 25, 2021 at 08:18:10PM +0100, Sebastian Benoit wrote: > Job Snijders(j...@openbsd.org) on 2021.11.25 16:13:51 +0000: > > It might be advantageous to permit operators to optionally specify the > > maximum number of publication points with which rpki-client will > > synchronize. > > > > For example: "doas rpki-client -m 1 -t /etc/rpki/ripe.tal" has as effect > > that only RIPE NCC's repository is contacted, but none of the delegated > > repositories. > > > > This flag perhaps permits us to start shipping with a more conservative > > default than 1000, like 100 or 200. It is clear that encouraging the > > ecosystem to embrace 'Publish in Parent' is a saner model than everyone > > running their own delegation. > > > > Thoughts? > > Yes, if we dont have these configurable, we will have users running into > them eventually and be sad.
I'm not a big fan of this config option. This is for sure not something that needs to change between runs and abusing it for the -m 1 case seems strange. Will we end up with a option salad for each and every limit? I think this is a bad direction. rpki-client should just work and the limits need to be chosen with that in mind. > maxpubpoints sounds a bit funny, but i have no other idea. > > code reads ok. The way the limit is implemented in this patch is not correct. More inline > > Index: main.c > > =================================================================== > > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v > > retrieving revision 1.166 > > diff -u -p -r1.166 main.c > > --- main.c 25 Nov 2021 14:03:40 -0000 1.166 > > +++ main.c 25 Nov 2021 16:08:10 -0000 > > @@ -69,6 +69,7 @@ int verbose; > > int noop; > > int rrdpon = 1; > > int repo_timeout = 15*60; > > +unsigned int maxpubpoints = 1000; > > > > struct stats stats; > > > > @@ -714,7 +715,7 @@ main(int argc, char *argv[]) > > "proc exec unveil", NULL) == -1) > > err(1, "pledge"); > > > > - while ((c = getopt(argc, argv, "b:Bcd:e:f:jnorRs:t:T:vV")) != -1) > > + while ((c = getopt(argc, argv, "b:Bcd:e:f:jm:norRs:t:T:vV")) != -1) > > switch (c) { > > case 'b': > > bind_addr = optarg; > > @@ -738,6 +739,11 @@ main(int argc, char *argv[]) > > case 'j': > > outformats |= FORMAT_JSON; > > break; > > + case 'm': > > + maxpubpoints = strtonum(optarg, 0, 100000, &errs); > > + if (errs) > > + errx(1, "-m: %s", errs); > > + break; Why allow 0 as minimum? Will that even work? > > case 'n': > > noop = 1; > > break; > > @@ -1220,7 +1226,7 @@ usage: > > fprintf(stderr, > > "usage: rpki-client [-BcjnoRrVv] [-b sourceaddr] [-d cachedir]" > > " [-e rsync_prog]\n" > > - " [-s timeout] [-T table] [-t tal]" > > - " [outputdir]\n"); > > + " [-m maxpubpoints] [-s timeout] [-T table] " > > + "[-t tal] [outputdir]\n"); > > return 1; > > } > > Index: repo.c > > =================================================================== > > RCS file: /cvs/src/usr.sbin/rpki-client/repo.c,v > > retrieving revision 1.14 > > diff -u -p -r1.14 repo.c > > --- repo.c 25 Nov 2021 14:03:40 -0000 1.14 > > +++ repo.c 25 Nov 2021 16:08:17 -0000 > > @@ -41,6 +41,7 @@ extern struct stats stats; > > extern int noop; > > extern int rrdpon; > > extern int repo_timeout; > > +extern unsigned int maxpubpoints; > > > > enum repo_state { > > REPO_LOADING = 0, > > @@ -1100,12 +1101,14 @@ ta_lookup(int id, struct tal *tal) > > if ((rp->repouri = strdup(tal->descr)) == NULL) > > err(1, NULL); > > > > - if (++talrepocnt[id] >= MAX_REPO_PER_TAL) { > > - if (talrepocnt[id] == MAX_REPO_PER_TAL) > > - warnx("too many repositories under %s", tals[id]); > > + if (talrepocnt[id] >= maxpubpoints + 1) { > > + if (talrepocnt[id] == maxpubpoints) This can not be right. If the first if is true the 2nd can never be true. > > + warnx("too many publication points under %s", tals[id]); > > nofetch = 1; > > } > > > > + talrepocnt[id]++; > > + > > rp->ta = ta_get(tal, nofetch); > > > > return rp; > > @@ -1146,11 +1149,12 @@ repo_lookup(int id, const char *uri, con > > if ((rp->notifyuri = strdup(notify)) == NULL) > > err(1, NULL); > > > > - if (++talrepocnt[id] >= MAX_REPO_PER_TAL) { > > - if (talrepocnt[id] == MAX_REPO_PER_TAL) > > - warnx("too many repositories under %s", tals[id]); > > + if (talrepocnt[id] >= maxpubpoints + 1) { > > + if (talrepocnt[id] == maxpubpoints) Same here. > > + warnx("too many publication points under %s", tals[id]); > > nofetch = 1; > > } > > + talrepocnt[id]++; > > > > /* try RRDP first if available */ > > if (notify != NULL) -- :wq Claudio