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 not readable #11

Closed
Meziu opened this issue Feb 10, 2022 · 16 comments · Fixed by #12
Closed

File not readable #11

Meziu opened this issue Feb 10, 2022 · 16 comments · Fixed by #12

Comments

@Meziu
Copy link
Owner

Meziu commented Feb 10, 2022

I was testing path and fs for #6.

Turns out, reading files is broken: doing so will result in a failed memory allocation. Looking into it it seems an issue with fstat, since it returns a wrong st_size value (always 1644524307), which the Rust File will then try to allocate into a Vec. That'd be about 1.67 GB of memory, so the system just dies.

@ian-h-chamberlain @AzureMarker

@AzureMarker
Copy link

AzureMarker commented Feb 11, 2022

I remember doing some debugging in this area when the "is_dir" check was failing in file-explorer. I think the underlying cause was the two std from static linking situation we had at that time, but I'd double check that the data structures line up between libc and the 3DS/libctru's definition.

@Meziu
Copy link
Owner Author

Meziu commented Feb 11, 2022

Hmm, it looks to be an issue only present with romfs files… I’ll search around the libctru docs.

@Meziu
Copy link
Owner Author

Meziu commented Feb 11, 2022

Yes, the various FileSystems don’t have full support of the normal posix functions. It looks like having the Romfs struct for using it as a normal FS is useless, since most of the functions used by std::fs aren’t available for that (including writing, now that I think about it…). We should work with ctru::services::fs instead.

@ian-h-chamberlain
Copy link

Hmm, some of this is a bit surprising to me, since I have been working on a change for colored test output which uses the romfs to create TERMINFO files and feed them to the std test runner (using std APIs of course). It seems to work as expected for the most part, although I do recall hitting a system crash on the first attempt I made.

I wonder what's different there vs the testing you've been doing. Perhaps the file size is relevant? In my case the file being opened is only 1462 bytes.

Looking at the test code that's running, it's in this codepath and the only check before opening it is fs::metadata(&p).is_ok():

    fn _from_path(path: &Path) -> Result<TermInfo, Error> {
        let file = File::open(path).map_err(Error::IoError)?;
        let mut reader = BufReader::new(file);
        parse(&mut reader, false).map_err(Error::MalformedTerminfo)
    }

@Meziu
Copy link
Owner Author

Meziu commented Feb 11, 2022

Welp, while it is great to have support for RomFS in the std::fs, it is surely prone to errors in any case: not being able to write, having to manually load the module, many features not working. It is interesting to have, but maybe we should focus on using the system specific functionality via ctru::fs, including RomFS.

@AzureMarker
Copy link

I'd like to take a look at this before we close out possibility of using std. The write issue shouldn't be a problem because it could just return an error when you try to open a file for writing (read only file systems already exist).

@Meziu
Copy link
Owner Author

Meziu commented Feb 11, 2022

I am looking into making a ctru::fs example. Having both would be cool, my example will include sdmc code too, it's just to test the module.

@Meziu
Copy link
Owner Author

Meziu commented Feb 11, 2022

Ok, looks like Romfs is just a big issue. Ironically, it doesn't work with FSUSER functions either. As someone from the Homebrew Discord said:

ARCHIVE_ROMFS gives you the raw romfs binary and the application is supposed to parse it.
Ctrulib does that, and makes it available through posix functions

This effectively means that (unless you are a god Homebrew programmer) Romfs ONLY works under std::fs, even though it has limited support. This is weird... I guess we should just test the whole std::fs module and see what we get...

@AzureMarker
Copy link

I wonder if we'll need to make libctru changes. At least it seems like they're still somewhat active, as there was a recent release.

@Meziu
Copy link
Owner Author

Meziu commented Feb 11, 2022

I wonder if we'll need to make libctru changes. At least it seems like they're still somewhat active, as there was a recent release.

I bumped ctru-sys, and that’ll be it most of the times. They usually only push breaking changes in major releases, though it depends.

@Meziu
Copy link
Owner Author

Meziu commented Feb 12, 2022

@ian-h-chamberlain could you make a working romfs example for ctru-rs, just so I can check my toolchain is alright.

@ian-h-chamberlain
Copy link

ian-h-chamberlain commented Feb 12, 2022

@ian-h-chamberlain could you make a working romfs example for ctru-rs, just so I can check my toolchain is alright.

Interesting... I started trying to extend the file-explorer example to read and display file contents and I found the same issue when using fs::read_to_string (fails to allocate)... However, it seems that BufReader avoids the problem since it doesn't allocate up front, and I am able to use it successfully. I have a couple more bugs to iron out and I'll push what I have. I think we can debug from there.

Edit: I think we might also need to reconsider the path scheme (or add OS-specific helpers), since I have noticed some weird behavior with .parent() and .pop().... and the romfs:/ seems a bit nonstandard for unix-like OSes.

Edit: seems like RedoxOS already uses a similar scheme (and has some support in sys::path), so maybe we can piggyback off of that: https://doc.redox-os.org/book/ch04-06-schemes.html

@Meziu
Copy link
Owner Author

Meziu commented Feb 12, 2022

Interesting... I started trying to extend the file-explorer example to read and display file contents and I found the same issue when using fs::read_to_string (fails to allocate)... However, it seems that BufReader avoids the problem since it doesn't allocate up front, and I am able to use it successfully.

Guessed so. It looks like a problem with “direct reading” functions that don’t have a buffer. We should find a way to get the filesize on romfs if fstat really doesn’t work.

@ian-h-chamberlain
Copy link

Ok, I did a little more digging, and it seems like stat called directly from 3ds-examples does what you would expect it to, so I suspect an FFI issue or similar here. Still trying to track it down, but it seems like we should be able to use those APIs since they are registered by libctru for the romfs "device", and supported in newlib.

@Meziu
Copy link
Owner Author

Meziu commented Feb 13, 2022

Ok, I did a little more digging, and it seems like stat called directly from 3ds-examples does what you would expect it to, so I suspect an FFI issue or similar here. Still trying to track it down, but it seems like we should be able to use those APIs since they are registered by libctru for the romfs "device", and supported in newlib.

std calls fstat though. Is that supported?

@ian-h-chamberlain
Copy link

ian-h-chamberlain commented Feb 13, 2022

Ok, I did a little more digging, and it seems like stat called directly from 3ds-examples does what you would expect it to, so I suspect an FFI issue or similar here. Still trying to track it down, but it seems like we should be able to use those APIs since they are registered by libctru for the romfs "device", and supported in newlib.

std calls fstat though. Is that supported?

Yeah, all the device-specific callbacks are registered here

I've made a little progress, I think there is an ABI mismatch somewhere on the stat type itself, where maybe gid_t is the wrong size?

// C:
{
    st_dev = 0,
    st_ino = 0,
    st_mode = 16676,
    st_nlink = 4,
    st_uid = 0,
    st_gid = 0,
    st_rdev = 0,
    st_size = 24,
    st_atim = {
        tv_sec = 1644690556,
        tv_nsec = 0
    },
    st_mtim = {
        tv_sec = 1644690556,
        tv_nsec = 0
    },
    st_ctim = {
        tv_sec = 1644690556,
        tv_nsec = 0
    },
    st_blksize = 512,
    st_blocks = 1,
    st_spare4 = {0, 0}
}

// Rust:
libc::unix::newlib::stat {
    st_dev: 0,
    st_ino: 0,
    st_mode: 16676,
    st_nlink: 4,
    st_uid: 0,
    st_gid: 0,
    st_rdev: 24,
    st_size: 1644690556,
    st_atime: 0,
    st_spare1: 1644690556,
    st_mtime: 0,
    st_spare2: 1644690556,
    st_ctime: 0,
    st_spare3: 512,
    st_blksize: 1,
    st_blocks: 0,
    st_spare4: [0, 0]
}

Edit: That was it! I'll post relevant PRs to libc + std shortly...

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 a pull request may close this issue.

3 participants