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

Introduce virtual file system abstraction #30

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

fknittel
Copy link
Contributor

(Unfortunately, this is again based on the previous two pull requests / branches. I'll happily rebase if you're not interested in pulling them.)

This basically re-implements my older patch from the old fabians-patches branch. The basic idea is to abstract away all file system access, allowing various neat tricks.

Since my last implementation you've added a dyn_file_func call-back, but unfortunately this doesn't cleanly cover the intended use-cases:

  • even when using dyn_file_func, the file-system is checked anyway (although I see that there's a different pull request that would change this)
  • if the dynamic function uses the file-system in some way, it has to re-implement the file-system access stuff again - it can't re-use tftpy's code.
  • tftpy can't easily provide multi-root support

The patches attempt to stay as compatible to the old interface as possible. If you find the approach generally acceptable, I'll continue along those lines to properly support tsize. (Right now tsize appears to be broken.)

The patches have the side-effect of accepting files starting with "/". BOOTP implementations appear to use this by default, so I see this as a feature, not a bug. :)

Please feel free to request smaller patches / reworked patches. I'm also willing to discuss a different approach.

`self.dyn_file_func` is currently set twice: Once in the base class and once in
the server child class.  As it's only used in the non-server case, remove it
from the base class.
The test currently fails, because the request path is improperly checked /
sanitised.
This patch fixes the request path check.  It makes sure that requested paths
are _below_ the specified root directory.
This patch modifies the TFTP server code to depend on a virtual file system
abstraction for all file-system access.  The previous file system access code
is moved to the TftpVfsNative class.  The patch also provides a TftpVfsStack
class, which allows multiple VFS classes to be combined into one name space.

Includes unit tests.
Allows the ``--root`` option to be specified more than once.  Any write
requests are sent to the first root.
@vitaminmoo
Copy link

This feature would make what I am trying to do significantly easier and more powerful.

@ghost ghost assigned msoulier Jul 28, 2013
@msoulier msoulier added the pending Not looking at it yet label May 9, 2018
@msoulier
Copy link
Owner

msoulier commented May 9, 2018

If you're still interested in this, maybe we should chat. I've obviously neglected this for a long time so I'll understand if you're not interested. I'm trying to rectify that now, and I don't think I fully understand what you're doing here.

@msoulier msoulier added feedback needed Commented and awaiting feedback and removed pending Not looking at it yet labels May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback needed Commented and awaiting feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants