Skip ExtendedAttribute processing in Windows for volumes that do not support EA#4980
Conversation
MichaelEischer
left a comment
There was a problem hiding this comment.
I guess we can give this approach a try. I've found a few problems though, see below.
internal/fs/ea_windows.go
Outdated
|
|
||
| kernel32dll = syscall.NewLazyDLL("kernel32.dll") | ||
|
|
||
| procGetVolumeInformationW = kernel32dll.NewProc("GetVolumeInformationW") |
There was a problem hiding this comment.
Please use https://pkg.go.dev/golang.org/x/sys/windows#GetVolumeInformation instead of manually building a wrapper.
internal/fs/ea_windows.go
Outdated
| const ( | ||
| // fileSupportsExtendedAttributes is a bitmask that indicates whether the file system supports extended attributes. | ||
| // https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_fs_attribute_information | ||
| fileSupportsExtendedAttributes = 0x00800000 |
There was a problem hiding this comment.
that constant is also available in x/sys/windows with the name FILE_SUPPORTS_EXTENDED_ATTRIBUTES.
internal/fs/ea_windows.go
Outdated
| } | ||
| ret, _, err := procGetVolumeInformationW.Call( | ||
| uintptr(unsafe.Pointer(utf16Path)), | ||
| uintptr(unsafe.Pointer(&volumeName[0])), |
There was a problem hiding this comment.
Most output parameters are optional.
| return false, nil | ||
| } | ||
| if !strings.HasSuffix(filepath.Clean(path), `\`) { | ||
| if strings.HasSuffix(filepath.Clean(path), `\`) { |
There was a problem hiding this comment.
This approach won't work for backups using relative paths.
cd /D Z: # some network share
restic backup local/file
There was a problem hiding this comment.
Thanks, I believe I've addressed this now.
internal/restic/node_windows.go
Outdated
| procDecryptFile = modAdvapi32.NewProc("DecryptFileW") | ||
|
|
||
| // eaUnsupportedVolumesMap is a map of volumes that do not support extended attributes. | ||
| eaUnsupportedVolumesMap = map[string]bool{} |
There was a problem hiding this comment.
I'm really no fan of introducing additional global state as it usually leads to problems sooner or later. However, until I've moved the NodeFromFileInfo and related code into the internal/fs package, I don't see a good way to avoid it. Either way this map MUST be synchronized.
There was a problem hiding this comment.
I agree. We can revisit later
Fixed using sync.Map
MichaelEischer
left a comment
There was a problem hiding this comment.
This seems to almost work, see the comments for the remaining problems.
| //Extended length path prefix needs to be trimmed to get the volume name correctly | ||
| path = path[len(extendedPathPrefix):] | ||
| } else if strings.HasPrefix(path, globalRootPrefix) { | ||
| // EAs are not supported for \\?\GLOBALROOT i.e. VSS snapshots |
There was a problem hiding this comment.
I made a mistake here while fixing the test case errors.
Extended attributes are in fact supported for VSS snapshots.
Instead of returning false, we should be handling the volume names for VSS snapshots correctly. Apologies, I can add a separate PR.
There was a problem hiding this comment.
Instead of returning false, we should be handling the volume names for VSS snapshots correctly. Apologies, I can add a separate PR.
Does GetVolumeInformation support such paths or do we needs even more weird special case handling?
There was a problem hiding this comment.
We need to get the 'volumeName' path like this -
// Extract the VSS snapshot volume name eg. `\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopyXX`
if parts := strings.SplitN(path, `\`, 7); len(parts) >= 6 {
volumeName = strings.Join(parts[:6], `\`)
}
Rest of the code remains same-
fs.PathSupportsExtendedAttributes(volumeName + \) i.e. GetVolumeInformation works.
There was a problem hiding this comment.
Sigh, I'm not happy about all those Windows special case. But looks like we'd better add them. Can you open a PR for it?
What does this PR change? What problem does it solve?
Some windows paths like network drives do not support ExtendedAttributes. Either they return error codes like windows.E_NOT_SET or windows.ERROR_INVALID_FUNCTION or it results in slower backups.
This PR aims to skip processing the ExtendedAttributes after detecting if the volume does not support them.
It completely skips the attempt to fetch Extended Attributes instead of trying to handle errors after trying to fetch EA for files on such volumes.
Was the change previously discussed in an issue or on the forum?
May resolve #4955 and #4950
Checklist
changelog/unreleased/that describes the changes for our users (see template).gofmton the code in all commits.