RFC: ospf6d: add conditional debug log macro

[ghost]

Introduces a new logging macro: zlog_debug_cond(condition, message) and uses it to replace a repeating pattern:

if (condition)
	zlog_debug(...)

becomes:

zlog_debug_cond(condition, ...)

Code readability is improved when debug logging reads like statements. It is meant to be used without side effects in the format string evaluation.

[ghost]

I've marked this PR as a draft, because I expect the change may be controversial.

I opted not to try a blanket replacement across the codebase, because I've only just started working on FRR (specifically only in the ospf6d area) and will be learning the maintainers' expectations and tolerance for code churn.

Nevertheless, I'm developing a sense that the ospf6d code base is not receiving a great deal of development focus at the moment, and there are a number of general maintenance issues I need to work through towards my goal of adding support for RFC8632 (OSPFv3 Link State Advertisement (LSA) Extensibility).

Please view this PR as an introduction of sorts and an opportunity to tell me about the kind of change that is or isn't likely to be acceptable 'upstream'.

Thanks!

[ton31337]

Before moving on, can zlog.h be split into a separate commit?

[ton31337]

We have similar (not for logging) COND_FLAG() macro, thinking loudly just if we could you the same naming instead? zlog_debug_cond(). Another example bgp_cond_adv_debug() (same cond naming)

[ghost]

I've changed zlog_debug_if() to zlog_debug_cond() as suggested.

I think a shorter macro name would be better, given how often it's used and how big an impact it has on line wrapping, but consistency with established convention probably takes precedence.

[eqvinox]

I think a shorter macro name would be better,

Indeed this is repeating the development of 12272. It started at zlog_debugif(), went to zlog_dbgif() and finally arrived at dbg()

[ghost]

@donaldsharp Apologies, I accidentally closed PR #15721 (trying to avoid embarrassment) and couldn't reopen it and unfortunately that caused some confusion and resulted in your comment almost getting missed.

The comment was:
"There are already several people looking at reworking debugs, before such a change is accepted I would prefer that this work of changing debugs in ospfv3 be in line with the other work. See #15646 as well as #12272 and #15522"

Thanks for pointing out the other work - I was unaware.

After looking at these PRs, my understanding of their primary intents are as follows:
#12272 - adds fine-grained dynamic tracing at run-time. It mentions the if (condition) zlog_debug("msg"); pattern but doesn't eliminate it. Instead, the continued need for the pattern is an obstacle for the PR. The intent of this PR looks to be in contradiction with the intent of #12272.

#15522 - adds additional uses of the if (condition) zlog_debug("msg"); pattern and doesn't aim to change or remove it.

#15646 - modifies the debug library, but it's not apparent to me what the calling API and usage patterns are. It isn't currently used by ospf6d and doesn't appear to offer a replacement for the if (condition) zlog_debug("msg"); pattern.

So merging any of those PRs would not address the kind of issue that this PR is trying to address, namely that there is a commonly used pattern for debug logging, which (imho as a new reader) could be improved to aid readability.

[eqvinox]

NAK, this is already being worked on in both #15646 and #12272 - these two already duplicated work and need to be reconciled. This is just doing it a third way yet again. Sorry about the slow progress on the other 2 PRs & the redundant work on your end.

[eqvinox]

NAK as commented

[eqvinox]

#15646 - modifies the debug library, but it's not apparent to me what the calling API and usage patterns are. It isn't currently used by ospf6d and doesn't appear to offer a replacement for the if (condition) zlog_debug("msg"); pattern.

btw, the target pattern with #12272 is

  dbg(FLAG_NAME, "message %s", arg);

It was previously discussed that this changeover will happen separately and 12272 is only going to add the new mechanism. ⇒ changeover can be done in smaller blocks

[ghost]

#15646 - modifies the debug library, but it's not apparent to me what the calling API and usage patterns are. It isn't currently used by ospf6d and doesn't appear to offer a replacement for the if (condition) zlog_debug("msg"); pattern.

btw, the target pattern with #12272 is

  dbg(FLAG_NAME, "message %s", arg);

It was previously discussed that this changeover will happen separately and 12272 is only going to add the new mechanism. ⇒ changeover can be done in smaller blocks

The approach of only allowing defined flag names is the major point of difference. It will be a much bigger change around the existing debug log call sites where conditions are used. It would be nice to see some WIP for how those conversions can be done.

However, I understand that there is a clear vision for the end state for the debug logging, and don't wish to distract from that, therefore this PR can probably close.

Thanks for the review comments!