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

[Sunil Kumar Kori]

Hi Thomas,

It will take some time to handle all the comments for this patch. So I would request you to defer this patch for next release and take other patches in series. 
Also I will send one patch to add feature matrix for dlb2 platform and fix minor review comments given by You. 

Regards
Sunil Kumar Kori

>-----Original Message-----
>From: Thomas Monjalon <thomas at monjalon.net>
>Sent: Wednesday, November 24, 2021 4:23 PM
>To: Sunil Kumar Kori <skori at marvell.com>
>Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; nikhil.rao at intel.com;
>Pavan Nikhilesh Bhagavatula <pbhagavatula at marvell.com>;
>hemant.agrawal at nxp.com; nipun.gupta at nxp.com;
>harry.van.haaren at intel.com; mattias.ronnblom at ericsson.com;
>liang.j.ma at intel.com; dev at dpdk.org
>Subject: [EXT] Re: [PATCH v8 10/10] devtools: check event device doc tables
>
>External Email
>
>----------------------------------------------------------------------
>23/11/2021 12:07, skori at 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.
>