From: Theo Buehler <tb@theobuehler.org>
Subject: Re: rpki-client: refactor non-functional CA metrics accounting
To: Job Snijders <job@bsd.nl>
Cc: tech@openbsd.org
Date: Mon, 22 Jun 2026 08:10:55 +0200

On Mon, Jun 22, 2026 at 07:44:44AM +0200, Theo Buehler wrote:
> On Sun, Jun 21, 2026 at 09:47:13PM +0000, Job Snijders wrote:
> > Rpki-client detects non-functional CAs by initially marking every CA as
> > non-functional, and then later on (after a valid manifest is discovered)
> > clearing that mark. Statistics were done along the same lines: first
> > increment and later on substract for all functional ones. This resulted
> > in rather unwieldy code, e.g., a repo pointer needed to be hoisted
> > around which is getting in the way of upcoming work.
> >
> > The below diff simplifies the dance described above, and also fixes a
> > subtle defect: non-functional CAs were counted towards the repository
> > those broken CAs were pointing towards, instead of being counted
> > towards the repository that contained the broken CA. Attributing the
> > non-functional CA to the issuing parent makes more sense to me, because
> > the issuing parent can actually do something about it, for example by
> > revoking the broken child.
> 
> I don't think this description is very on point. There is no unwieldy
> code being changed nor any real simplification being made. Your diff
> adds more code than it removes (the removed code is entirely trivial)
> and your code walks more trees and lists.
> 
> That said, I agree that the new accounting is better, so that bug fix
> is what your description in the commit message should focus on.
> 
> > In short, I think this diff helps better show which repository is
> > responsible for non-functional CAs. See the metrics output that follows.
> >
> > OK?
> 
> STYPE_NONFUNC and STYPE_FUNC need to be garbage collected as well.
> 
> I'm not sure repo_stat_inc_nca() is a very good name for the new
> function but I can live with it.
> 
> ok
> 

Accidentally dropped two review comments

> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> diff -u -p -r1.306 main.c
> --- main.c	9 May 2026 01:22:32 -0000	1.306
> +++ main.c	21 Jun 2026 21:21:26 -0000
> @@ -575,7 +575,8 @@ queue_add_from_cert(const struct cert *c
>  			err(1, NULL);
>  	}
>  
> -	cert_insert_nca(ncas, cert, repo);
> +	cert_insert_nca(ncas, cert);
> +
>  	entityq_add(npath, nfile, RTYPE_MFT, DIR_UNKNOWN, repo, NULL, 0,
>  	    cert->talid, cert->certid, NULL);
>  }
> @@ -675,7 +676,7 @@ entity_process(struct ibuf *b, struct va
>  		if (mft->seqnum_gap)
>  			repo_stat_inc(rp, talid, type, STYPE_SEQNUM_GAP);
>  		queue_add_from_mft(mft);
> -		cert_remove_nca(&vd->ncas, mft->certid, rp);
> +		cert_remove_nca(&vd->ncas, mft->certid);
>  		ccr_insert_mft(&vd->ccr.mfts, mft);
>  		mft_free(mft);
>  		break;
> @@ -1030,6 +1031,7 @@ main(int argc, char *argv[])
>  	struct rusage	 ru;
>  	struct timespec	 start_time, now_time;
>  	struct validation_data vd = { 0 };
> +	struct nonfunc_ca	*nca;

I would just not indent the variable like vd:

	struct nonfunc_ca *nca;

>  
>  	clock_gettime(CLOCK_MONOTONIC, &start_time);
>  
> @@ -1538,6 +1540,12 @@ main(int argc, char *argv[])
>  	/* if processing in filemode the process is done, no cleanup */
>  	if (filemode)
>  		return rc;
> +
> +	/*
> +	 * Account non-functional CAs statistics.
> +	 */

"Collect" would be a better word than "account". I'd drop the comment
or at least turn it into a single line. This isn't super important or
non-obvious.

> +	RB_FOREACH(nca, nca_tree, &vd.ncas)
> +		repo_stat_inc_nca(nca);
>  
>  	logx("all files parsed: generating output");
>