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

File-system refactor - plus minor bugfixes #395

Merged
merged 1 commit into from
Oct 6, 2024

Conversation

lmagyar
Copy link
Contributor

@lmagyar lmagyar commented Oct 3, 2024

Important: Root is not tested. It builds, there is no functional change in theory, but a quick test needed. I can't test root. The only functional changes:

  • create() is implemented with a copy-paste "touch" command, to be in sync with other implementations (FS, SAF) who do it for SSHFS
  • delete() also notifies MediaScanner, to be in sync with other implementations (FS)

Tests are running (except root, I can't test it), Quick Share and Media Scanner tested manually on real Android 9 phone.

Changes:

1. remove file property "caching" (speed did not change)

  • all "cached" properties become abstract in AbstractFile and got implemented in descendants
    • lastModified
    • size
    • readable
    • exists
    • isDirectory
  • additionally some strange SCP (eg. SAF breaks recursive copy to phone #205) and SSHFS issues are caused exactly by this caching, because it is OK for "one-off" usage, but SCP and SSHFS reuse handles, ie. after calling eg. an mkdir() on a file, SCP calls a doesExist(), and without refreshing these cached values, they fail (in case of SSHFS I've added some cache update previously in SAF, that was hacky, and now got removed)
  • only absPath and name remained in AbstractFile
    • they are used in nearly each log message, and at several other places
    • only move can change them, but in case of move, if we update these, we should update all other specific properties (file, bean, documentFile), but they are usually final, better not to go down this path, maybe later, if there is a real reason

2. File and FileSystemView (Fs, Root, QuickShare, Saf, RoSaf, Virtual) ctor/createFile/getFile simplification

  • all FileSystemView class has PftpdService as first argument, all arguments that can be reached through PftpdService got removed (Context, ContentResolver, etc.)
    • created a new AbstractFileSystemView class for this pftpdService and logger properties
  • all File class got a fileSystemView property, and a getFileSystemView() method, that handles the type properly in descendants (some Java generic magic)
    • this fileSystemView is the first argument in each ctor, all arguments that can be reached through fileSystemView (and PftpdService) got removed (Context, ContentResolver, StartUrl, etc.)
  • added a public abstract boolean move(AbstractFile destination) to AbstractFile, it was missing, though we still need a wrapper in xxxFtpFile and xxxSshFile classes
    • implementations in FsFile and RootFile calls Utils.mediaScanFile on the old and new file name, to remove and add them
  • reorganised the order of the method arguments in ctor/createFile/getFile methods to follow class hierarchy
  • this way a lot of clutter got removed, it is so clean now what is where, how to access it, etc.
  • plus some minor log changes (to log the return values (more) consistently)

3. mkdirs (note the 's' at the end) behavior in SAF

  • in case of SAF if there is non-existent dirs in path for a new file, it's near impossible to create the file, we do not have the Uri for the nonexistent dirs, and there is no mkdirs() for DocumentFile
  • this was an issue (or seemed to be) in SAF breaks recursive copy to phone #205, and an open issue in Speed up SAF 100x #372 and Refactor ROSAF #373
  • so SafFile in this case receives the list of the nonexistent parent folders, and creates them when needed (in mkdir() and in createOutputStream())
  • this fixes Speed up SAF 100x #372 (SAF), but roSaf is still unchanged, I will do it when I return to make it RW
  • the fun fact, that SAF breaks recursive copy to phone #205 got fixed with the cache removing above (SCP executes the necessary mkdir), but this fix is good for a direct SFTP file upload into a nonexistent dir

Note: mkdirs got backported into FsFile.mkdir(), but in RootFile.mkdir() and RootFile.createOutputStream() I've added only comments, I can't test it, better not modify it.

4. fix MediaScanner in FS an Root

  • the original implementation never worked (see Add media-scanning after upload (Writing files via root method produces different results from SAF method) #336)
    • it used AsyncTask, but that can be used only on the UI thread
    • did not call execute() on it (it would do nothing on the background thread)
    • did not call connect() on the scanner connection, without this scanFile() would throw exception
  • I refactored the functionality into a new class MediaScannerClient
    • it is instantiated and connected (without delay) in FS and Root FileSystemView ctor
    • it is used also in delete()
    • it uses only synchronize/wait/notifyAll
    • I've checked the Android source code about the implementation changes, it should work with all versions
    • tested on real Android 9 phone, uploaded/deleted FS images immediately appear/disappear from gallery

Note: as mentioned above, not tested on rootFS, in theory it should work, the same code is running like under FS, but a quick manual test won't harm.


The log that shows how 3. mkdirs works in SAF with SFTP

org.apache.sshd.server.sftp.SftpSubsystem                 Received SSH_FXP_OPEN (path=/saf/Test/dir/file.txt, pflags=26, attrs={})
org.primftpd.filesystem.VirtualSshFileSystemView          getFile '/saf/Test/dir/file.txt', absolute: '/saf/Test/dir/file.txt'
org.primftpd.filesystem.VirtualSshFileSystemView          Using SAF '/Test/dir/file.txt' for '/saf/Test/dir/file.txt'
org.primftpd.filesystem.SafSshFileSystemView              getFile(/Test/dir/file.txt), startUrl: content://com.android.externalstorage.documents/tree/17FB-0715%3A
org.primftpd.filesystem.SafSshFileSystemView                getFile(abs: /Test/dir/file.txt)
org.primftpd.filesystem.SafSshFileSystemView                  getFile(normalized: [Test, dir, file.txt])
org.primftpd.filesystem.SafSshFileSystemView                  building children uri for parent: 17FB-0715:
org.primftpd.filesystem.SafSshFileSystemView                  building children uri for parent: 17FB-0715:Test
org.primftpd.filesystem.SafSshFileSystemView                  calling createFile() for doc: file.txt, parentId: 17FB-0715:Test, parentUri: content://com.android.externalstorage.documents/tree/17FB-0715%3A/document/17FB-0715%3ATest, parentNonexistentDirs: /dir
--------------------------------------------------------------------------------------------------------------------v Utils.toPath() adds the extra '/', this is just in logging for the List<String>
org.primftpd.filesystem.SafSshFile                        new SafFile() with name 'file.txt', parent 'Test', dirs '/dir' and absPath '/Test/dir/file.txt'
org.primftpd.filesystem.SafSshFile                        [file.txt] getName()
org.primftpd.filesystem.SafSshFile                        [file.txt] getLastModified() -> 0
org.primftpd.filesystem.SafSshFile                        [file.txt] getSize() -> 0
org.primftpd.filesystem.SafSshFile                        [file.txt] isReadable() -> false
org.primftpd.filesystem.SafSshFile                        [file.txt] doesExist() -> false
org.primftpd.filesystem.SafSshFile                        [file.txt] isWritable() -> true
org.primftpd.filesystem.VirtualSshFile                    [file.txt] create()
org.primftpd.filesystem.SafSshFile                        [file.txt] create()
org.primftpd.filesystem.SafSshFile                        [file.txt] createNewFile()
---------------------------------------------------------------------v mkdirs() for SAF
org.primftpd.filesystem.SafSshFile                        [file.txt] creating parent folder, parent of parent: 'Test', parent: 'dir'
org.primftpd.filesystem.SafSshFile                        [file.txt] getLastModified() -> 1727639388000
org.primftpd.filesystem.SafSshFile                        [file.txt] isReadable() -> true
org.primftpd.filesystem.SafSshFile                        [file.txt] isWritable() -> true
org.primftpd.filesystem.VirtualSshFile                    [file.txt] truncate()
org.primftpd.filesystem.VirtualSshFile                    [file.txt] setAttributes()

The log that shows, that the 1. cache removing fixed SCP in #205

org.primftpd.filesystem.VirtualSshFileSystemView          getFile '/saf/Test/dir', absolute: '/saf/Test/dir'
org.primftpd.filesystem.VirtualSshFileSystemView          Using SAF '/Test/dir' for '/saf/Test/dir'
org.primftpd.filesystem.SafSshFileSystemView              getFile(/Test/dir), startUrl: content://com.android.externalstorage.documents/tree/17FB-0715%3A
org.primftpd.filesystem.SafSshFileSystemView                getFile(abs: /Test/dir)
org.primftpd.filesystem.SafSshFileSystemView                  getFile(normalized: [Test, dir])
org.primftpd.filesystem.SafSshFileSystemView                  building children uri for parent: 17FB-0715:
org.primftpd.filesystem.SafSshFileSystemView                  building children uri for parent: 17FB-0715:Test
org.primftpd.filesystem.SafSshFileSystemView                  calling createFile() for doc: dir, parentId: 17FB-0715:Test, parentUri: content://com.android.externalstorage.documents/tree/17FB-0715%3A/document/17FB-0715%3ATest
org.primftpd.filesystem.SafSshFile                        new SafFile() with name 'dir', parent 'Test', dirs '/' and absPath '/Test/dir'
org.primftpd.filesystem.SafSshFile                        [dir] getName()
org.primftpd.filesystem.SafSshFile                        [dir] getLastModified() -> 0
org.primftpd.filesystem.SafSshFile                        [dir] getSize() -> 0
org.primftpd.filesystem.SafSshFile                        [dir] isReadable() -> false
org.primftpd.filesystem.SafSshFile                        [dir] doesExist() -> false
----------------------------------------------------------------v SCP creates the parent dir
org.primftpd.filesystem.SafSshFile                        [dir] mkdir()
org.primftpd.filesystem.SafSshFile                        [dir] getAbsolutePath() -> '/Test/dir'
org.primftpd.services.SshServerService                    posting ClientActionEvent: ClientActionEvent{storage=SAF, protocol=SFTP, timestamp=Sun Sep 29 21:57:50 GMT+02:00 2024, clientIp='127.0.0.1', clientAction=CREATE_DIR, path='/Test/dir'}
org.apache.sshd.server.channel.ChannelSession             OUT: 00
org.apache.sshd.common.channel.Window                     Consume server remote window by 1 down to 2097150
org.apache.sshd.server.channel.ChannelSession             Send SSH_MSG_CHANNEL_DATA on channel 0
org.apache.sshd.server.session.ServerSession              Sending packet #11: 5e 00 00 00 00 00 00 00 01 00
org.apache.sshd.server.session.ServerSession              Received packet #12: 5e 00 00 00 00 00 00 00 13 43 30 36 36 36 20 35 20 66 69 6c 65 2d 61 2e 74 78 74 0a
org.apache.sshd.server.channel.ChannelSession             Received SSH_MSG_CHANNEL_DATA on channel ChannelSession[id=0, recipient=0]
org.apache.sshd.server.channel.ChannelSession             Received channel data: 43 30 36 36 36 20 35 20 66 69 6c 65 2d 61 2e 74 78 74 0a
org.apache.sshd.server.channel.PipeDataReceiver           IN:  43 30 36 36 36 20 35 20 66 69 6c 65 2d 61 2e 74 78 74 0a
...                                                       ...
org.apache.sshd.common.scp.ScpHelper                      Received header: C0666 5 file-a.txt
org.apache.sshd.common.scp.ScpHelper                      Receiving file org.primftpd.filesystem.VirtualSshFile@ccdd6d7
----------------------------------------------------------------v The value of 'exists' is modified in SafFile after cache removed, and VirtFile also returns this from SafFile (SCP reuses the [dir] SafFile)
org.primftpd.filesystem.SafSshFile                        [dir] doesExist() -> true
org.primftpd.filesystem.SafSshFile                        [dir] isDirectory() -> true
org.primftpd.filesystem.VirtualSshFile                    [dir] getAbsolutePath() -> '/saf/Test/dir'
org.primftpd.filesystem.VirtualSshFileSystemView          getFile(baseDir: /saf/Test/dir, file: file-a.txt)
org.primftpd.filesystem.VirtualSshFile                    [dir] getAbsolutePath() -> '/saf/Test/dir'
org.primftpd.filesystem.VirtualSshFileSystemView          getFile '/saf/Test/dir/file-a.txt', absolute: '/saf/Test/dir/file-a.txt'
org.primftpd.filesystem.VirtualSshFileSystemView          Using SAF '/Test/dir/file-a.txt' for '/saf/Test/dir/file-a.txt'
org.primftpd.filesystem.SafSshFileSystemView              getFile(/Test/dir/file-a.txt), startUrl: content://com.android.externalstorage.documents/tree/17FB-0715%3A
org.primftpd.filesystem.SafSshFileSystemView                getFile(abs: /Test/dir/file-a.txt)
org.primftpd.filesystem.SafSshFileSystemView                  getFile(normalized: [Test, dir, file-a.txt])
org.primftpd.filesystem.SafSshFileSystemView                  building children uri for parent: 17FB-0715:
org.primftpd.filesystem.SafSshFileSystemView                  building children uri for parent: 17FB-0715:Test
org.primftpd.filesystem.SafSshFileSystemView                  building children uri for parent: 17FB-0715:Test/dir
org.primftpd.filesystem.SafSshFileSystemView                  calling createFile() for doc: file-a.txt, parentId: 17FB-0715:Test/dir, parentUri: content://com.android.externalstorage.documents/tree/17FB-0715%3A/document/17FB-0715%3ATest%2Fdir
-------------------------------------------------------------------------------------------------------v Exists when scp starts to copy the file
org.primftpd.filesystem.SafSshFile                        new SafFile() with name 'file-a.txt', parent 'dir', dirs '/' and absPath '/Test/dir/file-a.txt'
org.primftpd.filesystem.SafSshFile                        [file-a.txt] getName()
org.primftpd.filesystem.SafSshFile                        [file-a.txt] getLastModified() -> 0
org.primftpd.filesystem.SafSshFile                        [file-a.txt] getSize() -> 0
org.primftpd.filesystem.SafSshFile                        [file-a.txt] isReadable() -> false
org.primftpd.filesystem.SafSshFile                        [file-a.txt] doesExist() -> false
org.primftpd.filesystem.SafSshFile                        [file-a.txt] doesExist() -> false
org.primftpd.filesystem.SafSshFile                        [file-a.txt] doesExist() -> false
org.primftpd.filesystem.SafSshFile                        [file-a.txt] createOutputStream(offset: 0)
org.primftpd.filesystem.SafSshFile                        [file-a.txt] getAbsolutePath() -> '/Test/dir/file-a.txt'
org.primftpd.services.SshServerService                    posting ClientActionEvent: ClientActionEvent{storage=SAF, protocol=SFTP, timestamp=Sun Sep 29 21:57:50 GMT+02:00 2024, clientIp='127.0.0.1', clientAction=UPLOAD, path='/Test/dir/file-a.txt'}
org.primftpd.filesystem.SafSshFile                        [file-a.txt] createNewFile()
org.primftpd.filesystem.SafSshFile                           createOutputStream() uri: content://com.android.externalstorage.documents/tree/17FB-0715%3A/document/17FB-0715%3ATest%2Fdir%2Ffile-a.txt

The log that shows how 4. MediaScanner works in FS

org.primftpd.services.SshServerService                     posting ClientActionEvent: ClientActionEvent{storage=FS, protocol=SFTP, timestamp=Sat Oct 05 21:40:51 GMT+02:00 2024, clientIp='192.168.1.122', clientAction=UPLOAD, path='/storage/emulated/0/Download/20240625_174813.jpg.filepart'}
org.primftpd.filesystem.MediaScannerClient                 media scanning started for file '/storage/emulated/0/Download/20240625_174813.jpg.filepart'
org.primftpd.services.SshServerService                     posting ClientActionEvent: ClientActionEvent{storage=FS, protocol=SFTP, timestamp=Sat Oct 05 21:40:51 GMT+02:00 2024, clientIp='192.168.1.122', clientAction=RENAME, path='/storage/emulated/0/Download/20240625_174813.jpg.filepart'}
org.primftpd.filesystem.MediaScannerClient                 media scanning started for file '/storage/emulated/0/Download/20240625_174813.jpg.filepart'
org.primftpd.filesystem.MediaScannerClient                 media scanning started for file '/storage/emulated/0/Download/20240625_174813.jpg'
org.primftpd.filesystem.MediaScannerClient                 media scanning completed for file '/storage/emulated/0/Download/20240625_174813.jpg.filepart'
org.primftpd.filesystem.MediaScannerClient                 media scanning completed for file '/storage/emulated/0/Download/20240625_174813.jpg.filepart'
org.primftpd.services.SshServerService                     posting ClientActionEvent: ClientActionEvent{storage=FS, protocol=SFTP, timestamp=Sat Oct 05 21:40:51 GMT+02:00 2024, clientIp='192.168.1.122', clientAction=LIST_DIR, path='/storage/emulated/0/Download'}
org.primftpd.filesystem.MediaScannerClient                 media scanning completed for file '/storage/emulated/0/Download/20240625_174813.jpg'
org.primftpd.services.SshServerService                     posting ClientActionEvent: ClientActionEvent{storage=FS, protocol=SFTP, timestamp=Sat Oct 05 21:41:12 GMT+02:00 2024, clientIp='192.168.1.122', clientAction=RENAME, path='/storage/emulated/0/Download/20240625_174813.jpg'}
org.primftpd.filesystem.MediaScannerClient                 media scanning started for file '/storage/emulated/0/Download/20240625_174813.jpg'
org.primftpd.filesystem.MediaScannerClient                 media scanning started for file '/storage/emulated/0/Documents/20240625_174813.jpg'
org.primftpd.services.SshServerService                     posting ClientActionEvent: ClientActionEvent{storage=FS, protocol=SFTP, timestamp=Sat Oct 05 21:41:12 GMT+02:00 2024, clientIp='192.168.1.122', clientAction=LIST_DIR, path='/storage/emulated/0/Download'}
org.primftpd.filesystem.MediaScannerClient                 media scanning completed for file '/storage/emulated/0/Download/20240625_174813.jpg'
org.primftpd.filesystem.MediaScannerClient                 media scanning completed for file '/storage/emulated/0/Documents/20240625_174813.jpg'
org.primftpd.services.SshServerService                     posting ClientActionEvent: ClientActionEvent{storage=FS, protocol=SFTP, timestamp=Sat Oct 05 21:41:18 GMT+02:00 2024, clientIp='192.168.1.122', clientAction=LIST_DIR, path='/storage/emulated/0/Documents'}
org.primftpd.services.SshServerService                     posting ClientActionEvent: ClientActionEvent{storage=FS, protocol=SFTP, timestamp=Sat Oct 05 21:41:23 GMT+02:00 2024, clientIp='192.168.1.122', clientAction=DELETE, path='/storage/emulated/0/Documents/20240625_174813.jpg'}
org.primftpd.filesystem.MediaScannerClient                 media scanning started for file '/storage/emulated/0/Documents/20240625_174813.jpg'
org.primftpd.services.SshServerService                     posting ClientActionEvent: ClientActionEvent{storage=FS, protocol=SFTP, timestamp=Sat Oct 05 21:41:23 GMT+02:00 2024, clientIp='192.168.1.122', clientAction=LIST_DIR, path='/storage/emulated/0/Documents'}
org.primftpd.filesystem.MediaScannerClient                 media scanning completed for file '/storage/emulated/0/Documents/20240625_174813.jpg'

@wolpi
Copy link
Owner

wolpi commented Oct 5, 2024

Interesting.

I never thought the "caching" of attributes would cause problems. It was done this way to avoid duplicated code.

The pftpdService was introduced somewhat later. With a fresh look on the code it makes sense to remove the other things and get it from pftpdService as it is there now.

@lmagyar
Copy link
Contributor Author

lmagyar commented Oct 5, 2024

This megarefactor is ready to review (see only the last commit).

I've updated the first comment:

  • updated the "header"
  • added 4. MediaScanner and it's logs

I've checked, after my 2 open issues merged, my forked and the original source is identical (except some local changes for test and build).

I will work on my draft "RW roSAF" PR later (I hope before this year's end).

@lmagyar lmagyar marked this pull request as ready for review October 5, 2024 20:05
@wolpi
Copy link
Owner

wolpi commented Oct 6, 2024

This PR still shows changes to StorageManagerUtil, even though #360 has been merged.
Is that correct?

I will do some minor changes after #360, update this PR, please.

All in all i think this refactoring makes sense and improves the code 👍

@lmagyar lmagyar force-pushed the pr-refactor-filesystem branch from 56fc5f2 to 493d45e Compare October 6, 2024 10:16
@lmagyar
Copy link
Contributor Author

lmagyar commented Oct 6, 2024

Yes, you are right, rebased it.

If you add some conflicting changes after #360, I will rebase again as needed.

wolpi added a commit that referenced this pull request Oct 6, 2024
@wolpi wolpi merged commit f0a38c6 into wolpi:master Oct 6, 2024
1 check passed
@wolpi
Copy link
Owner

wolpi commented Oct 6, 2024

here we go

@wolpi wolpi added this to the 7.3 milestone Oct 6, 2024
lmagyar pushed a commit to lmagyar/prim-ftpd that referenced this pull request Oct 6, 2024
lmagyar pushed a commit to lmagyar/prim-ftpd that referenced this pull request Oct 6, 2024
@lmagyar lmagyar deleted the pr-refactor-filesystem branch October 6, 2024 11:39
wolpi added a commit that referenced this pull request Oct 6, 2024
lmagyar pushed a commit to lmagyar/prim-ftpd that referenced this pull request Oct 6, 2024
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