[v3,00/16] stop using variadic argument pack extension

  RTE_LOG_LINE cannot be augmented with a prefix format and arguments
without the user of RTE_LOG_LINE using the args... and ## args compiler
extension to conditionally remove trailing comma when the macro receives
only a single argument.

Provide a new/similar macro RTE_LOG_LINE_PREFIX that accepts the prefix
format and arguments as separate parameters allowing them to be expanded
at the correct locations inside of RTE_FMT() allowing the rest of the
non-prefix format string and arguments to be collapsed to the argument
pack which can be directly forwarded with __VA_ARGS__ avoiding the need
for conditional comma removal.

I've done my best to manually check expansions (preprocessed) and compiled
printf of the logs to validate correct output.

note: due to drastic change in series i have not carried any series acks
      forward.

v3:
  * remove leading _ from RTE_LOG_COMMA the macro is not internal
  * add doxygen comment for existing RTE_LOG{,DP}_LINE function-like
    macros, based on RTE_LOG{,DP} comments.
  * add doxygen comment for new RTE_LOG{,DP}_LINE_PREFIX function-like
    macros, based on RTE_LOG{,DP} comments.
  * merge 2 vhost patches into a single patch (mistake in previous
    submission)

v2:
  * revamp entire series to be ISO C99 compliant, stop using variadic
    argument pack extension.

Tyler Retzlaff (16):
  log: add a per line log helper with parameterized prefix
  bpf: stop using variadic argument pack extension
  cfgfile: stop using variadic argument pack extension
  cmdline: stop using variadic argument pack extension
  compressdev: stop using variadic argument pack extension
  metrics: stop using variadic argument pack extension
  mldev: stop using variadic argument pack extension
  net: stop using variadic argument pack extension
  pdump: stop using variadic argument pack extension
  power: stop using variadic argument pack extension
  rawdev: stop using variadic argument pack extension
  rcu: stop using variadic argument pack extension
  stack: stop using variadic argument pack extension
  eal: stop using variadic argument pack extension
  vhost: stop using variadic argument pack extension
  ip_frag: stop using variadic argument pack extension

 lib/bpf/bpf_impl.h                         |  4 +-
 lib/cfgfile/rte_cfgfile.c                  |  5 +-
 lib/cmdline/cmdline_parse.c                |  2 +-
 lib/cmdline/cmdline_parse_num.c            |  4 +-
 lib/compressdev/rte_compressdev_internal.h |  4 +-
 lib/eal/common/eal_trace.h                 |  8 +--
 lib/ip_frag/ip_frag_common.h               |  4 +-
 lib/log/rte_log.h                          | 97 ++++++++++++++++++++++++++++++
 lib/metrics/rte_metrics_telemetry.c        | 12 ++--
 lib/mldev/rte_mldev.h                      |  4 +-
 lib/net/rte_net_crc.c                      |  4 +-
 lib/pdump/rte_pdump.c                      |  4 +-
 lib/power/power_common.h                   |  6 +-
 lib/rawdev/rte_rawdev_pmd.h                | 17 +++---
 lib/rcu/rte_rcu_qsbr.c                     |  4 +-
 lib/rcu/rte_rcu_qsbr.h                     | 12 ++--
 lib/stack/stack_pvt.h                      | 16 ++---
 lib/vhost/vhost.h                          |  8 +--
 lib/vhost/vhost_crypto.c                   | 21 ++++---
 19 files changed, 168 insertions(+), 68 deletions(-)
  

On Mon, 26 Feb 2024 12:19:30 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> RTE_LOG_LINE cannot be augmented with a prefix format and arguments
> without the user of RTE_LOG_LINE using the args... and ## args compiler
> extension to conditionally remove trailing comma when the macro receives
> only a single argument.
> 
> Provide a new/similar macro RTE_LOG_LINE_PREFIX that accepts the prefix
> format and arguments as separate parameters allowing them to be expanded
> at the correct locations inside of RTE_FMT() allowing the rest of the
> non-prefix format string and arguments to be collapsed to the argument
> pack which can be directly forwarded with __VA_ARGS__ avoiding the need
> for conditional comma removal.
> 
> I've done my best to manually check expansions (preprocessed) and compiled
> printf of the logs to validate correct output.
> 
> note: due to drastic change in series i have not carried any series acks
>       forward.

The changes look good, you might want to add release note, update coding
style doc, and/or update checkpatch to discourage re-introduction.

Series-acked-by: Stephen Hemminger <stephen@networkplumber.org>
  

On Mon, Feb 26, 2024 at 12:54:36PM -0800, Stephen Hemminger wrote:
> On Mon, 26 Feb 2024 12:19:30 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > RTE_LOG_LINE cannot be augmented with a prefix format and arguments
> > without the user of RTE_LOG_LINE using the args... and ## args compiler
> > extension to conditionally remove trailing comma when the macro receives
> > only a single argument.
> > 
> > Provide a new/similar macro RTE_LOG_LINE_PREFIX that accepts the prefix
> > format and arguments as separate parameters allowing them to be expanded
> > at the correct locations inside of RTE_FMT() allowing the rest of the
> > non-prefix format string and arguments to be collapsed to the argument
> > pack which can be directly forwarded with __VA_ARGS__ avoiding the need
> > for conditional comma removal.
> > 
> > I've done my best to manually check expansions (preprocessed) and compiled
> > printf of the logs to validate correct output.
> > 
> > note: due to drastic change in series i have not carried any series acks
> >       forward.
> 
> The changes look good, you might want to add release note, update coding
> style doc, and/or update checkpatch to discourage re-introduction.

re-introduction should be protected by the CI. i know checkpatch would
be better but i couldn't think of a good way to match an arbitrarily
named pack ... reliably where it could potentially cause noise.

i'll look into style doc in a separate series since i think there are a
couple of things i'd like to propose there now that we are using C11.

thanks for the review!

> 
> Series-acked-by: Stephen Hemminger <stephen@networkplumber.org>
  

On Tue, Feb 27, 2024 at 7:15 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Mon, Feb 26, 2024 at 12:54:36PM -0800, Stephen Hemminger wrote:
> > On Mon, 26 Feb 2024 12:19:30 -0800
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >
> > > RTE_LOG_LINE cannot be augmented with a prefix format and arguments
> > > without the user of RTE_LOG_LINE using the args... and ## args compiler
> > > extension to conditionally remove trailing comma when the macro receives
> > > only a single argument.
> > >
> > > Provide a new/similar macro RTE_LOG_LINE_PREFIX that accepts the prefix
> > > format and arguments as separate parameters allowing them to be expanded
> > > at the correct locations inside of RTE_FMT() allowing the rest of the
> > > non-prefix format string and arguments to be collapsed to the argument
> > > pack which can be directly forwarded with __VA_ARGS__ avoiding the need
> > > for conditional comma removal.
> > >
> > > I've done my best to manually check expansions (preprocessed) and compiled
> > > printf of the logs to validate correct output.
> > >
> > > note: due to drastic change in series i have not carried any series acks
> > >       forward.
> >
> > The changes look good, you might want to add release note, update coding
> > style doc, and/or update checkpatch to discourage re-introduction.
>
> re-introduction should be protected by the CI. i know checkpatch would
> be better but i couldn't think of a good way to match an arbitrarily
> named pack ... reliably where it could potentially cause noise.

What about a simple:

+       # forbid use of variadic argument pack extension in macros
+       awk -v FOLDERS="lib drivers app examples" \
+               -v
EXPRESSIONS='#[[:space:]]*define.*[^(,[:space:]]\\.\\.\\.[[:space:]]*)'
\
+               -v RET_ON_FAIL=1 \
+               -v MESSAGE='Do not use variadic argument pack in macros' \
+               -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
+               "$1" || res=1
+
  

On Wed, Feb 28, 2024 at 12:45:04PM +0100, David Marchand wrote:
> On Tue, Feb 27, 2024 at 7:15 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > On Mon, Feb 26, 2024 at 12:54:36PM -0800, Stephen Hemminger wrote:
> > > On Mon, 26 Feb 2024 12:19:30 -0800
> > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > >
> > > > RTE_LOG_LINE cannot be augmented with a prefix format and arguments
> > > > without the user of RTE_LOG_LINE using the args... and ## args compiler
> > > > extension to conditionally remove trailing comma when the macro receives
> > > > only a single argument.
> > > >
> > > > Provide a new/similar macro RTE_LOG_LINE_PREFIX that accepts the prefix
> > > > format and arguments as separate parameters allowing them to be expanded
> > > > at the correct locations inside of RTE_FMT() allowing the rest of the
> > > > non-prefix format string and arguments to be collapsed to the argument
> > > > pack which can be directly forwarded with __VA_ARGS__ avoiding the need
> > > > for conditional comma removal.
> > > >
> > > > I've done my best to manually check expansions (preprocessed) and compiled
> > > > printf of the logs to validate correct output.
> > > >
> > > > note: due to drastic change in series i have not carried any series acks
> > > >       forward.
> > >
> > > The changes look good, you might want to add release note, update coding
> > > style doc, and/or update checkpatch to discourage re-introduction.
> >
> > re-introduction should be protected by the CI. i know checkpatch would
> > be better but i couldn't think of a good way to match an arbitrarily
> > named pack ... reliably where it could potentially cause noise.
> 
> What about a simple:
> 
> +       # forbid use of variadic argument pack extension in macros
> +       awk -v FOLDERS="lib drivers app examples" \
> +               -v
> EXPRESSIONS='#[[:space:]]*define.*[^(,[:space:]]\\.\\.\\.[[:space:]]*)'
> \
> +               -v RET_ON_FAIL=1 \
> +               -v MESSAGE='Do not use variadic argument pack in macros' \
> +               -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> +               "$1" || res=1
> +

I have no objection, I suppose worst case scenario if it turns out to
catch things we don't want it to catch we can always ignore or remove
it.

I will add the check and submit a new rev.

> 
> 
> -- 
> David Marchand
  

On Mon, Feb 26, 2024 at 9:20 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> RTE_LOG_LINE cannot be augmented with a prefix format and arguments
> without the user of RTE_LOG_LINE using the args... and ## args compiler
> extension to conditionally remove trailing comma when the macro receives
> only a single argument.
>
> Provide a new/similar macro RTE_LOG_LINE_PREFIX that accepts the prefix
> format and arguments as separate parameters allowing them to be expanded
> at the correct locations inside of RTE_FMT() allowing the rest of the
> non-prefix format string and arguments to be collapsed to the argument
> pack which can be directly forwarded with __VA_ARGS__ avoiding the need
> for conditional comma removal.
>
> I've done my best to manually check expansions (preprocessed) and compiled
> printf of the logs to validate correct output.
>
> note: due to drastic change in series i have not carried any series acks
>       forward.
>
> v3:
>   * remove leading _ from RTE_LOG_COMMA the macro is not internal
>   * add doxygen comment for existing RTE_LOG{,DP}_LINE function-like
>     macros, based on RTE_LOG{,DP} comments.
>   * add doxygen comment for new RTE_LOG{,DP}_LINE_PREFIX function-like
>     macros, based on RTE_LOG{,DP} comments.
>   * merge 2 vhost patches into a single patch (mistake in previous
>     submission)

I find this new helper less tricky to use and easier to read than the
RTE_FMT_* stuff that gets copy/pasted everywhere.
The changes are quite mechanical, so even though we are past -rc1, +1
for me on the series.

Can we finish the job and convert remaining macros that prefix messages in lib/?
  

On Wed, Feb 28, 2024 at 06:29:14PM +0100, David Marchand wrote:
> On Mon, Feb 26, 2024 at 9:20 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > RTE_LOG_LINE cannot be augmented with a prefix format and arguments
> > without the user of RTE_LOG_LINE using the args... and ## args compiler
> > extension to conditionally remove trailing comma when the macro receives
> > only a single argument.
> >
> > Provide a new/similar macro RTE_LOG_LINE_PREFIX that accepts the prefix
> > format and arguments as separate parameters allowing them to be expanded
> > at the correct locations inside of RTE_FMT() allowing the rest of the
> > non-prefix format string and arguments to be collapsed to the argument
> > pack which can be directly forwarded with __VA_ARGS__ avoiding the need
> > for conditional comma removal.
> >
> > I've done my best to manually check expansions (preprocessed) and compiled
> > printf of the logs to validate correct output.
> >
> > note: due to drastic change in series i have not carried any series acks
> >       forward.
> >
> > v3:
> >   * remove leading _ from RTE_LOG_COMMA the macro is not internal
> >   * add doxygen comment for existing RTE_LOG{,DP}_LINE function-like
> >     macros, based on RTE_LOG{,DP} comments.
> >   * add doxygen comment for new RTE_LOG{,DP}_LINE_PREFIX function-like
> >     macros, based on RTE_LOG{,DP} comments.
> >   * merge 2 vhost patches into a single patch (mistake in previous
> >     submission)
> 
> I find this new helper less tricky to use and easier to read than the
> RTE_FMT_* stuff that gets copy/pasted everywhere.
> The changes are quite mechanical, so even though we are past -rc1, +1
> for me on the series.
> 
> Can we finish the job and convert remaining macros that prefix messages in lib/?

I didn't realize I missed any. do you have a list or a regex that points
me at them.  I was just searching for use of args... 

Happy to make the conversion of the others in the next rev.

ty

> 
> 
> -- 
> David Marchand
  

On Wed, Feb 28, 2024 at 6:59 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
> > I find this new helper less tricky to use and easier to read than the
> > RTE_FMT_* stuff that gets copy/pasted everywhere.
> > The changes are quite mechanical, so even though we are past -rc1, +1
> > for me on the series.
> >
> > Can we finish the job and convert remaining macros that prefix messages in lib/?
>
> I didn't realize I missed any. do you have a list or a regex that points
> me at them.  I was just searching for use of args...
>
> Happy to make the conversion of the others in the next rev.

Basically, this new macro/approach makes direct use of RTE_FMT_HEAD unneeded.

So I grepped like this:
$ git grep RTE_FMT_HEAD -- lib/ :^lib/log/ :^lib/eal/include/rte_common.h
b55361f252:lib/cryptodev/rte_cryptodev.h:               RTE_FMT("%s()
line %u: " RTE_FMT_HEAD(__VA_ARGS__ ,), \
b55361f252:lib/cryptodev/rte_cryptodev.h:               RTE_FMT("%s()
line %u: " RTE_FMT_HEAD(__VA_ARGS__ ,), \
b55361f252:lib/cryptodev/rte_cryptodev.h:               RTE_FMT("[%s]
%s: " RTE_FMT_HEAD(__VA_ARGS__ ,), \
b55361f252:lib/eal/windows/include/rte_windows.h:
RTE_FMT_HEAD(__VA_ARGS__ ,), GetLastError(), \
b55361f252:lib/eventdev/eventdev_pmd.h:         RTE_FMT("%s() line %u:
" RTE_FMT_HEAD(__VA_ARGS__ ,), \
b55361f252:lib/eventdev/eventdev_pmd.h:         RTE_FMT("%s() line %u:
" RTE_FMT_HEAD(__VA_ARGS__ ,), \
b55361f252:lib/eventdev/rte_event_timer_adapter.c:
RTE_FMT("EVTIMER: %s() line %u: " RTE_FMT_HEAD(__VA_ARGS__ ,), \
b55361f252:lib/graph/graph_private.h:           RTE_FMT("%s():%u "
RTE_FMT_HEAD(__VA_ARGS__ ,),                \
b55361f252:lib/member/member.h:         RTE_FMT("%s(): "
RTE_FMT_HEAD(__VA_ARGS__ ,), \
b55361f252:lib/node/node_private.h:             RTE_FMT("%s: %s():%u "
RTE_FMT_HEAD(__VA_ARGS__ ,),            \