Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

unix,win: add uv_fs_access() #1511

Closed
wants to merge 3 commits into from
Closed

unix,win: add uv_fs_access() #1511

wants to merge 3 commits into from

Conversation

cjihrig
Copy link

@cjihrig cjihrig commented Oct 3, 2014

@saghul here is the uv_fs_access() PR you asked for. The Unix side seems to be working, but on the Windows side, GetFileAttributesW() is always returning INVALID_FILE_ATTRIBUTES. Suggestions are more than welcome.

@saghul
Copy link
Contributor

saghul commented Oct 3, 2014

Thanks Colin! I'll check it asap (I'm moving these days and my Windows gear is in a box somewhere...)

@@ -2102,6 +2114,29 @@ int uv_fs_sendfile(uv_loop_t* loop, uv_fs_t* req, uv_file fd_out,
}


int uv_fs_access(uv_loop_t* loop, uv_fs_t* req, const char* path, int flags,
uv_fs_cb cb) {
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: prefer the following:

int uv_fs_access(uv_loop_t* loop,
                 uv_fs_t* req,
                 const char* path,
                 int flags,
                 uv_fs_cb cb) {

Copy link
Author

Choose a reason for hiding this comment

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

I agree, but this seems to be the convention used throughout this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@saghul keep convention w/ the rest of the file, or use the usual indentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with the usual.

@cjihrig
Copy link
Author

cjihrig commented Oct 5, 2014

@saghul addressed the previous round of comments and got it working on Windows.

uv_fs_req_init(loop, req, UV_FS_ACCESS, cb);

err = fs__capture_path(loop, req, path, NULL, cb != NULL);
if (err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for braces.

@saghul
Copy link
Contributor

saghul commented Oct 6, 2014

Good work Chris! Looks solid. I'll merge once after running it for a spin on Windows. Did you look into what @piscisaureus said about GetFileAttributesW? At any rate, it can be improved at a later stage since it's an implementation detail.

@cjihrig
Copy link
Author

cjihrig commented Oct 6, 2014

@saghul removed the braces. I didn't look into opening and closing the file, as their seemed to be a potential issue if the file was locked by another process. Linking to the node PR for future reference.

@saghul
Copy link
Contributor

saghul commented Oct 13, 2014

Cheers, Colin! Landed in c18205a.

@saghul saghul closed this Oct 13, 2014
@cjihrig cjihrig deleted the access branch October 13, 2014 14:05
@cjihrig
Copy link
Author

cjihrig commented Oct 13, 2014

Awesome. Thanks for the help @saghul. @trevnorris I'm sure you'll get an email about this, but you asked me to let you know when this landed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants