Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use virtual file system when checking CRC #6

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

alex-schulster
Copy link

Hi, I noticed the CRC file which verifies the downloaded file checksum does not use the file system provided in the user config.

This causes an issue for a file system which changes the root location of CFDP files (I created a file systems which handles all cfdp files in /tmp/cfdp/ to avoid polluting my executable dir for example).

I therefore edited the crc file to make use of this vfs.

All the tests have been run and passed successfully:

❯ python -m pytest
================================ test session starts =================================
platform linux -- Python 3.10.12, pytest-8.0.0, pluggy-1.4.0
rootdir: /home/alex/code/python/cfdppy/module
plugins: pyfakefs-5.3.5
collected 77 items

tests/test_checksum.py .                                                       [  1%]
tests/test_dest_handler_acked.py ...............                               [ 20%]
tests/test_dest_handler_naked.py ..............                                [ 38%]
tests/test_filestore.py ........                                               [ 49%]
tests/test_lost_seg_tracker.py .............                                   [ 66%]
tests/test_request.py ...                                                      [ 70%]
tests/test_src_handler_acked.py ........                                       [ 80%]
tests/test_src_handler_nak_closure.py .......                                  [ 89%]
tests/test_src_handler_nak_no_closure.py ........                              [100%]

================================= 77 passed in 1.31s =================================

Also minor adjusments in other files:

  • Authorize HostFilestore to create parent dirs when creating file
  • Replace absolute imports with relative imports
  • Typing consistency between HostFilestore and VirtualFilestore

@robamu
Copy link
Contributor

robamu commented Feb 21, 2024

Thanks for this PR. Some questions I have:

  1. Is there a special advantage for using relative imports instead of absolute ones?
  2. I am not fully sure whether the automatic creation is the best solution. I'd like this interface to behave closely to a UNIX filesystem. For example, when I use touch /tmp/test/hello.txt and test does not exist in tmp, the command will fail. I think it might be a better idea to inform the user that the filesystem structure might not be the way they expect it to be.

To be honest, practically it might also be a good idea to handle the CRC inside the VFS via a dedicated interface function. The VFS knows the most efficient way to read/write a file, so it would also know the most efficient way to calculate the CRC of a file. In C++/Rust, the CRC calculation being performed by the VFS was the only practical approach. I might consider moving the CRC calculation into the VFS in the future..

@robamu
Copy link
Contributor

robamu commented Aug 20, 2024

I have made the changes mentioned in this PR: #11

The CRC calculation is now part of the VFS abstraction and I also fixed a bug where the CRC algorithm was not used for empty files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants