Skip to content

src: add GetValidatedFd() to node_file - #49903

Closed
pluris wants to merge 1 commit into
nodejs:mainfrom
pluris:feat/get_validated_fd
Closed

src: add GetValidatedFd() to node_file#49903
pluris wants to merge 1 commit into
nodejs:mainfrom
pluris:feat/get_validated_fd

Conversation

@pluris

Copy link
Copy Markdown
Contributor

A function to validate file descriptor in c++ land has been added.
This is a C++ implementation of getValidatedFd() in js.

Refs: #49898, #49880

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 27, 2023
@plurispluris changed the title src: add GetValidatedFd() in node_filesrc: add GetValidatedFd() to node_fileSep 27, 2023
@pluris
plurisforce-pushed the feat/get_validated_fd branch from ed908e6 to b35894eCompareSeptember 27, 2023 17:11
@mscdex

Copy link
Copy Markdown
Contributor

I think we should only be adding code that is currently used somewhere. I get you have other PRs that use this function, but it would be better to just wait for one of them to land first and then rebase the other.

@pluris

Copy link
Copy Markdown
ContributorAuthor

@mscdex
Thank you for your comment. I will wait for the PR in which this function is used to be landed. I will close this PR.

@plurispluris closed this Sep 27, 2023
@pluris
pluris deleted the feat/get_validated_fd branch November 8, 2023 02:24
@anonrig

Copy link
Copy Markdown
Member

Hey @pluris, Are you working on this? I'd like to continue on the work on node:fs

@pluris

Copy link
Copy Markdown
ContributorAuthor

@anonrig No, I'm not working on it. I think it would be better for you to do it.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@pluris@mscdex@anonrig@nodejs-github-bot