Skip to content

storage.mount() irregular behaviour when mounting to a non-top-level directory #9045

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

Closed
bill88t opened this issue Mar 13, 2024 · 9 comments · Fixed by #10263
Closed

storage.mount() irregular behaviour when mounting to a non-top-level directory #9045

bill88t opened this issue Mar 13, 2024 · 9 comments · Fixed by #10263
Assignees
Milestone

Comments

@bill88t
Copy link

bill88t commented Mar 13, 2024

CircuitPython version

Adafruit CircuitPython 9.0.0-rc.0-8-g4f0da18204 on 2024-03-12; Seeeduino Wio Terminal with samd51p19

Code/REPL

>>> import os
>>> os.listdir("/")
['.fseventsd', '.metadata_never_index', '.Trashes', '.Trash-1000', 'Beryllium', 'code.py', 'boot_out.txt', 'repl.py', 'boot.py', 'settings.toml', 'LOST.DIR', 'Music', 'Podcasts', 'Android', 'Ringtones', 'Alarms', 'Notifications', 'Pictures', 'Movies', 'Download', 'DCIM', 'Documents', 'Audiobooks', 'Recordings', 'lib']
>>> import board, sdcardio, busio, storage
>>> spi = busio.SPI(board.SD_SCK, MOSI=board.SD_MOSI, MISO=board.SD_MISO)
>>> sdcard = sdcardio.SDCard(spi, board.SD_CS)
>>> vfs = storage.VfsFat(sdcard)
>>> storage.mount(vfs, "/Beryllium/mnt")
>>> os.listdir("/")
['Beryllium/mnt', '.fseventsd', '.metadata_never_index', '.Trashes', '.Trash-1000', 'Beryllium', 'code.py', 'boot_out.txt', 'repl.py', 'boot.py', 'settings.toml', 'LOST.DIR', 'Music', 'Podcasts', 'Android', 'Ringtones', 'Alarms', 'Notifications', 'Pictures', 'Movies', 'Download', 'DCIM', 'Documents', 'Audiobooks', 'Recordings', 'lib']
>>>

Behavior

'Beryllium/mnt'????????????

Description

The mnt directory exists and is empty.
The mount is in fact not attached there, but to the literal "Beryllium/mnt".

Additional information

No response

@bill88t bill88t added the bug label Mar 13, 2024
@dhalbert
Copy link
Collaborator

/Beryllium/mnt should be the mount point. Did that subdirectory exist beforehand?

Is the SD card empty? If not after the storage.mount(), is /Beryllium/mnt/ empty? It should show what is on the SD card.

@bill88t
Copy link
Author

bill88t commented Mar 13, 2024

Did that subdirectory exist beforehand?

Yes.

Is the SD card empty?

Has a single empty file inside.

I was incorrect in saying that /Beryllium/mnt is empty, it has a hidden file.

>>> os.listdir("/Beryllium/mnt")
['test_file.txt']
>>> os.listdir("//Beryllium/mnt")
['.gitkeep']
>>> os.chdir("/Beryllium")
>>> os.listdir("mnt")
['.gitkeep']
>>> 

@bill88t
Copy link
Author

bill88t commented Mar 13, 2024

mount() is clearly trying to mount from working directory, that is really risky when we are talking about relative paths, and leads to issues like this.
I'm sure if I did anything like "../../../../Beryllium/mnt", it would explode.
Checking with logic is also really slow, and buggy.
The simpliest and safest way is:

  1. store getcwd().
  2. chdir() to target, then to ... If this succeeds, this means target exists, is a folder and we are currently above it.
  3. Attempt to mount, the target name being what is after rfind("/").
  4. Go back to the starting directory.

If step 2 fails, it should just go back to starting and raise the error.

@tannewt tannewt added this to the 9.x.x milestone Mar 13, 2024
@RetiredWizard
Copy link

RetiredWizard commented Mar 19, 2024

This feels connected to #8409, the mount point "link" in the root folder does not show up in an os.listdir("./') from the root folder. Also, if from the root folder you enter os.listdir('./Beryllium/mnt') the SD card contents are not displayed, same for os.chdir('/Beryllium') followed by os.listdir('mnt').

Edit: At one point I apparently thought that CircuitPython couldn't mount SD cards to sub folders or perhaps I just blocked doing so in my code because I ran into something like this..

@RetiredWizard
Copy link

Another symptom is that os.rename('./sd','/newsd') effectively separates the physical mount point from the mounted file system. The contents of the sd card can then been seen by os.listdir('/sd') and the original placeholder file can be seen by os.listdir('/newsd'). Doing this also seems to hide the /sd folder (and SD card contents) from web workflow.

@bill88t
Copy link
Author

bill88t commented Mar 20, 2024

Sorry, I unraveled an even bigger bug.

Adafruit CircuitPython 9.0.0-dirty on 2024-03-19; Seeeduino Wio Terminal with samd51p19
>>> import board, sdcardio, busio, storage
>>> spi = busio.SPI(board.SD_SCK, MOSI=board.SD_MOSI, MISO=board.SD_MISO)
>>> sdcard = sdcardio.SDCard(spi, board.SD_CS)
>>> vfs = storage.VfsFat(sdcard)
>>> import os
>>> os.listdir()
['.fseventsd', '.metadata_never_index', '.Trashes', '.Trash-1000', 'Beryllium', 'code.py', 'boot_out.txt', 'repl.py', 'boot.py', 'settings.toml', 'LOST.DIR', 'Music', 'Podcasts', 'Android', 'Ringtones', 'Alarms', 'Notifications', 'Pictures', 'Movies', 'Download', 'DCIM', 'Documents', 'Audiobooks', 'Recordings', 'lib']
>>> os.chdir("Beryllium")
>>> os.listdir()
['bin', 'dev', 'etc', 'home', 'lib', 'lost+found', 'mnt', 'proc', 'root', 'run', 'sbin', 'srv', 'sys', 'tmp', 'usr', 'var', 'boot']
>>> storage.mount(vfs, "mnt")
>>> os.listdir("mnt")
['.gitkeep'] # not what is on the sd
>>> os.chdir("/")
>>> os.listdir()
['nt', '.fseventsd', '.metadata_never_index', '.Trashes', '.Trash-1000', 'Beryllium', 'code.py', 'boot_out.txt', 'repl.py', 'boot.py', 'settings.toml', 'LOST.DIR', 'Music', 'Podcasts', 'Android', 'Ringtones', 'Alarms', 'Notifications', 'Pictures', 'Movies', 'Download', 'DCIM', 'Documents', 'Audiobooks', 'Recordings', 'lib']
>>>

@jepler
Copy link

jepler commented May 29, 2024

This original behavior seems to exist in micropython 1.22

MicroPython v1.23.0-preview.352.g50b43fec1 on 2024-05-29; linux [GCC 12.2.0] version
Use Ctrl-D to exit, Ctrl-E for paste mode
>>> import os, vfs
>>> vfs.mount(os.VfsPosix("/tmp"), "/sub/dir")
>>> os.listdir("/")
['sub/dir', 'home', 'nix', 'lib', 'initrd.img', 'etc', 'run', 'libx32', 'sbin', 'usr', 'bin', 'vmlinuz', 'vmlinuz.old', 'bob', 'var', 'boot', 'dev', 'core', 'lib64', 'initrd.img.old', 'srv', 'opt', 'media', 'lib32', 'sys', 'tmp', 'mnt', 'root', 'proc']

@jepler jepler modified the milestones: 9.x.x, Long term May 29, 2024
@bill88t
Copy link
Author

bill88t commented May 29, 2024

Sat down and made a diagram for parsing paths.
I don't wanna imagine doing this in C.

We would need to take any relative paths and convert them to absolute.
That means also handling /path/to/some/weird/../weirder/./place and handling invalid fat32 characters in this parse.
Then from absolute paths we would need to look into the mount table (my brain stores it as a dict {"/absolute/path": vfsobject}), and then do the following mad procedure:

  • split the abs path into segments "/path/to/somewhere" -> ["path", "to", "somewhere"].
  • Store "/" as our "current location" and set the root vfs object as the "working vfs object".
  • Then for every segment:
    • Check if "current location" + next segment ("/" + "path"), exists in the mount table.
    • If it exists, we set that as the "working vfs object" and add the segment into our "current location".
      (Ignoring as to if the folder / file exists in current working vfs object.)
    • If it doesn't, we check the working vfs object's listdir, truncating the path before it.
      (for example, if we had a mount at "/path" and we are checking as to if the "to" segment exists, we will check if "to" exists in the listdir, not "/path/to")
    • If it exists in the vfs object we check if we have more segments to go.
    • If we do, we have to check if the last segment we just checked exists is a file or a folder.

Then this absolute chonker of an C function would have to be used in every os function.

@bill88t
Copy link
Author

bill88t commented May 30, 2024

The abs-path function would have to be seperate in order to reuse in areas like the mount function.

Also, it may be worth consider fixing this externally into perhaps a mount_manager module.
It's a lot of edits to the core.

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

Successfully merging a pull request may close this issue.

5 participants