ldorau commented 7 years ago

This PR depends on PR https://github.com/pmem/pmemfile/pull/346, which has to be merged first. Only 2 top-last commits actually belongs to this PR.

This change is

codecov[bot] commented 7 years ago

Codecov Report

Merging #341 into master will increase coverage by 0.31%. The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #341      +/-   ##
=========================================
+ Coverage   74.09%   74.4%   +0.31%     
=========================================
  Files          64      64              
  Lines        7999    7999              
  Branches     1617    1617              
=========================================
+ Hits         5927    5952      +25     
+ Misses       1562    1541      -21     
+ Partials      510     506       -4

Flag	Coverage Δ
#sqlite_tests	`45.87% <ø> (ø)`	:arrow_up:
#tests_antool	`12.46% <ø> (+0.26%)`	:arrow_up:
#tests_posix_multi_threaded	`25.93% <ø> (-0.02%)`	:arrow_down:
#tests_posix_single_threaded	`56% <ø> (ø)`	:arrow_up:
#tests_preload	`46.12% <ø> (-0.08%)`	:arrow_down:

Impacted Files	Coverage Δ
src/libpmemfile-posix/dir.c	`85.74% <0%> (+0.49%)`	:arrow_up:
src/tools/antool/listsyscalls.py	`76.99% <0%> (+0.7%)`	:arrow_up:
src/libpmemfile/vfd_table.c	`81.16% <0%> (+0.89%)`	:arrow_up:
src/tools/antool/antool.py	`71.27% <0%> (+1.09%)`	:arrow_up:
src/tools/antool/syscall.py	`79.07% <0%> (+4.3%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a7f1d34...1b551ef. Read the comment docs.

sarahjelinek commented 7 years ago

Reviewed 99 of 125 files at r1, 4 of 59 files at r2. Review status: 57 of 133 files reviewed at latest revision, 5 unresolved discussions.

src/tools/antool/antool.py, line 313 at r2 (raw file):

            # read header = command line
            data_size, argc = read_fmt_data(fh, 'ii')
            data_size -= sizei

Why do you decrement data_size by sizei when you have read 'ii'? Which is 2 * sizei? A comment would be helpful here.

src/tools/antool/antool.py, line 348 at r2 (raw file):

                # read data from the file
                data_size, info_all, pid_tid, sc_id, timestamp = read_fmt_data(fh, 'IIQQQ')
                data_size -= sizeIQQQ

data_size -= sizeIQQQ when you have read 'IIQQQ'?

tests/antool/output-err-1531-12.log.match, line 0 at r2 (raw file): Why are some of these empty files?

tests/antool/test-antool.sh, line 47 at r2 (raw file):

if [ "$4" == "" ]; then
  echo "ERROR($NAME): not enough arguments"
  echo "Usage: $0 [-f] <path-to-vltrace> <max-string-length> <test-app> <test-number>"

What is max-string-length used for and how would a user know what this length should be if they ran this by hand?

tests/antool/test-fi.sh, line 34 at r2 (raw file):

#
#
# test-others.sh -- other test for analyzing tool

Why is this name test-fi.sh when internally you call it test-others.sh?

Comments from Reviewable

ldorau commented 7 years ago

Review status: 57 of 133 files reviewed at latest revision, 5 unresolved discussions.

src/tools/antool/antool.py, line 313 at r2 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…

Why do you decrement data_size by sizei when you have read 'ii'? Which is 2 * sizei? A comment would be helpful here.

A comment added:

# subtract size of argc only, because 'data_size' does not include its own size

src/tools/antool/antool.py, line 348 at r2 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…

data_size -= sizeIQQQ when you have read 'IIQQQ'?

A comment added:

# subtract size of all read fields except of 'data_size' itself,
# because 'data_size' does not include its own size

tests/antool/output-err-1531-12.log.match, line at r2 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…

Why are some of these empty files?

Because there should be no stderr output for these tests.

tests/antool/test-antool.sh, line 47 at r2 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…

What is max-string-length used for and how would a user know what this length should be if they ran this by hand?

This is a parameter of vltrace. It is described in the manual and the help message of vltrace. I added a comment here. This changed was moved to a separate PR: https://github.com/pmem/pmemfile/pull/346 which this PR depends on now.

tests/antool/test-fi.sh, line 34 at r2 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…

Why is this name test-fi.sh when internally you call it test-others.sh?

Just a mistake - it was the previous, old name. Fixed.

Comments from Reviewable

ldorau commented 7 years ago

This PR depends on PR https://github.com/pmem/pmemfile/pull/346, which has to be merged first. Only 2 top-last commits actually belongs to this PR.

ldorau commented 7 years ago

Review status: 51 of 75 files reviewed at latest revision, 5 unresolved discussions.

tests/antool/output-err-1531-12.log.match, line at r2 (raw file):