Implement `winio.GetFileStandardInfo` #185

TBBle · 2020-12-12T14:41:45Z

This get-only API will be useful for detecting when a file has multiple links to it.

Specifically, I plan to use this to provide an optimisation for microsoft/hcsshim#901 (or afterwards, it's not a blocker...) by only attempting to recognise hard-links for files which have multiple links.

See TBBle/hcsshim@base-layer-manipulation...TBBle:hack-base-layer-manipulation-with-branched-go-winio for the example use-case.

fileinfo.go

TBBle · 2020-12-25T06:19:19Z

The discussion about bools reminded me to that I hadn't written any tests; It turns out the Directory and DeletePending flags are possibly swapped or otherwise faulty. I'm looking into it now, will repush once I have fixed it, and with tests. Marking this draft in the mean-time.

Update: Yeah, BOOL and BOOLEAN are distinct types with different sizes. And I'd used the wrong one.

TBBle · 2020-12-25T07:01:56Z

I was able to add tests for everything but the DeletePending field, assuming that a directory never has an AllocationSize or EndOfFile value other than 0, or a NumberOfLinks other than 1. (Creating files in the directory didn't affect it... I wonder if an alternative data stream would cause it to show data allocation? I assume the ADS would have its own distinct handle/allocation.)

As far as I can tell, the only way to trigger DeletePending is SetFileInformationByHandle with FileDispositionInfo. I tried FILE_FLAG_DELETE_ON_CLOSE and it still didn't appear to be true. (Unless the structure is still wrong somehow...)

fileinfo.go

TBBle · 2021-01-05T16:29:08Z

Flicking over to take advantage of golang.org/x/sys/windows ended up in a bit of a rabbit hole.

I wasn't sure if it's actually desirable to move from (Windows-specific) syscall to golang.org/x/sys/windows, so I did it just for go generate (because the mkwinsyscall script in Go 1.15 demanded that I do so) and fileinfo.go.

I did also convert the entire rest of the package, and if this is something that we actually want, rather than just a learning exercise for me, the branch is TBBle/go-winio@GetFileStandardInfo...TBBle:use-x_sys_windows-instead-of-syscall, and I can make it a PR after this lands.

~~The top commit of that branch also fixes a bug: pkg/fs/GetFileSystemType handles the return value from GetVolumeInformation differently from the documentation and the test confirms this.~~ Moved this to its own PR, #188.

The rest of the conversion should be ready now. I've only put it through the unit tests locally, but I did manually inspect every substitution except sometimes when I was replacing one generated function with another, and they had the same parameters.

On the other hand, if this PR just got too complex, it's easy enough to roll back to the last version and apply the uint32 fix that was probably the only blocker for this PR earlier. ~~I could also pull out the GetFileSystemType separately, since happily that code was stand-alone.~~

ambarve

LGTM. Just a minor nit.

fileinfo.go

This pulls in microsoft/go-winio#185, which is based on current go-winio master, to support more-efficient hardlink detection when exporting base layers. This ends up pulling a newer version of golang.org/x/sys too. Although not strictly necessary, this allows exercising this code when I pull this into containerd, since until microsoft#901 is merged, I need to pull _something_ for containerd anyway.

TBBle · 2021-01-22T11:35:56Z

Poke: I think this one's ready to go, and I'd like to land it so I can update microsoft/hcsshim#901 to take advantage of it, as well as flush it through so I can rebase my "Kill syscall" branch as a PR for consideration.

This pulls in microsoft/go-winio#185, which is based on current go-winio master, to support more-efficient hardlink detection when exporting base layers. This ends up pulling a newer version of golang.org/x/sys too. Although not strictly necessary, this allows exercising this code when I pull this into containerd, since until microsoft#901 is merged, I need to pull _something_ for containerd anyway.

thaJeztah · 2021-02-09T10:14:03Z

@kevpar ptal 🤗

katiewasnothere

LGTM, could you rebase?

This moves all the current users of $GOROOT/src/syscall/mksyscall_windows.go to instead use golang.org/x/sys/windows/mkwinsyscall, as directed by the version of the former in Go 1.15. It also syncs the local forks of mksyscall_windows.go with the latest version of golang.org/x/sys/windows/mkwinsyscall/mkwinsyscall.go, so that the local patches can be easily seen in a side-by-side comparison. Significant changes compared to the in-tree forked versions: * *bool parameters are read back through a temp-var, not directly like other pointer parameters. * ?-suffixed function names support testing for function presence before calling. This replaces a local implementation of this in pkg/security, which was not actually used anyway. The upstream version correctly supports functions that don't already have an error return. * `errnoErr(0)` is now useful, so each call of `errnoErr` doesn't need to be protected with a check for 0 first. * The generated functions are now sorted. This of course produced a *lot* of churn in the generated files. vhd\vhd.go was changed to generate syscalls into zvhd_windows.go, since regeneration removes the build tag added by hand in 9d82773. After all that, I also ran ``` go generate . .\pkg\etw\ .\pkg\process\ .\pkg\security\ .\vhd\ ``` to update all the existing generated code. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

At this point points of difference between the three local mksyscall_windows.go implementations and golang.org/x/sys/windows/mkwinsyscall v0.0.0-20210104204734-6f8348627aad are: - pkg/etw: Deduplicates imported functions due to multiple calling APIs - pkg/security and vhd: Forced UTF-16 mode, not checking A/W suffix. I also removed cosmetic differences between these two implementations. `go generate` reports no changes with these updates. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

We also get pre-defined constants instead of carrying our own copies. However, this changes the public API winio.FileBasicInfo from using syscall.Filetime to using windows.Filetime, as visible in backuptar/tar.go. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

TBBle · 2021-02-11T11:56:42Z

Rebased. Helpfully someone pulled in an even-newer golang.org/x/sys so the changes got shorter. ^_^

Edit: 🚀🚀🚀

This pulls in microsoft/go-winio#185, which is based on current go-winio master, to support more-efficient hardlink detection when exporting base layers. This ends up pulling a newer version of golang.org/x/sys too. Although not strictly necessary, this allows exercising this code when I pull this into containerd, since until microsoft#901 is merged, I need to pull _something_ for containerd anyway.

This pulls in an updated Microsoft/go-winio in order to use GetFileStandardInfo to recognise hard-linked files, added in microsoft/go-winio#185 Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

This pulls in an updated Microsoft/go-winio in order to use GetFileStandardInfo to recognise hard-linked files, added in microsoft/go-winio#185 Consequently, this pulls in containerd v1.5.0-beta1, as v1.4.0 and earlier have fork of go-winio's backuptar which has a different API in go-winio now. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

This pulls in an updated Microsoft/go-winio in order to use GetFileStandardInfo to recognise hard-linked files, added in microsoft/go-winio#185 Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

See microsoft/go-winio#185; this also pulls in a newer golang.org/x/sys release and some generated syscall cleanups. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

TBBle mentioned this pull request Dec 12, 2020

WCOW Base layer creation and export microsoft/hcsshim#901

Closed

anmaxvl reviewed Dec 22, 2020

View reviewed changes

fileinfo.go Outdated Show resolved Hide resolved

dcantah approved these changes Dec 23, 2020

View reviewed changes

TBBle marked this pull request as draft December 25, 2020 06:19

TBBle force-pushed the GetFileStandardInfo branch from a560866 to 8e16071 Compare December 25, 2020 06:58

TBBle marked this pull request as ready for review December 25, 2020 06:58

TBBle requested review from dcantah and anmaxvl December 25, 2020 07:13

anmaxvl approved these changes Dec 28, 2020

View reviewed changes

kevpar reviewed Jan 5, 2021

View reviewed changes

fileinfo.go Outdated Show resolved Hide resolved

kevpar reviewed Jan 5, 2021

View reviewed changes

fileinfo.go Outdated Show resolved Hide resolved

TBBle force-pushed the GetFileStandardInfo branch from 8e16071 to b35a191 Compare January 5, 2021 16:17

TBBle force-pushed the GetFileStandardInfo branch from b35a191 to 91aeaa6 Compare January 5, 2021 17:02

ambarve approved these changes Jan 11, 2021

View reviewed changes

fileinfo.go Outdated Show resolved Hide resolved

TBBle force-pushed the GetFileStandardInfo branch 2 times, most recently from d174434 to 373de8a Compare January 12, 2021 11:40

TBBle requested a review from kevpar January 29, 2021 18:59

katiewasnothere approved these changes Feb 11, 2021

View reviewed changes

TBBle added 4 commits February 11, 2021 22:47

Implement winio.GetFileStandardInfo

ef753e6

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

TBBle force-pushed the GetFileStandardInfo branch from 373de8a to ef753e6 Compare February 11, 2021 11:52

TBBle requested a review from a team as a code owner February 11, 2021 11:52

dcantah approved these changes Feb 11, 2021

View reviewed changes

dcantah merged commit 6eac466 into microsoft:master Feb 11, 2021

TBBle deleted the GetFileStandardInfo branch February 11, 2021 12:00

TBBle mentioned this pull request Feb 11, 2021

Use x/sys/windows instead of syscall #197

Open

TBBle mentioned this pull request Feb 17, 2021

Update go-winio to 6eac466e5fa3 for GetFileStandardInfo microsoft/hcsshim#947

Merged

thaJeztah mentioned this pull request Apr 20, 2021

Update go-winio package to latest version moby/moby#42307

Closed

katiewasnothere mentioned this pull request Apr 22, 2021

Revert removal of "Implement winio.GetFileStandardInfo FileInfo" commits #209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `winio.GetFileStandardInfo` #185

Implement `winio.GetFileStandardInfo` #185

TBBle commented Dec 12, 2020 •

edited

Loading

TBBle commented Dec 25, 2020 •

edited

Loading

TBBle commented Dec 25, 2020 •

edited

Loading

TBBle commented Jan 5, 2021 •

edited

Loading

ambarve left a comment

TBBle commented Jan 22, 2021

thaJeztah commented Feb 9, 2021

katiewasnothere left a comment

TBBle commented Feb 11, 2021 •

edited

Loading

Implement winio.GetFileStandardInfo #185

Implement winio.GetFileStandardInfo #185

Conversation

TBBle commented Dec 12, 2020 • edited Loading

TBBle commented Dec 25, 2020 • edited Loading

TBBle commented Dec 25, 2020 • edited Loading

TBBle commented Jan 5, 2021 • edited Loading

ambarve left a comment

Choose a reason for hiding this comment

TBBle commented Jan 22, 2021

thaJeztah commented Feb 9, 2021

katiewasnothere left a comment

Choose a reason for hiding this comment

TBBle commented Feb 11, 2021 • edited Loading

Implement `winio.GetFileStandardInfo` #185

Implement `winio.GetFileStandardInfo` #185

TBBle commented Dec 12, 2020 •

edited

Loading

TBBle commented Dec 25, 2020 •

edited

Loading

TBBle commented Dec 25, 2020 •

edited

Loading

TBBle commented Jan 5, 2021 •

edited

Loading

TBBle commented Feb 11, 2021 •

edited

Loading