On Fri, Jul 14, 2023 at 11:43:38AM +0200, Claudio Jeker wrote:
> On Thu, Jul 13, 2023 at 11:36:22AM +0200, Theo Buehler wrote:
> > On Thu, Jul 13, 2023 at 10:04:33AM +0200, Claudio Jeker wrote:
> > > This is a follow-up to use more of the new ibuf API to write the mrt
> > > message.
> > >
> > > This removes all of the DUMP_XYZ macros and replaces them with
> > > ibuf_add_nX() calls. Also unify the error handling by using
> > > goto fail; in all cases and use a more generic log_warn() there once.
> >
> > The conversions all look correct, so that's ok.
> >
> > There are a few silent failures and a few double logs. The former should
> > be avoided and I think this should be done in this diff. I'm not sure
> > how much effort should be invested in fully avoiding the latter. It's a
> > bit messy:
> >
> > The only caller mrt_dump_entry_v2_rib() already logs an ibuf failure on
> > error, so there's no need to add a log to mrt_dump_entry_v2_rib()'s fail
> > label.
> >
> > In mrt_dump_hdr_se() there is no log after the fail: label. I think it
> > needs one, otherwise mrt_dump_{bgp_msg,state}() could fail silently.
> >
> > Most callers of mrt_dump_hdr_rde() will log an ibuf error on failure, so
> > you probably want to remove the log in the early return after ibuf_dynamic.
> >
> > Also, add a log (or goto fail) on mrt_dump_hdr_rde() failure in
> > mrt_dump_entry(). In particular, there is often a double log for the
> > 'unsupported type' case in mrt_dump_hdr_rde()...
>
> This is a cleaned up version following what you mentioned. I marked the
> internal functions with static and removed most logging from them and
> instead log only on the primary functions.
Yes, that's much cleaner now.
> I did not do this for those default cases in various switches. These
> errors are more like asserts and IIRC once were fatalx() calls but they
> got demoted to warnings to reduce the number of fatal errors in bgpd.
> Since these code paths are in most cases unreachable (unless a bug is
> introduced) I'm fine with the double logging.
Agreed. Totally makes sense.
ok tb
