pmem / pmemfile

Userspace implementation of file APIs using persistent memory.
Other
34 stars 21 forks source link

antool: add support for symlinks #379

Closed ldorau closed 6 years ago

ldorau commented 7 years ago

This change is Reviewable

codecov[bot] commented 7 years ago

Codecov Report

Merging #379 into master will increase coverage by 0.22%. The diff coverage is 98.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #379      +/-   ##
=========================================
+ Coverage   76.87%   77.1%   +0.22%     
=========================================
  Files          67      67              
  Lines        8276    8302      +26     
  Branches     1665    1668       +3     
=========================================
+ Hits         6362    6401      +39     
+ Misses       1495    1486       -9     
+ Partials      419     415       -4
Flag Coverage Δ
#ltp_tests 46.54% <ø> (+0.1%) :arrow_up:
#sqlite_tests 45.26% <ø> (+0.1%) :arrow_up:
#tests_antool 15.14% <98.88%> (+0.42%) :arrow_up:
#tests_posix_multi_threaded 25.07% <ø> (+0.02%) :arrow_up:
#tests_posix_single_threaded 54.31% <ø> (+0.1%) :arrow_up:
#tests_preload 44.76% <ø> (+0.09%) :arrow_up:
Impacted Files Coverage Δ
src/tools/antool/antool.py 100% <100%> (ø) :arrow_up:
src/tools/antool/listsyscalls.py 99.47% <98.78%> (+0.03%) :arrow_up:
src/libpmemfile-posix/dir.c 84.76% <0%> (-0.25%) :arrow_down:
src/libpmemfile-posix/lseek.c 84.56% <0%> (-0.11%) :arrow_down:
src/libpmemfile-posix/block_array.c 96.77% <0%> (+0.02%) :arrow_up:
src/libpmemfile-posix/inode.c 88.65% <0%> (+0.34%) :arrow_up:
src/libpmemfile-posix/file.c 75.13% <0%> (+0.41%) :arrow_up:
src/libpmemfile-posix/rename.c 92% <0%> (+0.73%) :arrow_up:
src/libpmemfile-posix/stats.c 57.5% <0%> (+1.4%) :arrow_up:
src/libpmemfile-posix/fallocate.c 61.9% <0%> (+1.43%) :arrow_up:
... and 3 more

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 af46c7f...c3d9ea3. Read the comment docs.

sarahjelinek commented 6 years ago

Reviewed 15 of 18 files at r1, 10 of 11 files at r2. Review status: 24 of 27 files reviewed at latest revision, 4 unresolved discussions.


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


    parser.add_argument("--slink_os", action='store_true', required=False, help="resolve symlinks in the current OS")
    parser.add_argument("--slink_file", required=False, help="resolve symlinks saved in the given file")

Why is symlink support a command line option?


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


                if self.slink_file and new_path in self.symlinks:
                    new_path = self.symlinks[new_path]

Is the point that if it is already a resolved symlink we just assign it based on its previously resolved value? Maybe add comments here.


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

    # handle_fileat -- helper function of match_fd_with_path() - handles *at syscalls
    ####################################################################################################################
    def handle_fileat(self, syscall, arg1, arg2, msg, has_to_be_pmem):

What does has_to_be_pmem mean/do? Please add a comment.


tests/antool/test_syscalls.c, line 544 at r2 (raw file):

verify_and_open(const char *dir, const char *pmem, const char *nonp,
      char **abspmem_p, char **absnonp_p,
      int *dirfd_p, int *fdpmem_p, int *fdnonp_p)

Could you add comments as to what the variable names represent in the function header?


Comments from Reviewable

ldorau commented 6 years ago

Review status: 16 of 21 files reviewed at latest revision, 4 unresolved discussions.


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

Previously, sarahjelinek (Sarah Jelinek) wrote…
Why is symlink support a command line option?

I removed the '--slink_os' option and it is always enabled now. The '--slink_file' option is used to give a file with list of symlinks which existed when the app was running but are no longer valid.


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

Previously, sarahjelinek (Sarah Jelinek) wrote…
Is the point that if it is already a resolved symlink we just assign it based on its previously resolved value? Maybe add comments here.

I added the comment:

# if there is a file with symlinks given by the '--slink_file' CLI option
# and the current 'new_path' is saved as a symlink in this file,
# replace it with the target path saved also in this file.

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

Previously, sarahjelinek (Sarah Jelinek) wrote…
What does has_to_be_pmem mean/do? Please add a comment.

I added the comment :

################################################################################################
# handle_fileat -- helper function of match_fd_with_path() - handles *at syscalls
#    syscall        - current syscall
#    arg1           - number of the argument with a file descriptor
#    arg2           - number of the argument with a path
#    msg            - message to be printed out built so far
#    has_to_be_pmem - if set, force the new path to be pmem (in case of SyS_symlinkat)
################################################################################################

tests/antool/test_syscalls.c, line 544 at r2 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…
Could you add comments as to what the variable names represent in the function header?

I added a comment


Comments from Reviewable

ldorau commented 6 years ago

Review status: 16 of 21 files reviewed at latest revision, 4 unresolved discussions.


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

Previously, ldorau (Lukasz Dorau) wrote…
I removed the '--slink_os' option and it is always enabled now. The '--slink_file' option is used to give a file with list of symlinks which existed when the app was running but are no longer valid.

So symlink support is always enabled now.


Comments from Reviewable

sarahjelinek commented 6 years ago
:lgtm:

Review status: 16 of 21 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

sarahjelinek commented 6 years ago

Reviewed 5 of 5 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable