adafruit / circuitpython

CircuitPython - a Python implementation for teaching coding with microcontrollers
https://circuitpython.org
Other
4.04k stars 1.19k forks source link

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

Open bill88t opened 6 months ago

bill88t commented 6 months ago

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

dhalbert commented 6 months ago

/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 commented 6 months ago

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 commented 6 months ago

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.

RetiredWizard commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 3 months ago

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']
bill88t commented 3 months ago

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:

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

bill88t commented 3 months ago

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.