Re: [PATCH v8 10/10] devtools: check event device doc tables

[Thomas Monjalon <thomas@monjalon.net>]

23/11/2021 12:07, skori@marvell.com:
> --- a/devtools/check-doc-vs-code.sh
> +++ b/devtools/check-doc-vs-code.sh
> +all_event_drivers()
> +{
> +	find $rootdir/drivers/event -mindepth 1 -maxdepth 1 -type d |
> +	sed 's,.*/,,' |
> +	sort
> +}
> +
> +check_event_dev() # <driver>
> +{
> +	code=$rootdir/drivers/event/$1
> +	doc=$rootdir/doc/guides/eventdevs/features/$1.ini
> +	[ -d $code ] || return 0
> +	[ -f $doc ] || return 0
> +	report=$($selfdir/parse-event-support.sh $code $doc)
> +	if [ -n "$report" ]; then
> +		error "doc out of sync for $1"
> +		echo "$report" | sed 's,^,\t,'
> +	fi
> +}

These 2 functions are mostly copy/paste of rte_flow functions.
Given there will be more in future, I would prefer code being factorized.

>  if [ -z "$trusted_commit" ]; then
>  	# check all
>  	for driver in $(all_net_drivers); do
>  		check_rte_flow $driver
>  	done
> +

I would remove this blank line.

> +	for driver in $(all_event_drivers); do
> +		check_event_dev $driver
> +	done
>  	exit $result
>  fi
[...]
> +if has_code_change 'RTE_EVENT_DEV_CAP_*' ||
> +		has_code_change 'RTE_EVENT_ETH_RX_ADAPTER_CAP_*' ||
> +		has_code_change 'RTE_EVENT_ETH_TX_ADAPTER_CAP_*' ||
> +		has_code_change 'RTE_EVENT_CRYPTO_ADAPTER_CAP_*' ||
> +		has_code_change 'RTE_EVENT_TIMER_ADAPTER_CAP_*' ||

Can it be a single query?

> +		has_file_change 'doc/guides/eventdevs/features'; then
> +	for driver in $(all_event_drivers); do

No need to check all drivers.
For rte_flow, only changed drivers are checked.

> +		check_event_dev $driver
> +	done
> +fi
[...]
> +# generate INI section
> +list() # <title> <pattern> <extra_patterns>
> +{
> +	echo "[$1]"
> +	word0=$(git grep -who "$2[[:alnum:]_]*" $dir)
> +	word1=$(echo "$3")

Why echo?

> +	words="$word0""$word1"

Why so many quotes?

> +	echo "$words" | sort -u |
> +		awk 'sub(/'$2'/, "") {printf "%-20s = Y\n", tolower($0)}'
> +}
[...]
> +event_dev_rx_adptr_support()
> +{
> +	title="Eth Rx adapter Features"
> +	pattern=$(echo "RTE_EVENT_ETH_RX_ADAPTER_CAP_" |
> +		awk '{print toupper($0)}')
> +	check_rx_adptr_sw_capa || extra='RTE_EVENT_ETH_RX_ADAPTER_CAP_OVERRIDE_FLOW_ID
> +				RTE_EVENT_ETH_RX_ADAPTER_CAP_MULTI_EVENTQ
> +				RTE_EVENT_ETH_RX_ADAPTER_CAP_EVENT_VECTOR'

Why having extra parameter, instead of updating the pattern?
By the way, the pattern RTE_EVENT_ETH_RX_ADAPTER_CAP_ already include all of this.

> +	list "$title" "$pattern" "$extra"
> +}
[...]
> +# compare with reference input
> +event_dev_sched_compare()
> +{
> +	section="Scheduling Features]"
> +	{
> +		event_dev_sched_support
> +		sed -n "/$section/,/]/p" "$ref" | sed '/^$/d'
> +	} |
> +	sed '/]/d' | # ignore section title
> +	sed 's, *=.*,,' | # ignore value (better in doc than generated one)
> +	sort | uniq -u | # show differences
> +	sed "s,^,Scheduling Features ," # prefix with category name
> +}
> +
> +event_dev_rx_adptr_compare()
> +{
> +	section="Eth Rx adapter Features]"
> +	{
> +		event_dev_rx_adptr_support
> +		sed -n "/$section/,/]/p" "$ref" | sed '/^$/d'
> +	} |
> +	sed '/]/d' | # ignore section title
> +	sed 's, *=.*,,' | # ignore value (better in doc than generated one)
> +	sort | uniq -u | # show differences
> +	sed "s,^,Eth Rx adapter Features ," # prefix with category name
> +}
> +
> +event_dev_tx_adptr_compare()
> +{
> +	section="Eth Tx adapter Features]"
> +	{
> +		event_dev_tx_adptr_support
> +		sed -n "/$section/,/]/p" "$ref" | sed '/^$/d'
> +	} |
> +	sed '/]/d' | # ignore section title
> +	sed 's, *=.*,,' | # ignore value (better in doc than generated one)
> +	sort | uniq -u | # show differences
> +	sed "s,^,Eth Tx adapter Features ," # prefix with category name
> +}
> +
> +event_dev_crypto_adptr_compare()
> +{
> +	section="Crypto adapter Features]"
> +	{
> +		event_dev_crypto_adptr_support
> +		sed -n "/$section/,/]/p" "$ref" | sed '/^$/d'
> +	} |
> +	sed '/]/d' | # ignore section title
> +	sed 's, *=.*,,' | # ignore value (better in doc than generated one)
> +	sort | uniq -u | # show differences
> +	sed "s,^,Crypto adapter Features ," # prefix with category name
> +}
> +
> +event_dev_timer_adptr_compare()
> +{
> +	section="Timer adapter Features]"
> +	{
> +		event_dev_timer_adptr_support
> +		sed -n "/$section/,/]/p" "$ref" | sed '/^$/d'
> +	} |
> +	sed '/]/d' | # ignore section title
> +	sed 's, *=.*,,' | # ignore value (better in doc than generated one)
> +	sort | uniq -u | # show differences
> +	sed "s,^,Timer adapter Features ," # prefix with category name
> +}

I think these functions can be factorized.