Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Getter Setters for Extended Attributes and Reparse points. #229
Add Getter Setters for Extended Attributes and Reparse points. #229
Changes from all commits
a7564fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this contribution @ambarve and I'm thankful to all the contributors of this project, it's been very helpful.
I think one aspect that could be improved in this function is that if you pass a handle to a file which does not have any extended attributes, then it returns an error, "get file EA failed with: The file or directory is corrupted and unreadable."
However, this should ideally be a normal situation that should be handled gracefully.
I noticed that the system call returns -1073741742 status code for both files and directories in this case.
Suggested change -
That said, I do not know if this is the best way to handle this scenario or if that status code will always be consistent.
Anyone with a more comprehensive understanding of the system internals have any better suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need the else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the else with break otherwise the for loop doesn't end and the code gets stuck in a loop.
Existing code is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should've been more clear. Keep the break just don't need the else. But we'd need something to retry the loop so we'd need a continue after
buf = make([]byte, bufLen)
, e.g. the below. This seems mostly preference, but in my jumbled brain it flows a bit betterThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, that does seem a bit more concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another useful test would be for setting and getting extended attributes for folders.
The main change required is the passing of an extra attribute - windows.FILE_FLAG_BACKUP_SEMANTICS for getting the file handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to make these "friendly" APIs and just take in ReparseDataBuffers directly and we encode inside?
e.g.
and if folks want to call encode/decode and pass around byte slices themselves we leave those methods exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC the problem here is that we currently have two structs representing the reparse data buffer in this code. One is the
ReparsePoint
struct and other is the newly addedReparseDataBuffer
struct. TheReparsePoint
struct only supports symlinks and mount points whileReparseDataBuffer
supports all kinds of reparse points. We have to keep theReparsePoint
struct for backwards compatibility. We want to use theSetReparsePoint
call for a buffer that could be an encoded buffer ofReparseDataBuffer
orReparsePoint
, hence the []byte array. Let me know what you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the bit that confused me is the comment, it states that it should be an encoded ReparseDataBuffer explicitly, so I thought this didn't care about the old definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the split here in API between Get taking in a path and Set using a handle is strange. Maybe we expose a method to call CreateFile with the minimum set of flags get/set need for the reparse operations to work and make them tweakable. I'd have to look more on what's needed..