Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor copy of slices and maps #2128

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+80 −141
Open

Refactor copy of slices and maps #2128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
624a0ab
Refactor operations with slices
Honny1 Oct 8, 2024
13a96fe
Refactor of copying maps
Honny1 Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions containers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,16 @@ type containerStore struct {
func copyContainer(c *Container) *Container {
return &Container{
ID: c.ID,
Names: copyStringSlice(c.Names),
Names: copySlicePreferringNil(c.Names),
ImageID: c.ImageID,
LayerID: c.LayerID,
Metadata: c.Metadata,
BigDataNames: copyStringSlice(c.BigDataNames),
BigDataNames: copySlicePreferringNil(c.BigDataNames),
BigDataSizes: maps.Clone(c.BigDataSizes),
BigDataDigests: maps.Clone(c.BigDataDigests),
Created: c.Created,
UIDMap: copyIDMap(c.UIDMap),
GIDMap: copyIDMap(c.GIDMap),
UIDMap: copySlicePreferringNil(c.UIDMap),
GIDMap: copySlicePreferringNil(c.GIDMap),
Flags: maps.Clone(c.Flags),
volatileStore: c.volatileStore,
}
Expand Down Expand Up @@ -693,8 +693,8 @@ func (r *containerStore) create(id string, names []string, image, layer string,
BigDataDigests: make(map[string]digest.Digest),
Created: time.Now().UTC(),
Flags: copyStringInterfaceMap(options.Flags),
UIDMap: copyIDMap(options.UIDMap),
GIDMap: copyIDMap(options.GIDMap),
UIDMap: copySlicePreferringNil(options.UIDMap),
GIDMap: copySlicePreferringNil(options.GIDMap),
volatileStore: options.Volatile,
}
if options.MountOpts != nil {
Expand Down Expand Up @@ -906,7 +906,7 @@ func (r *containerStore) BigDataNames(id string) ([]string, error) {
if !ok {
return nil, ErrContainerUnknown
}
return copyStringSlice(c.BigDataNames), nil
return copySlicePreferringNil(c.BigDataNames), nil
}

// Requires startWriting.
Expand Down
14 changes: 7 additions & 7 deletions images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,13 +183,13 @@ func copyImage(i *Image) *Image {
return &Image{
ID: i.ID,
Digest: i.Digest,
Digests: copyDigestSlice(i.Digests),
Names: copyStringSlice(i.Names),
NamesHistory: copyStringSlice(i.NamesHistory),
Digests: copySlicePreferringNil(i.Digests),
Names: copySlicePreferringNil(i.Names),
NamesHistory: copySlicePreferringNil(i.NamesHistory),
TopLayer: i.TopLayer,
MappedTopLayers: copyStringSlice(i.MappedTopLayers),
MappedTopLayers: copySlicePreferringNil(i.MappedTopLayers),
Metadata: i.Metadata,
BigDataNames: copyStringSlice(i.BigDataNames),
BigDataNames: copySlicePreferringNil(i.BigDataNames),
BigDataSizes: maps.Clone(i.BigDataSizes),
BigDataDigests: maps.Clone(i.BigDataDigests),
Created: i.Created,
Expand Down Expand Up @@ -718,7 +718,7 @@ func (r *imageStore) create(id string, names []string, layer string, options Ima
Digest: options.Digest,
Digests: dedupeDigests(options.Digests),
Names: names,
NamesHistory: copyStringSlice(options.NamesHistory),
NamesHistory: copySlicePreferringNil(options.NamesHistory),
TopLayer: layer,
Metadata: options.Metadata,
BigDataNames: []string{},
Expand Down Expand Up @@ -967,7 +967,7 @@ func (r *imageStore) BigDataNames(id string) ([]string, error) {
if !ok {
return nil, fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown)
}
return copyStringSlice(image.BigDataNames), nil
return copySlicePreferringNil(image.BigDataNames), nil
}

// Requires startWriting.
Expand Down
18 changes: 9 additions & 9 deletions layers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ func layerLocation(l *Layer) layerLocations {
func copyLayer(l *Layer) *Layer {
return &Layer{
ID: l.ID,
Names: copyStringSlice(l.Names),
Names: copySlicePreferringNil(l.Names),
Parent: l.Parent,
Metadata: l.Metadata,
MountLabel: l.MountLabel,
Expand All @@ -451,12 +451,12 @@ func copyLayer(l *Layer) *Layer {
CompressionType: l.CompressionType,
ReadOnly: l.ReadOnly,
volatileStore: l.volatileStore,
BigDataNames: copyStringSlice(l.BigDataNames),
BigDataNames: copySlicePreferringNil(l.BigDataNames),
Flags: maps.Clone(l.Flags),
UIDMap: copyIDMap(l.UIDMap),
GIDMap: copyIDMap(l.GIDMap),
UIDs: copyUint32Slice(l.UIDs),
GIDs: copyUint32Slice(l.GIDs),
UIDMap: copySlicePreferringNil(l.UIDMap),
GIDMap: copySlicePreferringNil(l.GIDMap),
UIDs: copySlicePreferringNil(l.UIDs),
GIDs: copySlicePreferringNil(l.GIDs),
}
}

Expand Down Expand Up @@ -1404,8 +1404,8 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount
UIDs: templateUIDs,
GIDs: templateGIDs,
Flags: copyStringInterfaceMap(moreOptions.Flags),
UIDMap: copyIDMap(moreOptions.UIDMap),
GIDMap: copyIDMap(moreOptions.GIDMap),
UIDMap: copySlicePreferringNil(moreOptions.UIDMap),
GIDMap: copySlicePreferringNil(moreOptions.GIDMap),
BigDataNames: []string{},
volatileStore: moreOptions.Volatile,
}
Expand Down Expand Up @@ -1840,7 +1840,7 @@ func (r *layerStore) BigDataNames(id string) ([]string, error) {
if !ok {
return nil, fmt.Errorf("locating layer with ID %q to retrieve bigdata names: %w", id, ErrImageUnknown)
}
return copyStringSlice(layer.BigDataNames), nil
return copySlicePreferringNil(layer.BigDataNames), nil
}

// Requires startReading or startWriting.
Expand Down
125 changes: 31 additions & 94 deletions store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,8 +877,8 @@ func GetStore(options types.StoreOptions) (Store, error) {
graphOptions: options.GraphDriverOptions,
imageStoreDir: options.ImageStore,
pullOptions: options.PullOptions,
uidMap: copyIDMap(options.UIDMap),
gidMap: copyIDMap(options.GIDMap),
uidMap: copySlicePreferringNil(options.UIDMap),
gidMap: copySlicePreferringNil(options.GIDMap),
autoUsernsUser: options.RootAutoNsUser,
autoNsMinSize: autoNsMinSize,
autoNsMaxSize: autoNsMaxSize,
Expand All @@ -897,30 +897,6 @@ func GetStore(options types.StoreOptions) (Store, error) {
return s, nil
}

func copyUint32Slice(slice []uint32) []uint32 {
m := []uint32{}
if slice != nil {
m = make([]uint32, len(slice))
copy(m, slice)
}
if len(m) > 0 {
return m[:]
}
return nil
}

func copyIDMap(idmap []idtools.IDMap) []idtools.IDMap {
m := []idtools.IDMap{}
if idmap != nil {
m = make([]idtools.IDMap, len(idmap))
copy(m, idmap)
}
if len(m) > 0 {
return m[:]
}
return nil
}

func (s *store) RunRoot() string {
return s.runRoot
}
Expand Down Expand Up @@ -952,11 +928,11 @@ func (s *store) PullOptions() map[string]string {
}

func (s *store) UIDMap() []idtools.IDMap {
return copyIDMap(s.uidMap)
return copySlicePreferringNil(s.uidMap)
}

func (s *store) GIDMap() []idtools.IDMap {
return copyIDMap(s.gidMap)
return copySlicePreferringNil(s.gidMap)
}

// This must only be called when constructing store; it writes to fields that are assumed to be constant after construction.
Expand Down Expand Up @@ -1469,7 +1445,7 @@ func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, pare
var options LayerOptions
if lOptions != nil {
options = *lOptions
options.BigData = copyLayerBigDataOptionSlice(lOptions.BigData)
options.BigData = slices.Clone(lOptions.BigData)
options.Flags = maps.Clone(lOptions.Flags)
}
if options.HostUIDMapping {
Expand Down Expand Up @@ -1541,8 +1517,8 @@ func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, pare
options.IDMappingOptions = types.IDMappingOptions{
HostUIDMapping: options.HostUIDMapping,
HostGIDMapping: options.HostGIDMapping,
UIDMap: copyIDMap(uidMap),
GIDMap: copyIDMap(gidMap),
UIDMap: copySlicePreferringNil(uidMap),
GIDMap: copySlicePreferringNil(gidMap),
}
}
return rlstore.create(id, parentLayer, names, mountLabel, nil, &options, writeable, diff, slo)
Expand Down Expand Up @@ -1610,8 +1586,8 @@ func (s *store) CreateImage(id string, names []string, layer, metadata string, i
Metadata: i.Metadata,
CreationDate: i.Created,
Digest: i.Digest,
Digests: copyDigestSlice(i.Digests),
NamesHistory: copyStringSlice(i.NamesHistory),
Digests: copySlicePreferringNil(i.Digests),
NamesHistory: copySlicePreferringNil(i.NamesHistory),
}
for _, key := range i.BigDataNames {
data, err := store.BigData(id, key)
Expand Down Expand Up @@ -1646,7 +1622,7 @@ func (s *store) CreateImage(id string, names []string, layer, metadata string, i
if iOptions.Metadata != "" {
options.Metadata = iOptions.Metadata
}
options.BigData = append(options.BigData, copyImageBigDataOptionSlice(iOptions.BigData)...)
options.BigData = append(options.BigData, slices.Clone(iOptions.BigData)...)
options.NamesHistory = append(options.NamesHistory, iOptions.NamesHistory...)
if options.Flags == nil {
options.Flags = make(map[string]interface{})
Expand Down Expand Up @@ -1760,8 +1736,8 @@ func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, rlst
layerOptions.IDMappingOptions = types.IDMappingOptions{
HostUIDMapping: options.HostUIDMapping,
HostGIDMapping: options.HostGIDMapping,
UIDMap: copyIDMap(options.UIDMap),
GIDMap: copyIDMap(options.GIDMap),
UIDMap: copySlicePreferringNil(options.UIDMap),
GIDMap: copySlicePreferringNil(options.GIDMap),
}
}
layerOptions.TemplateLayer = layer.ID
Expand All @@ -1783,13 +1759,13 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat
var options ContainerOptions
if cOptions != nil {
options = *cOptions
options.IDMappingOptions.UIDMap = copyIDMap(cOptions.IDMappingOptions.UIDMap)
options.IDMappingOptions.GIDMap = copyIDMap(cOptions.IDMappingOptions.GIDMap)
options.LabelOpts = copyStringSlice(cOptions.LabelOpts)
options.IDMappingOptions.UIDMap = copySlicePreferringNil(cOptions.IDMappingOptions.UIDMap)
options.IDMappingOptions.GIDMap = copySlicePreferringNil(cOptions.IDMappingOptions.GIDMap)
options.LabelOpts = copySlicePreferringNil(cOptions.LabelOpts)
options.Flags = maps.Clone(cOptions.Flags)
options.MountOpts = copyStringSlice(cOptions.MountOpts)
options.MountOpts = copySlicePreferringNil(cOptions.MountOpts)
options.StorageOpt = copyStringStringMap(cOptions.StorageOpt)
options.BigData = copyContainerBigDataOptionSlice(cOptions.BigData)
options.BigData = slices.Clone(cOptions.BigData)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slices.Clone makes a shallow clone, i.e. Data slice is not really copied with this change.

}
if options.HostUIDMapping {
options.UIDMap = nil
Expand Down Expand Up @@ -1913,8 +1889,8 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat
layerOptions.IDMappingOptions = types.IDMappingOptions{
HostUIDMapping: idMappingsOptions.HostUIDMapping,
HostGIDMapping: idMappingsOptions.HostGIDMapping,
UIDMap: copyIDMap(uidMap),
GIDMap: copyIDMap(gidMap),
UIDMap: copySlicePreferringNil(uidMap),
GIDMap: copySlicePreferringNil(gidMap),
}
}
if options.Flags == nil {
Expand Down Expand Up @@ -1951,8 +1927,8 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat
options.IDMappingOptions = types.IDMappingOptions{
HostUIDMapping: len(options.UIDMap) == 0,
HostGIDMapping: len(options.GIDMap) == 0,
UIDMap: copyIDMap(options.UIDMap),
GIDMap: copyIDMap(options.GIDMap),
UIDMap: copySlicePreferringNil(options.UIDMap),
GIDMap: copySlicePreferringNil(options.GIDMap),
}
container, err := s.containerStore.create(id, names, imageID, layer, &options)
if err != nil || container == nil {
Expand Down Expand Up @@ -2450,9 +2426,9 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e
options := ImageOptions{
CreationDate: i.Created,
Digest: i.Digest,
Digests: copyDigestSlice(i.Digests),
Digests: copySlicePreferringNil(i.Digests),
Metadata: i.Metadata,
NamesHistory: copyStringSlice(i.NamesHistory),
NamesHistory: copySlicePreferringNil(i.NamesHistory),
Flags: copyStringInterfaceMap(i.Flags),
}
for _, key := range i.BigDataNames {
Expand Down Expand Up @@ -3674,23 +3650,18 @@ func makeBigDataBaseName(key string) string {
}

func stringSliceWithoutValue(slice []string, value string) []string {
modified := make([]string, 0, len(slice))
for _, v := range slice {
if v == value {
continue
}
modified = append(modified, v)
}
return modified
return slices.DeleteFunc(slices.Clone(slice), func(v string) bool {
return v == value
})
}

func copyStringSlice(slice []string) []string {
if len(slice) == 0 {
// copySlicePreferringNil returns a copy of the slice.
// If s is empty, a nil is returned.
func copySlicePreferringNil[S ~[]E, E any](s S) S {
if len(s) == 0 {
return nil
}
ret := make([]string, len(slice))
copy(ret, slice)
return ret
return slices.Clone(s)
}

func copyStringStringMap(m map[string]string) map[string]string {
Expand All @@ -3701,15 +3672,6 @@ func copyStringStringMap(m map[string]string) map[string]string {
return ret
}

func copyDigestSlice(slice []digest.Digest) []digest.Digest {
if len(slice) == 0 {
return nil
}
ret := make([]digest.Digest, len(slice))
copy(ret, slice)
return ret
}

// copyStringInterfaceMap still forces us to assume that the interface{} is
// a non-pointer scalar value
func copyStringInterfaceMap(m map[string]interface{}) map[string]interface{} {
Expand All @@ -3720,31 +3682,6 @@ func copyStringInterfaceMap(m map[string]interface{}) map[string]interface{} {
return ret
}

func copyLayerBigDataOptionSlice(slice []LayerBigDataOption) []LayerBigDataOption {
ret := make([]LayerBigDataOption, len(slice))
copy(ret, slice)
return ret
}

func copyImageBigDataOptionSlice(slice []ImageBigDataOption) []ImageBigDataOption {
ret := make([]ImageBigDataOption, len(slice))
for i := range slice {
ret[i].Key = slice[i].Key
ret[i].Data = slices.Clone(slice[i].Data)
ret[i].Digest = slice[i].Digest
}
return ret
}

func copyContainerBigDataOptionSlice(slice []ContainerBigDataOption) []ContainerBigDataOption {
ret := make([]ContainerBigDataOption, len(slice))
for i := range slice {
ret[i].Key = slice[i].Key
ret[i].Data = slices.Clone(slice[i].Data)
}
return ret
}

// AutoUserNsMinSize is the minimum size for automatically created user namespaces
const AutoUserNsMinSize = 1024

Expand Down