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

Fix fcntl #105

Open
wants to merge 2 commits into
base: vita
Choose a base branch
from
Open

Fix fcntl #105

wants to merge 2 commits into from

Conversation

isage
Copy link
Contributor

@isage isage commented Dec 3, 2024

No description provided.

Comment on lines 127 to 129
fdmap->flags = arg;
__vita_fd_drop(fdmap);
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

a patch seems good for me.
just one thing, F_SETFL wouldn't affect actual file flags & system behaviors.

do we really needs F_SETFL? how about only implement F_GETFL and F_SETFL keep as ENOTSUP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need F_SETFL, even if it does nothing for now.
fdopen calls it, and even though it doesn't check it's result, we'll have falsely set errno if we return ENOTSUP

Copy link
Contributor

@d3m3vilurr d3m3vilurr Dec 6, 2024

Choose a reason for hiding this comment

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

hm.... i see....

at first, i thought it was only allowed if an orig flag included all input flags.

if ((fdmap->flags & arg) == arg) {
  __vita_fd_drop(fdmap);
  return 0;
}

btw, fdopen_r implementation automatically appends O_APPEND into new flag.

so it should be...

int new_flag = arg;
// if original flags doesn't have O_APPEND and input has O_APPEND,
// it can be possible to be added by fdopen_r logic.
// we drop this flag from input to restore original one.
if (!(fdmap->flags & O_APPEND) && (arg & O_APPEND)) {
  arg &= ~O_APPEND;
}
// our fcntl cannot update actual flags of system file descriptor.
// that this reason, we only check original flags contains input flags
if ((fdmap->flags & arg) == arg) {
  __vita_fd_drop(fdmap);
  return 0
}

i'm not sure that this is correct or not.
maybe just checking same flag value (fdmap->flags == arg) is batter.

ps. imo, we might have to reimplement fdopen too...
for example, both implementation (pr version and my suggestions) have problem.
F_SETFL doesn't affect actual flags of file description of the vita system, it wouldn't work programmer's expectation in file IO

so, more good way is use fdopen of the sdk version or newlib's fdopen always returns an error when fd is file descriptor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fdopen not only changes flags, but also sets them on internal file descriptor, see
https://github.com/vitasdk/newlib/blob/vita/newlib/libc/stdio/fdopen.c#L86
but those are only used for fwrite, not for write.
I've pushed another fix that

  1. updates those flags in fcntl (so fwrite works as intended)
  2. write now handles O_APPEND flags by seeking to the end of file.

This simple test

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>

#if defined(__vita__)
#include <psp2/kernel/clib.h>
#define printf sceClibPrintf
#define FPATH "ux0:/data/fd_share.txt"
#else
#define FPATH "fd_share.txt"
#endif

int main() {
    int fd = open(FPATH, O_RDWR | O_CREAT, 0644);

    if (fd < 0)
    {
        printf("errno: 0x%08x\n", errno);
        return 0;
    }

    // append to empty file
    write(fd, "aaaaaaaaaa", 10);

    off_t cur = lseek(fd, 1, SEEK_SET);
    printf("offset after being set to 1: %ld\n", (long)cur);

    // append
    write(fd, "bbbbbbbb", 8);

    cur = lseek(fd, 0, SEEK_CUR);
    printf("offset after appending bbbbbbbb: %ld\n", (long)cur);

    cur = lseek(fd, 2, SEEK_SET);
    printf("offset after being set to 2: %ld\n", (long)cur);

    // now toggle "append mode"
    int open_flag = fcntl(fd, F_GETFL);
    if (fcntl(fd, F_SETFL, open_flag | O_APPEND) == -1) {
        printf("failed to set flag\n");
        return 0;
    }

    cur = lseek(fd, 0, SEEK_CUR);
    printf("offset after setting O_APPEND: %ld\n", (long)cur);

    // write without appending
    write(fd, "cccc", 4);

    cur = lseek(fd, 0, SEEK_CUR);
    printf("offset after writing cccc: %ld\n", (long)cur);


    close(fd);

    return 0;
}

produces same results on PC and vita now.

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