common: call stack analysis for DAOS #6157

grom72 · 2025-03-07T14:59:33Z

Provide call stack analysis for latest state of DAOS source code.

This change is

janekmi

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @grom72)

a discussion (no related file):
I have a problem with the name do_call_stack_analysis_for_daos.sh. Because it is DAOS-oriented but at the same time without -u it would use the provided example API filter which you would like to have the references to DAOS removed. So, following your logic it won't do a DAOS-oriented analysis unless -u is used, hence the name is not appropriate.

Please make it straight.

utils/call_stacks_analysis/make_stack_usage.sh line 15 at r1 (raw file):

SRC=src/$BUILD

pushd $TOP

I see no difference. Why push/popd is better compared to cd/cd -?

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 6 at r1 (raw file):

#
#
# Execute complete call stack analysis filtrered for DAOS only funcion calls

Suggestion:

Execute complete call stack analysis filtered for the DAOS function calls.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 8 at r1 (raw file):

# Execute complete call stack analysis filtrered for DAOS only funcion calls
#
# update examples/api_filter.txt file based on actual DAOS source code using:

Suggestion:

Update the examples/api_filter.txt file based on the actual DAOS source code. (Optional)

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 10 at r1 (raw file):

# update examples/api_filter.txt file based on actual DAOS source code using:
# grep -r -E 'pmemobj_[^(]*\(' $daos_src_folder | grep -v vos_pmemobj | \
#      sed 's/.*\(pmemobj_[^)]*(\).*/\1/p' | sed 's/(.*//' |  sort | uniq

Redundancy.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 13 at r1 (raw file):

#

function help() {

There is no -h/--help mentioned in the usage.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 17 at r1 (raw file):

        echo "       $0 [-u|--update]"
        echo
        echo "       -u/--update       use actual pmemobj api call from DAOS source code repo"

Suggestion:

use the actual pmemobj API calls from the DAOS source code

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 22 at r1 (raw file):

function main() {
        local update=${1:-0}

The provided argument is always set. No need to have a default.

Suggestion:

local update=$1

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 27 at r1 (raw file):

        if [ $update == 1 ]; then
                wget -q https://github.com/daos-stack/daos/archive/refs/heads/master.zip -O daos.zip
                unzip -q -u daos.zip; rm daos.zip

I am not sure why these two are squeezed in a single line.

Suggestion:

unzip -q -u daos.zip
rm daos.zip

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 27 at r1 (raw file):

        if [ $update == 1 ]; then
                wget -q https://github.com/daos-stack/daos/archive/refs/heads/master.zip -O daos.zip
                unzip -q -u daos.zip; rm daos.zip

I am not sure why -u. It looks like you clean up afterwards, don't you?

-u  update files, create if necessary

Suggestion:

unzip -q daos.zip

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 28 at r1 (raw file):

                wget -q https://github.com/daos-stack/daos/archive/refs/heads/master.zip -O daos.zip
                unzip -q -u daos.zip; rm daos.zip
                grep -r -E 'pmemobj_[^(]*\(' "daos-master/src" | grep -v vos_pmemobj | \

It seems you use double quotes vs single quotes pretty willy-nilly. I have no idea why in a single line you have both of them.

Suggestion:

'pmemobj_[^(]*\(' 'daos-master/src'

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 28 at r1 (raw file):

                wget -q https://github.com/daos-stack/daos/archive/refs/heads/master.zip -O daos.zip
                unzip -q -u daos.zip; rm daos.zip
                grep -r -E 'pmemobj_[^(]*\(' "daos-master/src" | grep -v vos_pmemobj | \

I think this procedure's robustness would benefit if we skip printing file names. TBH vos_pmemobj sounds like a file name already. In which case you would exclude all API calls from this file which would be bad.

       -h, --no-filename
              Suppress the prefixing of file names on output.  This is
              the default when there is only one file (or only standard
              input) to search.

Suggestion:

grep -r -E -h 'pmemobj_[^(]*\(' "daos-master/src" | grep -v vos_pmemobj | \

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 29 at r1 (raw file):

                unzip -q -u daos.zip; rm daos.zip
                grep -r -E 'pmemobj_[^(]*\(' "daos-master/src" | grep -v vos_pmemobj | \
                        sed 's/.*\(pmemobj_[^)]*(\).*/\1/p' | sed 's/(.*//' | \

You do not need this bit at all if you use grep(1) to its full potential. Namely:

grep -r -E -h -o 'pmemobj_[^( ]*\('

No file names. (-h)
Print only the matching bits.

-o, --only-matching
Print only the matched (non-empty) parts of a matching line, with each such part on a separate output line.

Use sed(1) just to remove the last character from all the lines.

Suggestion:

sed 's/.$//'

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 38 at r1 (raw file):

        ./make_extra.py && \
        ./make_cflow.sh && \
        ./make_call_stacks.py --filter-api-file $filter_name --filter-lower-limit 11568

It is the third copy of this value. What can we do about this?

Code quote:

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 39 at r1 (raw file):

        ./make_cflow.sh && \
        ./make_call_stacks.py --filter-api-file $filter_name --filter-lower-limit 11568
        --dump-all-stacks

?

Suggestion:

        ./make_call_stacks.py --filter-api-file $filter_name --filter-lower-limit 11568 \
        --dump-all-stacks

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 39 at r1 (raw file):

        ./make_cflow.sh && \
        ./make_call_stacks.py --filter-api-file $filter_name --filter-lower-limit 11568
        --dump-all-stacks

Indentation.

Suggestion:

        ./make_stack_usage.sh && \
                ./make_api.sh && \
                ./make_extra.py && \
                ./make_cflow.sh && \
                ./make_call_stacks.py --filter-api-file $filter_name --filter-lower-limit 11568 \
                        --dump-all-stacks

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 53 at r1 (raw file):

fi

main $update

The newline is missing.

utils/call_stacks_analysis/examples/api_filter.txt line 1 at r1 (raw file):

# api_filter file containing libpmemobj API calls used in DAOS

This comment still looks relevant.
I think it might be important to add also a comment as follows:

# This file was generated by tool X. Please do not update manually.

utils/call_stacks_analysis/make_cflow.sh line 18 at r1 (raw file):

	echo "cflow not found in PATH, please check if it is available."
	echo
	echo "You can download the lates source code of cflow from:" \

Suggestion:

latest

utils/call_stacks_analysis/make_cflow.sh line 23 at r1 (raw file):

	echo "'tar -xvzf cflow-latest.tar.gz && pushd cflow-1.7 &&" \
	     "./configure && make && sudo make install && popd'"
	exit 1

Can we have instead a script you can reference to? Keeping command lines in comments is not a very effective way of maintaining your code.

Code quote:

	echo "You can download the lates source code of cflow from:" \
	     "https://ftp.gnu.org/gnu/cflow/cflow-latest.tar.gz"
	echo "Then build and install it using the following commands:"
	echo "'tar -xvzf cflow-latest.tar.gz && pushd cflow-1.7 &&" \
	     "./configure && make && sudo make install && popd'"
	exit 1

grom72

Reviewable status: 1 of 8 files reviewed, 20 unresolved discussions (waiting on @janekmi)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

I have a problem with the name do_call_stack_analysis_for_daos.sh. Because it is DAOS-oriented but at the same time without -u it would use the provided example API filter which you would like to have the references to DAOS removed. So, following your logic it won't do a DAOS-oriented analysis unless -u is used, hence the name is not appropriate.

Please make it straight.

Done.

utils/call_stacks_analysis/make_cflow.sh line 23 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Can we have instead a script you can reference to? Keeping command lines in comments is not a very effective way of maintaining your code.

Done.

utils/call_stacks_analysis/make_stack_usage.sh line 15 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I see no difference. Why push/popd is better compared to cd/cd -?

Because push/popd is design for such situation
cd - is about switching to the previous one

utils/call_stacks_analysis/examples/api_filter.txt line 1 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This comment still looks relevant.

I think it might be important to add also a comment as follows:
# This file was generated by tool X. Please do not update manually.

Hmmm

the problem is that we can not easily check if new functions has been added. If we do not have header simple git diff says that.
it is an example, so user can modify it in whatever way

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 10 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Redundancy.

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 13 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

There is no -h/--help mentioned in the usage.

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 22 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

The provided argument is always set. No need to have a default.

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 27 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I am not sure why these two are squeezed in a single line.

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 27 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I am not sure why -u. It looks like you clean up afterwards, don't you?
-u  update files, create if necessary

-u just in case you use this code locally and you have already such folder - just to avoid overwright confirmation questions

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 28 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems you use double quotes vs single quotes pretty willy-nilly. I have no idea why in a single line you have both of them.

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 28 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think this procedure's robustness would benefit if we skip printing file names. TBH vos_pmemobj sounds like a file name already. In which case you would exclude all API calls from this file which would be bad.
       -h, --no-filename
              Suppress the prefixing of file names on output.  This is
              the default when there is only one file (or only standard
              input) to search.

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 29 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You do not need this bit at all if you use grep(1) to its full potential. Namely:
grep -r -E -h -o 'pmemobj_[^( ]*\('
No file names. (-h)

Print only the matching bits.
-o, --only-matching
Print only the matched (non-empty) parts of a matching line, with each such part on a separate output line.
Use sed(1) just to remove the last character from all the lines.

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 38 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is the third copy of this value. What can we do about this?

Now we have two copies ;)

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 39 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 39 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Indentation.

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 53 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

The newline is missing.

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 6 at r1 (raw file):

#
#
# Execute complete call stack analysis filtrered for DAOS only funcion calls

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 8 at r1 (raw file):

# Execute complete call stack analysis filtrered for DAOS only funcion calls
#
# update examples/api_filter.txt file based on actual DAOS source code using:

Done.

utils/call_stacks_analysis/do_call_stack_analysis_for_daos.sh line 17 at r1 (raw file):

        echo "       $0 [-u|--update]"
        echo
        echo "       -u/--update       use actual pmemobj api call from DAOS source code repo"

Done.

utils/call_stacks_analysis/make_cflow.sh line 18 at r1 (raw file):

	echo "cflow not found in PATH, please check if it is available."
	echo
	echo "You can download the lates source code of cflow from:" \

Done.

grom72

Reviewed 4 of 7 files at r2, all commit messages.
Reviewable status: 5 of 8 files reviewed, 20 unresolved discussions (waiting on @janekmi)

janekmi

Reviewed 6 of 7 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @grom72)

utils/call_stacks_analysis/make_stack_usage.sh line 15 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Because push/popd is design for such situation
cd - is about switching to the previous one

As far as I can tell, pushd/popd are designed to manipulate the directories stack. You do not even have to change the working directory with each of the pushd calls. So, I would argue it is not exactly our use case:

We do not use more than one directory at the time.
We always move along with the directory we use.

So, again, why do you think pushd/popd is better?

Sorry for being picky but when you start to replace things around with their functional equivalents based on some belief you have to sell it well. ;-)

utils/call_stacks_analysis/examples/api_filter.txt line 1 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Hmmm

the problem is that we can not easily check if new functions has been added. If we do not have header simple git diff says that.

it is an example, so user can modify it in whatever way

I think we can just drop the pretence. It is daos_api_filter.txt. And it is the only one we are interested in maintaining right now. If we just rename the file:

The comment would become unnecessary.
User can always copy the file and change its contents to their heart's content.
In this case we probably should make the API filter an argument of the run_call_stack_analysis.sh.

utils/call_stacks_analysis/README.md line 20 at r3 (raw file):

    --filter-lower-limit 11568 --dump-all-stacks

or

I think this becomes redundant. Anybody can look inside the run_call_stack_analysis.sh and read it.

.github/workflows/scans.yml line 9 at r3 (raw file):

    # run this job at 00:00 UTC every day
    - cron:  '0 0 * * *'
  pull_request:

What is it?

utils/call_stacks_analysis/install_cflow.sh line 6 at r3 (raw file):

#
#
# Install cflow tool

Suggestion:

Install the cflow tool.

utils/call_stacks_analysis/install_cflow.sh line 10 at r3 (raw file):

function main() {
	if  which "cflow" >/dev/null 2>&1; then

Redundant.

Suggestion:

if  which cflow >/dev/null 2>&1; then

utils/call_stacks_analysis/install_cflow.sh line 12 at r3 (raw file):

	if  which "cflow" >/dev/null 2>&1; then
		echo "cflow already installed"
		#return 0

Very missing.

Suggestion:

return 0

utils/call_stacks_analysis/install_cflow.sh line 17 at r3 (raw file):

		tar -xvzf cflow-latest.tar.gz && \
		rm cflow-latest.tar.gz
	echo $?

I guess it was here for debug purposes.

utils/call_stacks_analysis/install_cflow.sh line 19 at r3 (raw file):

	echo $?
	if [ $? != 0 ]; then
		echo "Can not download cflow source code"

Suggestion:

Can not download the cflow source code

utils/call_stacks_analysis/install_cflow.sh line 19 at r3 (raw file):

	echo $?
	if [ $? != 0 ]; then
		echo "Can not download cflow source code"

To be exact you may end up here when any of these fails:

download
untar
rm

utils/call_stacks_analysis/install_cflow.sh line 34 at r3 (raw file):

}

main

The existence of the main function looks redundant in this case. But I will blame it on the new style you employ here. I will give it a pass for now.

utils/call_stacks_analysis/install_cflow.sh line 36 at r3 (raw file):

main

exit $?

What is it's role?

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 6 at r3 (raw file):

#
#
# Execute complete call stack analysis based on API subset defined in examples/api_filter.txt.

Suggestion:

Execute complete call stack analysis based on the API subset defined in examples/api_filter.txt.

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 13 at r3 (raw file):

function help() {
        echo "usage:"
        echo "       $0 [-h|--help] [-d|--daos]"

Keeping a complete list of options here seems a bad idea.

Suggestion:

[OPTION]"

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 21 at r3 (raw file):

function main() {
        local update=$1

Otherwise it is getting confusing.

Suggestion:

local daos=$1

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 29 at r3 (raw file):

                rm daos.zip
                grep -r -E -h -o 'pmemobj_[^(]*\(' 'daos-master/src' | sed 's/(.*$//' | \
                        sort | uniq > $filter_name

It looks like calling sed as the last one would mean less work for it to do. It would make it insignificantly faster I suppose.

Suggestion:

                grep -r -E -h -o 'pmemobj_[^(]*\(' 'daos-master/src' | \
                        sort | uniq | sed 's/(.*$//' > $filter_name

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 63 at r3 (raw file):

main $daos

exit $?

.

grom72

Reviewed 1 of 4 files at r1, 1 of 7 files at r2.
Reviewable status: 2 of 8 files reviewed, 15 unresolved discussions (waiting on @janekmi)

utils/call_stacks_analysis/install_cflow.sh line 10 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Redundant.

Done.

utils/call_stacks_analysis/install_cflow.sh line 12 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Very missing.

Done.

utils/call_stacks_analysis/install_cflow.sh line 17 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I guess it was here for debug purposes.

Done.

utils/call_stacks_analysis/install_cflow.sh line 19 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

To be exact you may end up here when any of these fails:

download

untar

rm

Hmmm I do not expect that rm or tar may fail in "normal situation.

utils/call_stacks_analysis/install_cflow.sh line 36 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What is it's role?

Done

utils/call_stacks_analysis/README.md line 20 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think this becomes redundant. Anybody can look inside the run_call_stack_analysis.sh and read it.

Done.

utils/call_stacks_analysis/examples/api_filter.txt line 1 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think we can just drop the pretence. It is daos_api_filter.txt. And it is the only one we are interested in maintaining right now. If we just rename the file:

The comment would become unnecessary.

User can always copy the file and change its contents to their heart's content.

In this case we probably should make the API filter an argument of the run_call_stack_analysis.sh.

Done.

.github/workflows/scans.yml line 9 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What is it?

It was described in commit message, ...

utils/call_stacks_analysis/make_stack_usage.sh line 15 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

As far as I can tell, pushd/popd are designed to manipulate the directories stack. You do not even have to change the working directory with each of the pushd calls. So, I would argue it is not exactly our use case:

We do not use more than one directory at the time.

We always move along with the directory we use.

So, again, why do you think pushd/popd is better?

Sorry for being picky but when you start to replace things around with their functional equivalents based on some belief you have to sell it well. ;-)

Done.
I can not agree but it does not add any value to this PR ;)

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 13 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Keeping a complete list of options here seems a bad idea.

Done.

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 21 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Otherwise it is getting confusing.

Done.

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 29 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It looks like calling sed as the last one would mean less work for it to do. It would make it insignificantly faster I suppose.

I dont think 0.02s is worth of any changes here ;)

your proposal:

real    0m0.053s
user    0m0.020s
sys     0m0.033s

vs original:

real    0m0.077s
user    0m0.019s
sys     0m0.042s

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 63 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 6 at r3 (raw file):

#
#
# Execute complete call stack analysis based on API subset defined in examples/api_filter.txt.

Done.

utils/call_stacks_analysis/install_cflow.sh line 6 at r3 (raw file):

#
#
# Install cflow tool

Done.

utils/call_stacks_analysis/install_cflow.sh line 19 at r3 (raw file):

	echo $?
	if [ $? != 0 ]; then
		echo "Can not download cflow source code"

Done.

grom72

Reviewed 1 of 6 files at r4.
Reviewable status: 3 of 8 files reviewed, 15 unresolved discussions (waiting on @janekmi)

grom72

Reviewed 2 of 6 files at r4.
Reviewable status: 5 of 8 files reviewed, 15 unresolved discussions (waiting on @janekmi)

grom72

Reviewed 2 of 6 files at r4, all commit messages.
Reviewable status: 7 of 8 files reviewed, 15 unresolved discussions (waiting on @janekmi)

grom72

Reviewed 1 of 6 files at r4.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @janekmi)

janekmi

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @grom72 and @janekmi)

utils/call_stacks_analysis/examples/api_filter.txt line 1 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done.

Do you plan to rename this file?

.github/workflows/scans.yml line 9 at r3 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

It was described in commit message, ...

Sorry. I missed that bit. But I see you was not very persistent either. ;-)

utils/call_stacks_analysis/make_stack_usage.sh line 15 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done.
I can not agree but it does not add any value to this PR ;)

It does. Limiting random changes which are very confusing when browsing the repository's history is always worth fighting for. ;-)

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 29 at r3 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

I dont think 0.02s is worth of any changes here ;)

your proposal:
real    0m0.053s
user    0m0.020s
sys     0m0.033s
vs original:
real    0m0.077s
user    0m0.019s
sys     0m0.042s

This is why it was a non-blocking comment. ;-)

It is not only 0.02s. It is over 30%. xD

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 63 at r3 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done.

I still don't know what it is for here.

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 24 at r4 (raw file):

        echo "  -f, --filter FILE  custom API filter file"
        echo
        exit 1

It is not an error to call for help. Please either handle cleanly both cases (just printing help vs printing an error) or keep it separately. It looks a little weird to call:

help "Unknown option: $1"

How about call it as follows:

error "Unknown option: $1"

Where the error() function would both:

print the error message
call help() to print the correct usage
exit 1 since it is an error case.

Whereas exit 0 would be called by all other callers of the help() function.

Suggestion:

exit 0

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 59 at r4 (raw file):

update=0
filter_name="examples/api_filter.txt"
while [[ "$#" -gt 0 ]]; do

Redundancy.

Suggestion:

[ "$#" -gt 0 ]

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 79 at r4 (raw file):

if [ -z "$filter_name" ]; then
        HELP "File parameter is missing"

Are you actually calling for help or what? ;-)

Suggestion:

help

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 82 at r4 (raw file):

fi

main $update $filter_name

Since the path may contains spaces.

Suggestion:

main $update "$filter_name"

utils/call_stacks_analysis/install_cflow.sh line 22 at r4 (raw file):

tar -xvzf cflow-latest.tar.gz
rm cflow-latest.tar.gz
cd cflow-*

Suggestion:

tar -xvzf cflow-latest.tar.gz && cd cflow-*
if [ $? != 0]; then
	echo "Unexpected contents of the source code package"
	exit 1
fi
rm cflow-latest.tar.gz

utils/call_stacks_analysis/install_cflow.sh line 31 at r4 (raw file):

if [ $status != 0 ]; then
	echo "Can not install the cflow"

Suggestion:

Can not install cflow

janekmi

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72)

utils/call_stacks_analysis/install_cflow.sh line 28 at r5 (raw file):

fi

cd cflow-*

Trailing space.

grom72

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @janekmi)

utils/call_stacks_analysis/install_cflow.sh line 28 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Trailing space.

Done.

utils/call_stacks_analysis/examples/api_filter.txt line 1 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Do you plan to rename this file?

No - it is just an example

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 63 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I still don't know what it is for here.

to return the status of main function call as final result of script execution

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 24 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is not an error to call for help. Please either handle cleanly both cases (just printing help vs printing an error) or keep it separately. It looks a little weird to call:
help "Unknown option: $1"
How about call it as follows:
error "Unknown option: $1"
Where the error() function would both:

print the error message

call help() to print the correct usage

exit 1 since it is an error case.

Whereas exit 0 would be called by all other callers of the help() function.

Done.

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 59 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Redundancy.

Done.

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 79 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Are you actually calling for help or what? ;-)

Done.

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 82 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Since the path may contains spaces.

Done.

utils/call_stacks_analysis/install_cflow.sh line 22 at r4 (raw file):

tar -xvzf cflow-latest.tar.gz
rm cflow-latest.tar.gz
cd cflow-*

Done.

utils/call_stacks_analysis/install_cflow.sh line 31 at r4 (raw file):

if [ $status != 0 ]; then
	echo "Can not install the cflow"

Done.

grom72

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi)

janekmi

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72, @janekmi, and @osalyk)

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 63 at r3 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

to return the status of main function call as final result of script execution

It is redundant. Please remove it.

A comprehensive analysis of stack sizes requires several steps. To facilitate this process, these steps have been aggregated into a single script run_call_stacks_analysis.sh. In addition, it is possible to perform this operation based on the set of functions used in the DAOS project. The cflow program, a key component of stack analysis, is not available by default on Linux. The install_cflow.sh script solves this problem by downloading the sources, compiling and then installing the cflow program on the system. Both scripts has been integrated into their respective GHA workflows to avoid code redundancy. Signed-off-by: Tomasz Gromadzki <[email protected]>

grom72

Reviewed 1 of 1 files at r3, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)

utils/call_stacks_analysis/run_call_stacks_analysis.sh line 63 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is redundant. Please remove it.

Done.

janekmi

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

osalyk

Reviewed 1 of 7 files at r2, 1 of 1 files at r3, 4 of 6 files at r4, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)

grom72 requested a review from janekmi March 7, 2025 14:59

grom72 added the no changelog Add to skip the changelog check on your pull request label Mar 7, 2025

janekmi reviewed Mar 7, 2025

View reviewed changes

grom72 force-pushed the call-stack-analysis-for-daos branch from 4d1e4bb to c5f381d Compare March 9, 2025 14:53

grom72 commented Mar 9, 2025

View reviewed changes

grom72 force-pushed the call-stack-analysis-for-daos branch from c5f381d to 3cb5834 Compare March 10, 2025 07:59

grom72 commented Mar 10, 2025

View reviewed changes

grom72 force-pushed the call-stack-analysis-for-daos branch from 3cb5834 to aa52446 Compare March 10, 2025 09:05

janekmi reviewed Mar 10, 2025

View reviewed changes

grom72 force-pushed the call-stack-analysis-for-daos branch from aa52446 to c09d0eb Compare March 10, 2025 10:49

grom72 commented Mar 10, 2025

View reviewed changes

grom72 requested a review from janekmi March 10, 2025 12:51

janekmi reviewed Mar 10, 2025

View reviewed changes

grom72 force-pushed the call-stack-analysis-for-daos branch from c09d0eb to 1772630 Compare March 10, 2025 17:32

janekmi reviewed Mar 10, 2025

View reviewed changes

grom72 force-pushed the call-stack-analysis-for-daos branch 2 times, most recently from da6a8bc to ba6f989 Compare March 10, 2025 20:32

grom72 commented Mar 10, 2025

View reviewed changes

grom72 commented Mar 11, 2025

View reviewed changes

grom72 marked this pull request as ready for review March 11, 2025 07:03

grom72 requested review from janekmi and osalyk March 11, 2025 07:03

janekmi requested changes Mar 11, 2025

View reviewed changes

grom72 force-pushed the call-stack-analysis-for-daos branch from ba6f989 to f1434ef Compare March 11, 2025 11:28

grom72 commented Mar 11, 2025

View reviewed changes

janekmi approved these changes Mar 11, 2025

View reviewed changes

osalyk approved these changes Mar 11, 2025

View reviewed changes

janekmi merged commit d4da1b1 into pmem:master Mar 11, 2025
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common: call stack analysis for DAOS #6157

common: call stack analysis for DAOS #6157

grom72 commented Mar 7, 2025 •

edited

Loading

janekmi left a comment

grom72 left a comment

grom72 left a comment

janekmi left a comment

grom72 left a comment

grom72 left a comment

grom72 left a comment

grom72 left a comment

grom72 left a comment

janekmi left a comment

janekmi left a comment

grom72 left a comment

grom72 left a comment

janekmi left a comment

grom72 left a comment

janekmi left a comment

osalyk left a comment

common: call stack analysis for DAOS #6157

common: call stack analysis for DAOS #6157

Conversation

grom72 commented Mar 7, 2025 • edited Loading

janekmi left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

osalyk left a comment

Choose a reason for hiding this comment

grom72 commented Mar 7, 2025 •

edited

Loading