Rockbox

Tasklist

FS#12500 - /./ is invalid when not using dircache, so HOME_DIR breaks things

Attached to Project: Rockbox
Opened by Boris Gjenero (dreamlayers) - Saturday, 31 December 2011, 17:57 GMT
Last edited by Boris Gjenero (dreamlayers) - Friday, 20 January 2012, 00:43 GMT
Task Type Bugs
Category Operating System/Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

When Rockbox is directly accessing the disk, paths starting with /./ are not valid. The FAT32 root directory does not have the . and .. entries found in other directories. It is possible to use . and .. in other directories, even for getting to the root directory from a 1st level directory.

This causes HOME_DIR, introduced in r31430, to break some things. It seems dircache can deal with it. However, file creation always uses opendir_uncached(), which fails. Here are two examples:

When recording to the default recording directory, the path is "/./filename.ext". With dircache disabled, the recording screen can't be entered, because "/./" can't be accessed. With dircache enabled, recording starts, but saving fails as if the disk is full.

When saving a playlist to the playlist catalog, the path is "/./Playlists/name.m3u8". With dircache disabled, I can't even enter a filename because the path is inaccessible. With dircache enabled, I can edit the filename and remove the /. at the start to make it work. If I don't remove the /. then there is no error message but the playlist fails to save. This lack of error message is another bug.

I tested this on my 5G 30GB iPod running r31467.
This task depends upon

Closed by  Boris Gjenero (dreamlayers)
Friday, 20 January 2012, 00:43 GMT
Reason for closing:  Fixed
Additional comments about closing:  HOME_DIR issue fixed in 6e11289. There doesn't seem to be much interest in adding support for "." and "..", and I'm not even convinced that it's a good thing to add.
Comment by Thomas Martitz (kugel.) - Sunday, 01 January 2012, 17:43 GMT
That's an error in our code isnt it? My linux has . and .. in /.

If so, we should probably fix it. Otherwise we can undo that particular change and go back to HOME_DIR == "/" which is not entirely right IIUC (it can create paths starting with //). But that was before and worked, although amiconn noted that cygwin interprets // paths as network locations.
Comment by Boris Gjenero (dreamlayers) - Sunday, 01 January 2012, 19:34 GMT
Windows also accepts \. even when it's a FAT32 partition, where . does not not actually exist in the root directory. I guess recognizing /. is a good thing. There's no other need for it, but consistency is good. It's a trivial change. This patch causes opendir_uncached() to recognize . as the current directory, without actually needing to search through the directory to find the . entry. It fixes the problems I described.

I don't like how /. is visible to the user, and how it uses up screen and memory space. Because of this concern, I was thinking about making HOME_DIR blank. The problem there is that opendir(HOME_DIR) won't work. This would currently only be a problem in create_numbered_filename(). It doesn't seem like there's any simple and elegant solution for removing the prefix.

Edit: With opendir_uncached_recognize_dot.patch, /./<volumename>/ is still broken, because it goes to the root file system instead of the volume. In strip_volume(), repeated / at the start are handled, but /./ isn't. Since strip_volume() is also used in open_internal(), it would have to be changed to handle this. I guess it doesn't matter though, because "/./<volumename>/" wouldn't happen. BTW Also .. is broken in connection with multivolume.

It may be best to use "/" for HOME_DIR. Rockbox can already handle //, and I don't see why Windows would ever be given paths starting with //.
Comment by Thomas Martitz (kugel.) - Monday, 02 January 2012, 15:16 GMT
RaaA can possibly pass "//"-paths to the OS.
Comment by Jean-Louis Biasini (JeanLouisBiasini) - Monday, 02 January 2012, 19:34 GMT
wow this is the reason why I can't get recording and playlist to work on my fuze+. Desactiving cache is useful on fuze+ because of http://www.rockbox.org/tracker/task/12447?project=1&opened=6464

the patch solve the problem.
Comment by Michael Sevakis (MikeS) - Tuesday, 03 January 2012, 07:34 GMT
Why must it be static in nature? Couldn't "/" be used always and converted where necessary before accessing the hosts file system?
Comment by Boris Gjenero (dreamlayers) - Tuesday, 03 January 2012, 17:24 GMT
RaaA should do substitution for HOME_DIR. In both Unix-like environments and in Windows, writes to the root directory are undesirable. Stuff that belongs in HOME_DIR should go somewhere under the current user's home directory. Simply substituting HOME_DIR with the current user's home directory is ok, though respecting the desktop environment's documents and music folders is better. The function handling this could remove any extra unwanted prefix if necessary.
Comment by Thomas Martitz (kugel.) - Sunday, 08 January 2012, 18:02 GMT
If it's too difficult to make "." and ".." work for the root I suggest changing HOME_DIR back to /, but I think create_*_filename in firmware/general.c need to be fixed to handle that (the comments say it doesn't handle trailing slashes).
Comment by Frank Gevaerts (fg) - Sunday, 15 January 2012, 13:00 GMT
create_*_filename() seems to work just fine with HOME_DIR set to "/". If no other proposal is committed during the next two days, I'll commit that.
Comment by Boris Gjenero (dreamlayers) - Sunday, 15 January 2012, 17:07 GMT
Thanks for deciding to make the "/" change. It doensn't make sense to leave recording to the default directory broken to avoid problems which only happen in a configuration which doesn't currently exist. The "/" change is correct even if there's the intention of eventually doing something better.

Passing "/" to create_*_filename() works, though it creates filenames starting with "//". Recording replaces "/" with "" to avoid this. BTW This makes me think the following change in r31430 in create_numbered_filename() is wrong:
- dir = opendir(pathlen ? buffer : "/");
+ dir = opendir(pathlen ? buffer : HOME_DIR);
The code was probably there because passing "" as path is the proper way to create a filename in the root directory. Also, in r31430 the caller passes HOME_DIR, so either the defaulting to HOME_DIR or the passing of HOME_DIR is redundant.

It would be easy to change create_*_filename() to not create paths with "//" in the result. However, not much is gained as long as the preprocessor elsewhere creates paths starting with "//".

Would it be okay to have HOME_DIR end with a slash instead of appended things starting with a slash? This will certainly work if something is being appended. Also, our directory functions should work because strtok_r() will cause trailing slashes to be ignored. I just don't know if any other operating system requires that paths passed to directory functions not have a trailing slash.

I'm attaching code for more fully supporting "." and "..". Remaining issues and limitations are described below. Note that it's possible to test HAVE_MULTIVOLUME by shrinking the main FAT32 partition and creating another partition.

In opendir_uncached(), the parent directory passed to fat_opendir() is wrong after "..". It only seems to cause a problem in an unlikely rmdir() race condition. For more info, read the FIXME comment.

The _dircache_get_entry_volume() function nicely matches the API, but it does an extra dircache traverse from the entry toward the root. I think it's ok because the traverse is cheap. Otherwise, dircache_get_entry() would have to be changed to return the volume.

I did not fix strip_volume(). It's now only used in generate_bookmark_file_name().

In playlist_save(), strncmp() is used to check if the current playlist is being overwritten. That will fail if the playlist is being overwritten using a different path. I did not search for similar path comparisons elsewhere.

The last two could be solved by a function which creates a canonical file name, similar to POSIX realpath(). Adding more disk access seems like a bad idea, so it should probably be done via string manipulation. This could also replace the dircache changes. Accepting "/nonexistent/path/../../real/path/" is okay, because it would fail elsewhere. If succeeding with such paths is okay, then such a function could even replace the opendir_uncached() changes.

Full support for . and .. would make it possible to simplify format_track_path() in playlist code.

Loading...