From 605e8f53b13bfc13e7d2a35df1254590fc7eaad8 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Oct 2022 16:34:46 +0200 Subject: [PATCH 1/6] daemon/graphdriver/windows: InitFilter() don't use idtools.MkdirAllAndChown() idtools.MkdirAllAndChown on Windows does not chown directories, which makes idtools.MkdirAllAndChown() just an alias for system.MkDirAll(). Also setting the filemode to `0`, as changing filemode is a no-op on Windows as well; both of these changes should make it more transparent that no chown'ing, nor changing filemode takes place. Signed-off-by: Sebastiaan van Stijn --- daemon/graphdriver/windows/windows.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index 4328dee93eed0..550cd6de854f1 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -30,6 +30,7 @@ import ( "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/longpath" "github.com/docker/docker/pkg/reexec" + "github.com/docker/docker/pkg/system" units "github.com/docker/go-units" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -106,7 +107,9 @@ func InitFilter(home string, options []string, _ idtools.IdentityMapping) (graph return nil, fmt.Errorf("%s is on an ReFS volume - ReFS volumes are not supported", home) } - if err := idtools.MkdirAllAndChown(home, 0700, idtools.Identity{UID: 0, GID: 0}); err != nil { + // Setting file-mode is a no-op on Windows, so passing "0" to make it more + // transparent that the filemode passed has no effect. + if err = system.MkdirAll(home, 0); err != nil { return nil, fmt.Errorf("windowsfilter failed to create '%s': %v", home, err) } From 3a8c97be452e7fffd7a1bc2e708fc4df3857e225 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Oct 2022 16:57:29 +0200 Subject: [PATCH 2/6] daemon/graphdriver/windows: rename vars that collided with imports Signed-off-by: Sebastiaan van Stijn --- daemon/graphdriver/windows/windows.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index 550cd6de854f1..e0d37495396fa 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -223,14 +223,14 @@ func (d *Driver) create(id, parent, mountLabel string, readOnly bool, storageOpt return err } - storageOptions, err := parseStorageOpt(storageOpt) + storageOpts, err := parseStorageOpt(storageOpt) if err != nil { return fmt.Errorf("Failed to parse storage options - %s", err) } sandboxSize := d.defaultStorageOpts.size - if storageOptions.size != 0 { - sandboxSize = storageOptions.size + if storageOpts.size != 0 { + sandboxSize = storageOpts.size } if sandboxSize != 0 { @@ -663,7 +663,7 @@ func writeTarFromLayer(r hcsshim.LayerReader, w io.Writer) error { // exportLayer generates an archive from a layer based on the given ID. func (d *Driver) exportLayer(id string, parentLayerPaths []string) (io.ReadCloser, error) { - archive, w := io.Pipe() + archiveRdr, w := io.Pipe() go func() { err := winio.RunWithPrivilege(winio.SeBackupPrivilege, func() error { r, err := hcsshim.NewLayerReader(d.info, id, parentLayerPaths) @@ -681,7 +681,7 @@ func (d *Driver) exportLayer(id string, parentLayerPaths []string) (io.ReadClose w.CloseWithError(err) }() - return archive, nil + return archiveRdr, nil } // writeBackupStreamFromTarAndSaveMutatedFiles reads data from a tar stream and @@ -906,17 +906,17 @@ func (fg *fileGetCloserWithBackupPrivileges) Get(filename string) (io.ReadCloser // to the security descriptor. Also use sequential file access to avoid depleting the // standby list - Microsoft VSO Bug Tracker #9900466 err := winio.RunWithPrivilege(winio.SeBackupPrivilege, func() error { - path := longpath.AddPrefix(filepath.Join(fg.path, filename)) - p, err := windows.UTF16FromString(path) + longPath := longpath.AddPrefix(filepath.Join(fg.path, filename)) + p, err := windows.UTF16FromString(longPath) if err != nil { return err } const fileFlagSequentialScan = 0x08000000 // FILE_FLAG_SEQUENTIAL_SCAN h, err := windows.CreateFile(&p[0], windows.GENERIC_READ, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, windows.FILE_FLAG_BACKUP_SEMANTICS|fileFlagSequentialScan, 0) if err != nil { - return &os.PathError{Op: "open", Path: path, Err: err} + return &os.PathError{Op: "open", Path: longPath, Err: err} } - f = os.NewFile(uintptr(h), path) + f = os.NewFile(uintptr(h), longPath) return nil }) return f, err From d742188e3bbb1589d7acba5b9913aaf54a26c12e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Oct 2022 16:59:11 +0200 Subject: [PATCH 3/6] daemon/graphdriver/windows: remove fileFlagSequentialScan const Replace it with the const that's now defined in golang.org/x/sys/windows Signed-off-by: Sebastiaan van Stijn --- daemon/graphdriver/windows/windows.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index e0d37495396fa..ac92b8c9e85e8 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -911,8 +911,7 @@ func (fg *fileGetCloserWithBackupPrivileges) Get(filename string) (io.ReadCloser if err != nil { return err } - const fileFlagSequentialScan = 0x08000000 // FILE_FLAG_SEQUENTIAL_SCAN - h, err := windows.CreateFile(&p[0], windows.GENERIC_READ, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, windows.FILE_FLAG_BACKUP_SEMANTICS|fileFlagSequentialScan, 0) + h, err := windows.CreateFile(&p[0], windows.GENERIC_READ, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, windows.FILE_FLAG_BACKUP_SEMANTICS|windows.FILE_FLAG_SEQUENTIAL_SCAN, 0) if err != nil { return &os.PathError{Op: "open", Path: longPath, Err: err} } From 9db5dc9a465d5a3f6d9723fa5b5efdc26b8167f7 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Oct 2022 17:06:04 +0200 Subject: [PATCH 4/6] daemon/graphdriver/windows: use strings.EqualFold() Saves some allocations BenchmarkTolower BenchmarkTolower-5 7917788 150.4 ns/op 16 B/op 3 allocs/op BenchmarkEqualFold BenchmarkEqualFold-5 8248605 143.5 ns/op 8 B/op 1 allocs/op Signed-off-by: Sebastiaan van Stijn --- daemon/graphdriver/windows/windows.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index ac92b8c9e85e8..d929b3b296adb 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -103,7 +103,7 @@ func InitFilter(home string, options []string, _ idtools.IdentityMapping) (graph if err != nil { return nil, err } - if strings.ToLower(fsType) == "refs" { + if strings.EqualFold(fsType, "refs") { return nil, fmt.Errorf("%s is on an ReFS volume - ReFS volumes are not supported", home) } @@ -937,13 +937,12 @@ func (d *Driver) DiffGetter(id string) (graphdriver.FileGetCloser, error) { } func parseStorageOpt(storageOpt map[string]string) (*storageOptions, error) { - options := storageOptions{} + options := &storageOptions{} // Read size to change the block device size per container. for key, val := range storageOpt { - key := strings.ToLower(key) - switch key { - case "size": + // FIXME(thaJeztah): options should not be case-insensitive + if strings.EqualFold(key, "size") { size, err := units.RAMInBytes(val) if err != nil { return nil, err @@ -951,5 +950,5 @@ func parseStorageOpt(storageOpt map[string]string) (*storageOptions, error) { options.size = uint64(size) } } - return &options, nil + return options, nil } From bbeaeee3c71e15d1ec81abe231116f370376de95 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Oct 2022 17:29:15 +0200 Subject: [PATCH 5/6] daemon/graphdriver/windows: remove some intermediate variables Signed-off-by: Sebastiaan van Stijn --- daemon/graphdriver/windows/windows.go | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index d929b3b296adb..baf0afefcf176 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -480,7 +480,7 @@ func (d *Driver) Cleanup() error { // Diff produces an archive of the changes between the specified // layer and its parent layer which may be "". // The layer should be mounted when calling this function -func (d *Driver) Diff(id, parent string) (_ io.ReadCloser, err error) { +func (d *Driver) Diff(id, _ string) (_ io.ReadCloser, err error) { rID, err := d.resolveID(id) if err != nil { return @@ -516,7 +516,7 @@ func (d *Driver) Diff(id, parent string) (_ io.ReadCloser, err error) { // Changes produces a list of changes between the specified layer // and its parent layer. If parent is "", then all changes will be ADD changes. // The layer should not be mounted when calling this function. -func (d *Driver) Changes(id, parent string) ([]archive.Change, error) { +func (d *Driver) Changes(id, _ string) ([]archive.Change, error) { rID, err := d.resolveID(id) if err != nil { return nil, err @@ -627,9 +627,7 @@ func (d *Driver) DiffSize(id, parent string) (size int64, err error) { // GetMetadata returns custom driver information. func (d *Driver) GetMetadata(id string) (map[string]string, error) { - m := make(map[string]string) - m["dir"] = d.dir(id) - return m, nil + return map[string]string{"dir": d.dir(id)}, nil } func writeTarFromLayer(r hcsshim.LayerReader, w io.Writer) error { @@ -644,10 +642,9 @@ func writeTarFromLayer(r hcsshim.LayerReader, w io.Writer) error { } if fileInfo == nil { // Write a whiteout file. - hdr := &tar.Header{ + err = t.WriteHeader(&tar.Header{ Name: filepath.ToSlash(filepath.Join(filepath.Dir(name), archive.WhiteoutPrefix+filepath.Base(name))), - } - err := t.WriteHeader(hdr) + }) if err != nil { return err } @@ -817,12 +814,7 @@ func writeLayer(layerData io.Reader, home string, id string, parentLayerPaths .. }() } - info := hcsshim.DriverInfo{ - Flavour: filterDriver, - HomeDir: home, - } - - w, err := hcsshim.NewLayerWriter(info, id, parentLayerPaths) + w, err := hcsshim.NewLayerWriter(hcsshim.DriverInfo{Flavour: filterDriver, HomeDir: home}, id, parentLayerPaths) if err != nil { return 0, err } From 3b569cc68650c9ac7de3a1cd76b9c59680d25b1f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Oct 2022 17:48:03 +0200 Subject: [PATCH 6/6] daemon/graphdriver/windows: cleanup errors Signed-off-by: Sebastiaan van Stijn --- daemon/graphdriver/windows/windows.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index baf0afefcf176..835af9d718e6f 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -110,7 +110,7 @@ func InitFilter(home string, options []string, _ idtools.IdentityMapping) (graph // Setting file-mode is a no-op on Windows, so passing "0" to make it more // transparent that the filemode passed has no effect. if err = system.MkdirAll(home, 0); err != nil { - return nil, fmt.Errorf("windowsfilter failed to create '%s': %v", home, err) + return nil, errors.Wrapf(err, "windowsfilter failed to create '%s'", home) } storageOpt := map[string]string{ @@ -124,7 +124,7 @@ func InitFilter(home string, options []string, _ idtools.IdentityMapping) (graph opts, err := parseStorageOpt(storageOpt) if err != nil { - return nil, fmt.Errorf("windowsfilter failed to parse default storage options - %s", err) + return nil, errors.Wrap(err, "windowsfilter failed to parse default storage options") } d := &Driver{ @@ -225,7 +225,7 @@ func (d *Driver) create(id, parent, mountLabel string, readOnly bool, storageOpt storageOpts, err := parseStorageOpt(storageOpt) if err != nil { - return fmt.Errorf("Failed to parse storage options - %s", err) + return errors.Wrap(err, "failed to parse storage options") } sandboxSize := d.defaultStorageOpts.size @@ -244,7 +244,7 @@ func (d *Driver) create(id, parent, mountLabel string, readOnly bool, storageOpt if err2 := hcsshim.DestroyLayer(d.info, id); err2 != nil { logrus.Warnf("Failed to DestroyLayer %s: %s", id, err2) } - return fmt.Errorf("Cannot create layer with missing parent %s: %s", parent, err) + return errors.Wrapf(err, "cannot create layer with missing parent %s", parent) } if err := d.setLayerChain(id, layerChain); err != nil { @@ -855,13 +855,13 @@ func (d *Driver) getLayerChain(id string) ([]string, error) { if os.IsNotExist(err) { return nil, nil } else if err != nil { - return nil, fmt.Errorf("Unable to read layerchain file - %s", err) + return nil, errors.Wrapf(err, "read layerchain file") } var layerChain []string err = json.Unmarshal(content, &layerChain) if err != nil { - return nil, fmt.Errorf("Failed to unmarshall layerchain json - %s", err) + return nil, errors.Wrapf(err, "failed to unmarshal layerchain JSON") } return layerChain, nil @@ -871,13 +871,13 @@ func (d *Driver) getLayerChain(id string) ([]string, error) { func (d *Driver) setLayerChain(id string, chain []string) error { content, err := json.Marshal(&chain) if err != nil { - return fmt.Errorf("Failed to marshall layerchain json - %s", err) + return errors.Wrap(err, "failed to marshal layerchain JSON") } jPath := filepath.Join(d.dir(id), "layerchain.json") - err = os.WriteFile(jPath, content, 0600) + err = os.WriteFile(jPath, content, 0o600) if err != nil { - return fmt.Errorf("Unable to write layerchain file - %s", err) + return errors.Wrap(err, "write layerchain file") } return nil