From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: pass around a container struct instead of a long parameter list
To: Job Snijders <job@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 8 Jul 2025 14:52:20 +0200

On Tue, Jul 08, 2025 at 12:44:12PM +0000, Job Snijders wrote:
> I think it is fine to continue to pass around 'everything' into the
> various output functions - even when a particular function doesn't use a
> one or more of the available trees; BUT, it seems tedious that every
> time we add or remove a feature, every output function's signature also
> has to change.
> 
> Perhaps it is better to use a struct that contains references to the
> various trees and pass that 'container' around. I believe this kind of
> refactor will help avoid certain churn in future work.
> 
> Thoughts?

Please call it struct container or context or so and not struct trees.
Apart from that OK. 

> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> diff -u -p -r1.243 extern.h
> --- extern.h	30 Jun 2025 14:20:26 -0000	1.243
> +++ extern.h	8 Jul 2025 12:40:46 -0000
> @@ -501,6 +501,14 @@ struct brk {
>  RB_HEAD(brk_tree, brk);
>  RB_PROTOTYPE(brk_tree, brk, entry, brkcmp);
>  
> +struct trees {
> +	struct vrp_tree	vrps;
> +	struct brk_tree	brks;
> +	struct vap_tree	vaps;
> +	struct vsp_tree	vsps;
> +	struct nca_tree ncas;
> +};
> +
>  /*
>   * A single CRL
>   */
> @@ -995,25 +1003,13 @@ extern int	 outformats;
>  #define FORMAT_JSON	0x08
>  #define FORMAT_OMETRIC	0x10
>  
> -int		 outputfiles(struct vrp_tree *v, struct brk_tree *b,
> -		    struct vap_tree *, struct vsp_tree *, struct nca_tree *,
> -		    struct stats *);
> +int		 outputfiles(struct trees *, struct stats *);
>  int		 outputheader(FILE *, struct stats *);
> -int		 output_bgpd(FILE *, struct vrp_tree *, struct brk_tree *,
> -		    struct vap_tree *, struct vsp_tree *, struct nca_tree *,
> -		    struct stats *);
> -int		 output_bird(FILE *, struct vrp_tree *, struct brk_tree *,
> -		    struct vap_tree *, struct vsp_tree *, struct nca_tree *,
> -		    struct stats *);
> -int		 output_csv(FILE *, struct vrp_tree *, struct brk_tree *,
> -		    struct vap_tree *, struct vsp_tree *, struct nca_tree *,
> -		    struct stats *);
> -int		 output_json(FILE *, struct vrp_tree *, struct brk_tree *,
> -		    struct vap_tree *, struct vsp_tree *, struct nca_tree *,
> -		    struct stats *);
> -int		 output_ometric(FILE *, struct vrp_tree *, struct brk_tree *,
> -		    struct vap_tree *, struct vsp_tree *, struct nca_tree *,
> -		    struct stats *);
> +int		 output_bgpd(FILE *, struct trees *, struct stats *);
> +int		 output_bird(FILE *, struct trees *, struct stats *);
> +int		 output_csv(FILE *, struct trees *, struct stats *);
> +int		 output_json(FILE *, struct trees *, struct stats *);
> +int		 output_ometric(FILE *, struct trees *, struct stats *);
>  
>  void		 logx(const char *fmt, ...)
>  		    __attribute__((format(printf, 1, 2)));
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> diff -u -p -r1.284 main.c
> --- main.c	26 Jun 2025 06:00:32 -0000	1.284
> +++ main.c	8 Jul 2025 12:40:46 -0000
> @@ -561,9 +561,7 @@ queue_add_from_cert(const struct cert *c
>   * In all cases, we gather statistics.
>   */
>  static void
> -entity_process(struct ibuf *b, struct stats *st, struct vrp_tree *tree,
> -    struct brk_tree *brktree, struct vap_tree *vaptree,
> -    struct vsp_tree *vsptree, struct nca_tree *ncatree)
> +entity_process(struct ibuf *b, struct stats *st, struct trees *trees)
>  {
>  	enum rtype	 type;
>  	struct tal	*tal;
> @@ -620,10 +618,10 @@ entity_process(struct ibuf *b, struct st
>  		switch (cert->purpose) {
>  		case CERT_PURPOSE_TA:
>  		case CERT_PURPOSE_CA:
> -			queue_add_from_cert(cert, ncatree);
> +			queue_add_from_cert(cert, &trees->ncas);
>  			break;
>  		case CERT_PURPOSE_BGPSEC_ROUTER:
> -			cert_insert_brks(brktree, cert);
> +			cert_insert_brks(&trees->brks, cert);
>  			repo_stat_inc(rp, talid, type, STYPE_BGPSEC);
>  			break;
>  		default:
> @@ -642,7 +640,7 @@ entity_process(struct ibuf *b, struct st
>  		if (mft->seqnum_gap)
>  			repo_stat_inc(rp, talid, type, STYPE_SEQNUM_GAP);
>  		queue_add_from_mft(mft);
> -		cert_remove_nca(ncatree, mft->certid, rp);
> +		cert_remove_nca(&trees->ncas, mft->certid, rp);
>  		mft_free(mft);
>  		break;
>  	case RTYPE_CRL:
> @@ -657,7 +655,7 @@ entity_process(struct ibuf *b, struct st
>  		}
>  		roa = roa_read(b);
>  		if (roa->valid)
> -			roa_insert_vrps(tree, roa, rp);
> +			roa_insert_vrps(&trees->vrps, roa, rp);
>  		else
>  			repo_stat_inc(rp, talid, type, STYPE_INVALID);
>  		roa_free(roa);
> @@ -672,7 +670,7 @@ entity_process(struct ibuf *b, struct st
>  		}
>  		aspa = aspa_read(b);
>  		if (aspa->valid)
> -			aspa_insert_vaps(file, vaptree, aspa, rp);
> +			aspa_insert_vaps(file, &trees->vaps, aspa, rp);
>  		else
>  			repo_stat_inc(rp, talid, type, STYPE_INVALID);
>  		aspa_free(aspa);
> @@ -686,7 +684,7 @@ entity_process(struct ibuf *b, struct st
>  		}
>  		spl = spl_read(b);
>  		if (spl->valid)
> -			spl_insert_vsps(vsptree, spl, rp);
> +			spl_insert_vsps(&trees->vsps, spl, rp);
>  		else
>  			repo_stat_inc(rp, talid, type, STYPE_INVALID);
>  		spl_free(spl);
> @@ -989,16 +987,18 @@ main(int argc, char *argv[])
>  	const char	*cachedir = NULL, *outputdir = NULL;
>  	const char	*errs, *name;
>  	const char	*skiplistfile = NULL;
> -	struct vrp_tree	 vrps = RB_INITIALIZER(&vrps);
> -	struct vsp_tree	 vsps = RB_INITIALIZER(&vsps);
> -	struct brk_tree	 brks = RB_INITIALIZER(&brks);
> -	struct vap_tree	 vaps = RB_INITIALIZER(&vaps);
> -	struct nca_tree	 ncas = RB_INITIALIZER(&ncas);
> +	struct trees	 trees;
>  	struct rusage	 ru;
>  	struct timespec	 start_time, now_time;
>  
>  	clock_gettime(CLOCK_MONOTONIC, &start_time);
>  
> +	RB_INIT(&trees.vrps);
> +	RB_INIT(&trees.brks);
> +	RB_INIT(&trees.vaps);
> +	RB_INIT(&trees.vsps);
> +	RB_INIT(&trees.ncas);
> +
>  	/* If started as root, priv-drop to _rpki-client */
>  	if (getuid() == 0) {
>  		struct passwd *pw;
> @@ -1414,8 +1414,7 @@ main(int argc, char *argv[])
>  				errx(1, "ibuf_read: connection closed");
>  			}
>  			while ((b = io_buf_get(queues[0])) != NULL) {
> -				entity_process(b, &stats, &vrps, &brks, &vaps,
> -				    &vsps, &ncas);
> +				entity_process(b, &stats, &trees);
>  				ibuf_free(b);
>  			}
>  		}
> @@ -1508,7 +1507,7 @@ main(int argc, char *argv[])
>  	}
>  	repo_stats_collect(sum_repostats, &stats.repo_stats);
>  
> -	if (outputfiles(&vrps, &brks, &vaps, &vsps, &ncas, &stats))
> +	if (outputfiles(&trees, &stats))
>  		rc = 1;
>  
>  	printf("Processing time %lld seconds "
> Index: output-bgpd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/output-bgpd.c,v
> diff -u -p -r1.33 output-bgpd.c
> --- output-bgpd.c	27 Mar 2025 05:03:09 -0000	1.33
> +++ output-bgpd.c	8 Jul 2025 12:40:46 -0000
> @@ -20,9 +20,7 @@
>  #include "extern.h"
>  
>  int
> -output_bgpd(FILE *out, struct vrp_tree *vrps, struct brk_tree *brks,
> -    struct vap_tree *vaps, struct vsp_tree *vsps, struct nca_tree *ncas,
> -    struct stats *st)
> +output_bgpd(FILE *out, struct trees *trees, struct stats *st)
>  {
>  	struct vrp	*vrp;
>  	struct vap	*vap;
> @@ -34,7 +32,7 @@ output_bgpd(FILE *out, struct vrp_tree *
>  	if (fprintf(out, "roa-set {\n") < 0)
>  		return -1;
>  
> -	RB_FOREACH(vrp, vrp_tree, vrps) {
> +	RB_FOREACH(vrp, vrp_tree, &trees->vrps) {
>  		char ipbuf[64], maxlenbuf[100];
>  
>  		ip_addr_print(&vrp->addr, vrp->afi, ipbuf, sizeof(ipbuf));
> @@ -58,7 +56,7 @@ output_bgpd(FILE *out, struct vrp_tree *
>  
>  	if (fprintf(out, "\naspa-set {\n") < 0)
>  		return -1;
> -	RB_FOREACH(vap, vap_tree, vaps) {
> +	RB_FOREACH(vap, vap_tree, &trees->vaps) {
>  		if (vap->overflowed)
>  			continue;
>  		if (fprintf(out, "\tcustomer-as %d expires %lld "
> Index: output-bird.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/output-bird.c,v
> diff -u -p -r1.23 output-bird.c
> --- output-bird.c	27 Mar 2025 05:03:09 -0000	1.23
> +++ output-bird.c	8 Jul 2025 12:40:46 -0000
> @@ -21,9 +21,7 @@
>  #include "extern.h"
>  
>  int
> -output_bird(FILE *out, struct vrp_tree *vrps, struct brk_tree *brks,
> -    struct vap_tree *vaps, struct vsp_tree *vsps, struct nca_tree *ncas,
> -    struct stats *st)
> +output_bird(FILE *out, struct trees *trees, struct stats *st)
>  {
>  	struct vrp	*v;
>  	struct vap	*vap;
> @@ -49,7 +47,7 @@ output_bird(FILE *out, struct vrp_tree *
>  	    "\troa4 { table ROAS4; };\n\n") < 0)
>  		return -1;
>  
> -	RB_FOREACH(v, vrp_tree, vrps) {
> +	RB_FOREACH(v, vrp_tree, &trees->vrps) {
>  		char buf[64];
>  
>  		if (v->afi == AFI_IPV4) {
> @@ -64,7 +62,7 @@ output_bird(FILE *out, struct vrp_tree *
>  	    "\troa6 { table ROAS6; };\n\n") < 0)
>  		return -1;
>  
> -	RB_FOREACH(v, vrp_tree, vrps) {
> +	RB_FOREACH(v, vrp_tree, &trees->vrps) {
>  		char buf[64];
>  
>  		if (v->afi == AFI_IPV6) {
> @@ -85,7 +83,7 @@ output_bird(FILE *out, struct vrp_tree *
>  	    "};\n\n") < 0)
>  		return -1;
>  
> -	RB_FOREACH(vap, vap_tree, vaps) {
> +	RB_FOREACH(vap, vap_tree, &trees->vaps) {
>  		if (vap->overflowed)
>  			continue;
>  		if (fprintf(out, "\troute aspa %d providers ",
> Index: output-csv.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/output-csv.c,v
> diff -u -p -r1.15 output-csv.c
> --- output-csv.c	27 Mar 2025 05:03:09 -0000	1.15
> +++ output-csv.c	8 Jul 2025 12:40:46 -0000
> @@ -20,16 +20,14 @@
>  #include "extern.h"
>  
>  int
> -output_csv(FILE *out, struct vrp_tree *vrps, struct brk_tree *brks,
> -    struct vap_tree *vaps, struct vsp_tree *vsps, struct nca_tree *ncas,
> -    struct stats *st)
> +output_csv(FILE *out, struct trees *trees, struct stats *st)
>  {
>  	struct vrp	*v;
>  
>  	if (fprintf(out, "ASN,IP Prefix,Max Length,Trust Anchor,Expires\n") < 0)
>  		return -1;
>  
> -	RB_FOREACH(v, vrp_tree, vrps) {
> +	RB_FOREACH(v, vrp_tree, &trees->vrps) {
>  		char buf[64];
>  
>  		ip_addr_print(&v->addr, v->afi, buf, sizeof(buf));
> Index: output-json.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/output-json.c,v
> diff -u -p -r1.53 output-json.c
> --- output-json.c	3 Apr 2025 14:29:44 -0000	1.53
> +++ output-json.c	8 Jul 2025 12:40:46 -0000
> @@ -145,9 +145,7 @@ output_spl(struct vsp_tree *vsps)
>  }
>  
>  int
> -output_json(FILE *out, struct vrp_tree *vrps, struct brk_tree *brks,
> -    struct vap_tree *vaps, struct vsp_tree *vsps, struct nca_tree *ncas,
> -    struct stats *st)
> +output_json(FILE *out, struct trees *trees, struct stats *st)
>  {
>  	char			 buf[64];
>  	struct vrp		*v;
> @@ -158,7 +156,7 @@ output_json(FILE *out, struct vrp_tree *
>  	outputheader_json(st);
>  
>  	json_do_array("roas");
> -	RB_FOREACH(v, vrp_tree, vrps) {
> +	RB_FOREACH(v, vrp_tree, &trees->vrps) {
>  		ip_addr_print(&v->addr, v->afi, buf, sizeof(buf));
>  
>  		json_do_object("roa", 1);
> @@ -172,7 +170,7 @@ output_json(FILE *out, struct vrp_tree *
>  	json_do_end();
>  
>  	json_do_array("bgpsec_keys");
> -	RB_FOREACH(b, brk_tree, brks) {
> +	RB_FOREACH(b, brk_tree, &trees->brks) {
>  		json_do_object("brks", 0);
>  		json_do_int("asn", b->asid);
>  		json_do_string("ski", b->ski);
> @@ -184,7 +182,7 @@ output_json(FILE *out, struct vrp_tree *
>  	json_do_end();
>  
>  	json_do_array("nonfunc_cas");
> -	RB_FOREACH(nca, nca_tree, ncas) {
> +	RB_FOREACH(nca, nca_tree, &trees->ncas) {
>  		json_do_object("nca", 1);
>  		json_do_string("location", nca->location);
>  		json_do_string("ta", taldescs[nca->talid]);
> @@ -196,10 +194,10 @@ output_json(FILE *out, struct vrp_tree *
>  	json_do_end();
>  
>  	if (!excludeaspa)
> -		output_aspa(vaps);
> +		output_aspa(&trees->vaps);
>  
>  	if (experimental)
> -		output_spl(vsps);
> +		output_spl(&trees->vsps);
>  
>  	return json_do_finish();
>  }
> Index: output-ometric.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/output-ometric.c,v
> diff -u -p -r1.14 output-ometric.c
> --- output-ometric.c	3 Apr 2025 14:29:44 -0000	1.14
> +++ output-ometric.c	8 Jul 2025 12:40:46 -0000
> @@ -167,9 +167,7 @@ repo_stats(const struct repo *rp, const 
>  }
>  
>  int
> -output_ometric(FILE *out, struct vrp_tree *vrps, struct brk_tree *brks,
> -    struct vap_tree *vaps, struct vsp_tree *vsps, struct nca_tree *ncas,
> -    struct stats *st)
> +output_ometric(FILE *out, struct trees *trees, struct stats *st)
>  {
>  	struct olabels *ol;
>  	const char *keys[4] = { "nodename", "domainname", "release", NULL };
> Index: output.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/output.c,v
> diff -u -p -r1.40 output.c
> --- output.c	3 Apr 2025 14:29:44 -0000	1.40
> +++ output.c	8 Jul 2025 12:40:46 -0000
> @@ -63,9 +63,7 @@ static char	 output_name[PATH_MAX];
>  static const struct outputs {
>  	int	 format;
>  	char	*name;
> -	int	(*fn)(FILE *, struct vrp_tree *, struct brk_tree *,
> -		    struct vap_tree *, struct vsp_tree *, struct nca_tree *,
> -		    struct stats *);
> +	int	(*fn)(FILE *, struct trees *, struct stats *);
>  } outputs[] = {
>  	{ FORMAT_OPENBGPD, "openbgpd", output_bgpd },
>  	{ FORMAT_BIRD, "bird", output_bird },
> @@ -124,8 +122,7 @@ prune_as0_tals(struct vrp_tree *vrps)
>  }
>  
>  int
> -outputfiles(struct vrp_tree *v, struct brk_tree *b, struct vap_tree *a,
> -    struct vsp_tree *p, struct nca_tree *ncas, struct stats *st)
> +outputfiles(struct trees *trees, struct stats *st)
>  {
>  	int i, rc = 0;
>  
> @@ -133,7 +130,7 @@ outputfiles(struct vrp_tree *v, struct b
>  	set_signal_handler();
>  
>  	if (excludeas0)
> -		prune_as0_tals(v);
> +		prune_as0_tals(&trees->vrps);
>  
>  	for (i = 0; outputs[i].name; i++) {
>  		FILE *fout;
> @@ -147,7 +144,7 @@ outputfiles(struct vrp_tree *v, struct b
>  			rc = 1;
>  			continue;
>  		}
> -		if ((*outputs[i].fn)(fout, v, b, a, p, ncas, st) != 0) {
> +		if ((*outputs[i].fn)(fout, trees, st) != 0) {
>  			warn("output for %s format failed", outputs[i].name);
>  			fclose(fout);
>  			output_cleantmp();
> 

-- 
:wq Claudio