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

Stage to master: readonly in dav_open #356

Merged
merged 6 commits into from
Apr 24, 2017
Merged

Stage to master: readonly in dav_open #356

merged 6 commits into from
Apr 24, 2017

Conversation

jerryblakley
Copy link
Contributor

No description provided.

Copy link
Contributor

@kf6nux kf6nux left a comment

Choose a reason for hiding this comment

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

LGTM. I'd like to better understand the change to readdirectory when we chat Friday.

@@ -68,7 +68,7 @@ static int readdirectory(char *dirname, int *dirsread, int *filesread, int *erro
else {
int bytes_read;
char rbuf [1025];
fd = open(fn, O_RDWR);
fd = open(fn, O_RDONLY);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know this code well enough to understand what the change here is for, or why we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a test file. I was considering adding O_RDWR to the list of 'write' flags to open to cause it to fail if in readonly mode, then realized that without thinking I used O_RDWR to open a file which was merely for reading. So I changed to test to be O_RDONLY so it could be used to test read success while testing readonly mode. Then I decided not to add O_RDWR to the 'write' flags. Still, the most correct way to open a file which will merely be read from is to use O_RDONLY.
This test just recurses through the filesystem, enters all directories, and reads all files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! 😄

@kf6nux
Copy link
Contributor

kf6nux commented Apr 24, 2017

LGTM 👍

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