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

Fix new file upload on SAF and VirtualFS with SSHFS #346

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

lmagyar
Copy link
Contributor

@lmagyar lmagyar commented May 26, 2024

SSHFS after opening a new, non-existent file for upload, assumes it is created and calls a STAT on it (see). And that STAT fails on SAF and virtual FS, because the file doesn't created, doesn't exists.

UPDATE: And SSHFS even calls FSTAT on this open file, so the created file's properties have to be updated on the already created object.

It is created on the "plain old" file-system, and that works with SSHFS. So I've added it for SAF and virtual. I'm sure it is not fast, but at least works. Tested on real phone: Android 9.0, Samsung A8.

I didn't change the root FS implementation, because I can't test that, my phone is not rooted.

@lmagyar lmagyar changed the title Fix new file upload on SAF with SSHFS Fix new file upload on SAF and VirtualFS with SSHFS May 27, 2024
@wolpi
Copy link
Owner

wolpi commented Jun 2, 2024

Interesting to see, I want to do some tests on that before merging

@lmagyar
Copy link
Contributor Author

lmagyar commented Jun 2, 2024

Yes, yes, test everything, these are "works on my phone" modifications. :) And my first ever Android/java modifications.

The concept is that in case of real files, when they are created, the next call to isSomething() returns the changed fact/reality, but in case of SAF this "background" communication between the methods doesn't happen, because nearly all these information are "cached" in the objects, so we have to update the facts inside the objects. It is against the more or less "immutable" state of these classes, it smells to me a bit, but couldnt come up with a better idea.

@wolpi
Copy link
Owner

wolpi commented Jun 9, 2024

For me creation of files, even in SAF below virtual, works. E.g. I can create a file virtual SAF and switch to virtual FS and it is present.

What was your issue you want to solve with this?

@lmagyar
Copy link
Contributor Author

lmagyar commented Jun 9, 2024

This is for SSHFS specifically, because SSHFS after opens the file (for creation), it calls a STAT (not FSTAT) on the filename immediately, and that fails on SAF and Virtual, because these (not like plain-old FS), do not create the real file, and the STAT tries to access the nonexistent file and fails.

It worked for SSHFS in case of plain-old, because the file was really created, but SAF and virtual did nothing.

It is not a problem for eg. WinSCP, that opens, writes, closes the file, simple, works, but SSHFS makes more administrative steps (also the main reason it is slower)


The other part of the modifications is also for SSHFS specifically, because later it calls an FSTAT also, and the existing handle's, ie. the SAF and Virtual file's "cached" info about the real file also has to be updated.

In case of plain-old it is not a problem, because there are mush less "cached" info in the FsFile class, most of it comes from the file itself.

@lmagyar lmagyar force-pushed the pr-fix-new-file-upload-with-sshfs branch from f1cd35b to 943433d Compare June 9, 2024 16:22
@lmagyar
Copy link
Contributor Author

lmagyar commented Jun 9, 2024

Rebased it to resolve conflicts.

@wolpi
Copy link
Owner

wolpi commented Jun 23, 2024

Ok, I see.

Than I would like to have code like this:

  • no change to SafFile
  • The createNewFile() method should instead be placed in SafSshFile
  • a comment be placed at this method, that it is required especially for one specific client: sshfs
  • similar comment on create() in VirtualSshFile

Would you change it like that?
Otherwise I can merge and change it myself.

@lmagyar
Copy link
Contributor Author

lmagyar commented Jun 23, 2024

I've tried to move it "down" to SafSshFile now, but the createNewFile() method uses writable, documentFile, parentDocumentFile properties, and all these are private, so SafSshFile can't access them. So it seems to be a better place to be is SafFile. This is similar to FsSshFile's create(), that calls file.createNewFile(), though that is part of the OS, we have to put our SAF equivalent of File.createNewFile() somewhere, and SafFile seems to be a better place.

I've added the comment.

But please feel free to modify anything as you see it a better solution, it's your project, your style, your concept, anything. :)

@wolpi wolpi added this to the 7.2 milestone Jun 26, 2024
@wolpi wolpi merged commit c282b4f into wolpi:master Jun 26, 2024
1 check passed
@wolpi
Copy link
Owner

wolpi commented Jun 26, 2024

ok, for now it is fine 😄

@lmagyar lmagyar deleted the pr-fix-new-file-upload-with-sshfs branch June 27, 2024 10:12
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