Skip to content

Allow truncation to work within VFileFromMemory #3455

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CasualPokePlayer
Copy link
Contributor

@CasualPokePlayer CasualPokePlayer commented Apr 7, 2025

Truncation here is limited to buffer size (allowing for shrinking but not growing the VFile size)

Truncation here is limited to buffer size (allowing for shrinking but not growing the buffer size)
Copy link
Member

@endrift endrift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious what this is useful for.

struct VFileMem* vfm = (struct VFileMem*) vf;

if (size > vfm->bufferSize) {
size = vfm->bufferSize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding the size to match the buffer size after shrinking should not be allowed because the expectation of an expanded buffer is that the remainder is zeroed, which will not be the case for const mem. You'd also need a separate implementation for non-const mem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zeroed as in there would just be a memset within the existing buffer within the "expanded" space?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume f50a383 should cover this. For VFileFromConstMemory, I've backed out those changes entirely (I suppose anyways under strict ftruncate semantics you can't truncate at all when the file is read-only).

Not sure entirely if clamping the size done is correct here. Under strict ftruncate semantics, expanding past the maximum file size is just an error (EFBIG or EINVAL or EPERM) rather than a clamp.

@CasualPokePlayer
Copy link
Contributor Author

I'm curious what this is useful for.

The intent of this is mainly for VFileFromMemory. I'm trying to work on removing internal header usage within GSE, and one of the usages is just trying to look into savedata size. With the current code, truncation would do nothing here, so the savedata vf would just be the maximum savedata size, so the only way to know the actual savedata size is internal functions. By having truncation do something, the savedata vf can initially be set to 0, then later mGBA functions can grow the buffer, at least until it hits the maximum size.

Although in hindsight, this also doesn't really work well unless write is also able to "expand" the size until the buffer size (for RTC writes), so that'd need to be updated too.

@CasualPokePlayer CasualPokePlayer changed the title Allow truncation to work within VFileFromMemory/VFileFromConstMemory Allow truncation to work within VFileFromMemory Apr 7, 2025
Comment on lines +240 to +245
if (size + vfm->offset > vfm->size) {
vfm->size = size + vfm->offset;
if (vfm->size > vfm->bufferSize) {
vfm->size = vfm->bufferSize;
size = vfm->size - vfm->offset;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this wound up in the wrong PR, or at least the wrong commit. Could you fixup the history at least?

Copy link
Contributor Author

@CasualPokePlayer CasualPokePlayer Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's have VFileFromMemory write "expand" size until the buffer size. Is this better as separate commits or separate PRs? (or squashing all the history into one commit?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same PR is fine, but one of the commits is basically "back out half of the last commit, and also this", so instead just splitting out the first half and then doing a fixup in to the previous commit would be preferable.

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