Back up and restore Extended Attributes on Windows NTFS#4807
Back up and restore Extended Attributes on Windows NTFS#4807MichaelEischer merged 7 commits intorestic:masterfrom
Conversation
Refactor Extended Attribute related functions in node files as windows apis get and set EA in bulk
|
I thought this would be the smallest of the PRs, but the LOC bloat is mainly due to the multiple copyright comments and the test cases. |
MichaelEischer
left a comment
There was a problem hiding this comment.
That was surprisingly easy to review; there's just a shocking amount of boilerplate code (not that I see how we could change that). I've added a few minor comments. The most interesting one is probably whether it would make sense to import go-winio instead of copying some functions.
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| // SOFTWARE. | ||
|
|
||
| // The code below was copied over from https://github.com/microsoft/go-winio/blob/main/ea.go under MIT license. |
There was a problem hiding this comment.
I'm wondering whether it would make sense to import the directly usable functions from go-winio and only add the parts that are missing or require actual changes. Together with a previous PR, the code amounts to at least a few hundred lines of code, so it might make sense. I don't have a good estimate how move code we'd actually be able to save, what's your take on that?
There was a problem hiding this comment.
The important code that is being used in our ea_windows.go was taken from a PR of go-winio - microsoft/go-winio#229
This PR was never merged into go-winio, because they no longer needed those functions. They are using PAXHeaders for some tar related functionality and only use the encoding/decoding functions for EA in their ea.go.
Also, the code that we need like GetFileEA uses a private struct ioStatusBlock and private function ntStatus which are defined in go-winio. Hence, even if we import go-winio we would have to redefine that code in our ea.go.
The entire test case is from that unmerged PR as well.
Still, by importing go-winio, we will be able to use winio.EncodeExtendedAttributes and winio.DecodeExtendedAttributes.
ea_windows.go would reduce from 285 lines to 140 lines.
In sd_windows.go we would be able to directly use winio.EnableProcessPrivileges and all the privilege related functions would be handled in go-winio.
sd_windows would reduce from 440 lines to 189 lines.
Also, I see that by adding the go-winio module and making all the necessary changes, the windows binary size increases by 10 KB only.
So, it might make sense to use go-winio. We would have to use an older version 0.6.1 as the latest 0.6.2 requires go 1.21.
Would you like me to make the changes?
There was a problem hiding this comment.
How about the following: let's merge this PR with the duplicated code and import go-winio in a follow-up PR. My plan is to release restic 0.17.0 in a few weeks and after that is done, I'll bump the go requirement to 1.21. Then we could directly start with using 0.6.2
MichaelEischer
left a comment
There was a problem hiding this comment.
LGTM. Thanks!
I hope that we can release restic 0.17.0 in the next weeks, so the alternate data streams support might have to wait for the next release. Is there anything related to your PRs that has to be done before releasing 0.17.0?
As a heads-up, I'll move a lot of the node*.go to the fs package some time after restic 0.17.0 is released.
|
Sounds like plan, I'm good with these PRs for now. |
What does this PR change? What problem does it solve?
Restic did not back up Extended Attributes of files on Windows.
Restic now backs up and restores Extended Attributes for Windows NTFS files.
https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/ns-wdm-_file_full_ea_information
An extended attribute is a piece of application-specific metadata that an application can associate with a file that is not part of the file's data. In addition to the built-in attributes of a file, such as creation and modification times, applications can add non-file system attributes, such as the author's name and a description of the file content.
Was the change previously discussed in an issue or on the forum?
Follow-up to #4611
Checklist
changelog/unreleased/that describes the changes for our users (see template).gofmton the code in all commits.