On Wed, Nov 30, 2022 at 11:01:01AM +0100, Theo Buehler wrote:
> On Wed, Nov 30, 2022 at 10:36:08AM +0100, Claudio Jeker wrote:
> > I want to use the bgpctl ometric.c code in rpki-client to implement a
> > metrics output. Currently ometric_output_all() just dumps to stdout but
> > that does not work for rpki-client. Instead pass a FILE pointer to
> > ometric_output_all() and also return -1 if an error occured. With this
> > ometric usage becomes more versatile.
>
> The *printf functions are documented to return a value less than 0 on
> error. Your diff assumes that value is -1 which I believe to be correct
> on OpenBSD. Is it safe to assume that this is the case for -portable?
>
> Should we ensure -1 is returned in the ometric_foo() functions, should
> the ometric_foo() functions be error checked with if (ometric_foo() < 0)
> or should we leave the diff as-is because I'm overthinking this?
>
> Other than that, the diff reads fine to me.
Ah shit, I wanted to use the same idiom that we use in rpki-client but did
not pay attention to the < 0 checks. I will replace all == -1 checks with
< 0. For the ometric_output_* functions I think checking against < 0 is
better since we can directly return fprintf().
> > --
> > :wq Claudio
> >
> > Index: ometric.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpctl/ometric.c,v
> > retrieving revision 1.2
> > diff -u -p -r1.2 ometric.c
> > --- ometric.c 1 Nov 2022 13:35:09 -0000 1.2
> > +++ ometric.c 30 Nov 2022 08:56:26 -0000
> > @@ -246,66 +246,77 @@ ometric_type(enum ometric_type type)
> > }
> > }
> >
> > -static void
> > -ometric_output_labels(const struct olabels *ol)
> > +static int
> > +ometric_output_labels(FILE *out, const struct olabels *ol)
> > {
> > struct olabel *l;
> > const char *comma = "";
> >
> > - if (ol == NULL) {
> > - printf(" ");
> > - return;
> > - }
> > + if (ol == NULL)
> > + return fprintf(out, " ");
> >
> > - printf("{");
> > + if (fprintf(out, "{") == -1)
> > + return -1;
> >
> > while (ol != NULL) {
> > STAILQ_FOREACH(l, &ol->labels, entry) {
> > - printf("%s%s=\"%s\"", comma, l->key, l->value);
> > + if (fprintf(out, "%s%s=\"%s\"", comma, l->key,
> > + l->value) == -1)
> > + return -1;
> > comma = ",";
> > }
> > ol = ol->next;
> > }
> >
> > - printf("} ");
> > + return fprintf(out, "} ");
> > }
> >
> > -static void
> > -ometric_output_value(const struct ovalue *ov)
> > +static int
> > +ometric_output_value(FILE *out, const struct ovalue *ov)
> > {
> > switch (ov->valtype) {
> > case OVT_INTEGER:
> > - printf("%llu", ov->value.i);
> > - return;
> > + return fprintf(out, "%llu", ov->value.i);
> > case OVT_DOUBLE:
> > - printf("%g", ov->value.f);
> > - return;
> > + return fprintf(out, "%g", ov->value.f);
> > }
> > + return -1;
> > }
> >
> > /*
> > * Output all metric values with TYPE and optional HELP strings.
> > */
> > -void
> > -ometric_output_all(void)
> > +int
> > +ometric_output_all(FILE *out)
> > {
> > struct ometric *om;
> > struct ovalue *ov;
> >
> > STAILQ_FOREACH(om, &ometrics, entry) {
> > if (om->help)
> > - printf("# HELP %s %s\n", om->name, om->help);
> > - printf("# TYPE %s %s\n", om->name, ometric_type(om->type));
> > + if (fprintf(out, "# HELP %s %s\n", om->name,
> > + om->help) == -1)
> > + return -1;
> > +
> > + if (fprintf(out, "# TYPE %s %s\n", om->name,
> > + ometric_type(om->type)) == -1)
> > + return -1;
> >
> > STAILQ_FOREACH(ov, &om->vals, entry) {
> > - printf("%s", om->name);
> > - ometric_output_labels(ov->labels);
> > - ometric_output_value(ov);
> > - printf("\n");
> > + if (fprintf(out, "%s", om->name) == -1)
> > + return -1;
> > + if (ometric_output_labels(out, ov->labels) == -1)
> > + return -1;
> > + if (ometric_output_value(out, ov) == -1)
> > + return -1;
> > + if (fprintf(out, "\n") == -1)
> > + return -1;
> > }
> > }
> >
> > - printf("# EOF\n");
> > + if (fprintf(out, "# EOF\n") == -1)
> > + return -1;
> > + return 0;
> > }
> >
> > /*
> > Index: ometric.h
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpctl/ometric.h,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 ometric.h
> > --- ometric.h 17 Oct 2022 12:01:19 -0000 1.1
> > +++ ometric.h 30 Nov 2022 08:49:36 -0000
> > @@ -36,9 +36,8 @@ void ometric_free_all(void);
> > struct olabels *olabels_new(const char * const *, const char **);
> > void olabels_free(struct olabels *);
> >
> > -void ometric_output_all(void);
> > +int ometric_output_all(FILE *);
> >
> > -/* XXX how to pass attributes */
> > /* functions to set gauge and counter metrics */
> > void ometric_set_int(struct ometric *, uint64_t, struct olabels *);
> > void ometric_set_float(struct ometric *, double, struct olabels *);
> > Index: output_ometric.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpctl/output_ometric.c,v
> > retrieving revision 1.3
> > diff -u -p -r1.3 output_ometric.c
> > --- output_ometric.c 7 Nov 2022 11:33:24 -0000 1.3
> > +++ output_ometric.c 30 Nov 2022 08:48:32 -0000
> > @@ -75,9 +75,7 @@ ometric_head(struct parse_result *arg)
> > values[3] = NULL;
> >
> > ol = olabels_new(keys, values);
> > -
> > ometric_set_info(bgpd_info, NULL, NULL, ol);
> > -
> > olabels_free(ol);
> >
> > /*
> > @@ -324,7 +322,7 @@ ometric_tail(void)
> > (double)elapsed_time.tv_usec / 1000000;
> >
> > ometric_set_float(bgpd_scrape_time, scrape, NULL);
> > - ometric_output_all();
> > + ometric_output_all(stdout);
> >
> > ometric_free_all();
> > }
> >
>
--
:wq Claudio
