Add Getter Setters for Extended Attributes and Reparse points. #229
Conversation
ea.go
Outdated
| return GetFileEAHandle(windows.Handle(f.Fd())) | ||
| } | ||
|
|
||
| func GetFileEAHandle(handle windows.Handle) ([]ExtendedAttribute, error) { |
There was a problem hiding this comment.
With the way the *Handle methods are named it seems like I'd expect to get back some handle to the EA's themselves which doesn't make sense (guess this is the way my brain works haha). Is exposing string and handle equivalents just to make this as flexible as possible?
There was a problem hiding this comment.
Do we really need the shortcut methods that take a file path? It's probably not too onerous to expect callers to pass a handle.
There was a problem hiding this comment.
Right, I think dropping the Handle off the names and having these be the only functions makes sense to me at least. Not the only function we have in this project that just takes in a handle and has no shortcut string equivalent https://github.com/microsoft/go-winio/blob/master/file.go#L113
There was a problem hiding this comment.
I added the method which takes the file path mainly for convenience and also so that we can encapsulate correct file opening permissions (for example, you need to open the file with specific access right when gettting/setting EA, or when gettting/setting reparse points). However, there are times when you already have a handle open for a file and you just want to pass in that (probably because you opened it in non sharing mode) that's why I added the other method. I agree that the handle methods can be named better but I think keeping both can be helpful. What do you think?
There was a problem hiding this comment.
If our use case for these is to open a handle, and then query SDs, EAs and reparse points it makes sense to just have the NtAPI style that take handles. Otherwise there is a separate createfile on each function call.
There was a problem hiding this comment.
I think for reparse points its best to have the functions that take file paths but for EA we mostly deal with the handles so I have kept handles there. For SDs there is no particular preference but the wrapper from golang.org/x/sys/windows already provides a function which takes in the handle so I kept the file path version here.
There was a problem hiding this comment.
Can you explain this comment a bit more? Why is it best to take paths for reparsepoint functions?
There was a problem hiding this comment.
I only said that because in order to get handle to a reparse point you need to open it with the FLAG_OPEN_REPARSE_POINT flag so its better to do that in the call that gets or sets the reparse points than expecting the caller to do it.
There was a problem hiding this comment.
@msscotb , @kevpar , @dcantah I have updated the PR, with the latest set of changes getters/setters for EAs take a file handle, getter for SDDL takes a path and getter for reparse takes a path but setter for reparse takes a file handle. The reason for this is that a reparse point could be created on a normal file or a directory so I have left the job of opening the handle to it on the caller. Let me know if this looks good.
|
What will this be used for? A PR description and commit descriptions will be helpful |
ea.go
Outdated
| return GetFileEAHandle(windows.Handle(f.Fd())) | ||
| } | ||
|
|
||
| func GetFileEAHandle(handle windows.Handle) ([]ExtendedAttribute, error) { |
There was a problem hiding this comment.
If our use case for these is to open a handle, and then query SDs, EAs and reparse points it makes sense to just have the NtAPI style that take handles. Otherwise there is a separate createfile on each function call.
|
I almost forgot we have a Linux CI here now haha, but it's not happy with this PR apparently. |
ambarve
force-pushed
the
get_set_ea
branch
2 times, most recently
from
8deb21d
to
7755641
Compare
Why do we need Linux CI for this repo? |
|
|
||
| // SetReparsePoint sets a reparse point with `data` on the file/directory represented by | ||
| // `handle` . `data` should be an encoded ReparseDataBuffer struct. | ||
| func SetReparsePoint(handle windows.Handle, data []byte) error { |
There was a problem hiding this comment.
Would it make sense to make these "friendly" APIs and just take in ReparseDataBuffers directly and we encode inside?
e.g.
func SetReparsePoint(handle windows.Handle, data *ReparseDataBuffer) error {
dataBuf := data.Encode()
// rest of the function
}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.
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 added ReparseDataBuffer struct. The ReparsePoint struct only supports symlinks and mount points while ReparseDataBuffer supports all kinds of reparse points. We have to keep the ReparsePoint struct for backwards compatibility. We want to use the SetReparsePoint call for a buffer that could be an encoded buffer of ReparseDataBuffer or ReparsePoint, hence the []byte array. Let me know what you think?
There was a problem hiding this comment.
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
|
@ambarve We were looking to get out a new winio release soon for Hamza's hvsocket changes/fixes, lemme know if you think we'd be able to get this in |
Signed-off-by: Amit Barve <[email protected]>
| } else { | ||
| return nil, fmt.Errorf("get file EA failed with: %w", status.Err()) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
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.
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 better
for {
status := getFileEA(handle, &iosb, &buf[0], uint32(bufLen), false, 0, 0, nil, true)
if status.Err() != nil {
// convert ntstatus code to windows error
if status.Err() == windows.ERROR_INSUFFICIENT_BUFFER || status.Err() == windows.ERROR_MORE_DATA {
bufLen *= 2
buf = make([]byte, bufLen)
continue
}
return nil, fmt.Errorf("get file EA failed with: %w", status.Err())
}
break
}
There was a problem hiding this comment.
I see what you mean, that does seem a bit more concise.
| func GetReparsePoint(path string) ([]byte, error) { | ||
| utf16DestPath, err := windows.UTF16FromString(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // We need to open the file with windows specific permissions so just using | ||
| // os.Open won't work. | ||
| fileHandle, err := windows.CreateFile(&utf16DestPath[0], windows.GENERIC_READ, 0, nil, windows.OPEN_EXISTING, (windows.FILE_ATTRIBUTE_NORMAL | windows.FILE_FLAG_OPEN_REPARSE_POINT | windows.FILE_FLAG_BACKUP_SEMANTICS), 0) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("open file failed with: %w", err) | ||
| } | ||
| defer windows.Close(fileHandle) | ||
|
|
||
| outBuf := make([]byte, _MAXIMUM_REPARSE_DATA_BUFFER_SIZE) | ||
| var outBufLen uint32 | ||
| err = windows.DeviceIoControl(fileHandle, _FSCTL_GET_REPARSE_POINT, nil, 0, &outBuf[0], _MAXIMUM_REPARSE_DATA_BUFFER_SIZE, &outBufLen, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get reparse point for file %s: %w", path, err) | ||
| } | ||
|
|
||
| return outBuf[:outBufLen], nil | ||
| } | ||
|
|
||
| // SetReparsePoint sets a reparse point with `data` on the file/directory represented by | ||
| // `handle` . `data` should be an encoded ReparseDataBuffer struct. | ||
| func SetReparsePoint(handle windows.Handle, data []byte) error { |
There was a problem hiding this comment.
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..
| // keep increasing the buffer size until it is large enough | ||
| for { | ||
| status := getFileEA(handle, &iosb, &buf[0], uint32(bufLen), false, 0, 0, nil, true) | ||
| if status.Err() != nil { |
There was a problem hiding this comment.
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 -
status := getFileEA(handle, &iosb, &buf[0], uint32(bufLen), false, 0, 0, nil, true)
if status == -1073741742 {
//If status is -1073741742, no extended attributes were found
return nil, nil
} else {
err := status.Err()
if err != nil {
// convert ntstatus code to windows error
if err == windows.ERROR_INSUFFICIENT_BUFFER || err == windows.ERROR_MORE_DATA {
bufLen *= 2
buf = make([]byte, bufLen)
continue
}
return nil, fmt.Errorf("get file EA failed with: %w", err)
}
}
break
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?
| t.Logf("expected: %+v, found: %+v\n", testEAs, readEAs) | ||
| t.Fatalf("EAs read from testfile don't match") | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
func Test_SetGetFolderEA(t *testing.T) {
tempDir := t.TempDir()
testfolderPath := filepath.Join(tempDir, "testfolder")
// create temp folder
err := os.Mkdir(testfolderPath, os.ModeDir)
if err != nil {
t.Fatalf("failed to create temporary file: %s", err)
}
nAttrs := 3
testEAs := make([]ExtendedAttribute, 3)
// generate random extended attributes for test
for i := 0; i < nAttrs; i++ {
// EA name is automatically converted to upper case before storing, so
// when reading it back it returns the upper case name. To avoid test
// failures because of that keep the name upper cased.
testEAs[i].Name = fmt.Sprintf("TESTEA%d", i+1)
testEAs[i].Value = make([]byte, rand.Int31n(math.MaxUint8))
rand.Read(testEAs[i].Value)
}
utf16Path := windows.StringToUTF16Ptr(testfolderPath)
fileAccessRightReadWriteEA := (0x8 | 0x10)
fileHandle, err := windows.CreateFile(utf16Path, uint32(fileAccessRightReadWriteEA), 0, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_BACKUP_SEMANTICS, 0)
if err != nil {
t.Fatalf("open folder failed with: %s", err)
}
defer windows.Close(fileHandle)
if err := SetFileEA(fileHandle, testEAs); err != nil {
t.Fatalf("set EA for folder failed: %s", err)
}
var readEAs []ExtendedAttribute
if readEAs, err = GetFileEA(fileHandle); err != nil {
t.Fatalf("get EA for folder failed: %s", err)
}
if !reflect.DeepEqual(readEAs, testEAs) {
t.Logf("expected: %+v, found: %+v\n", testEAs, readEAs)
t.Fatalf("EAs read from test folder don't match")
}
}
|
These changes are not required any more so closing this out. Will be reopened later if required. |
This PR adds the Go wrappers for getting / setting the extended file attributes and reparse points. This is required for preparing the container scratch directly from hcsshim instead of calling into existing OS APIs.