Skip to content

Add a function to clone map and use it where appropriate #1397

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.

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

Merged
merged 2 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 7 additions & 16 deletions cmd/incusd/api_1.0.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/lxc/incus/v6/shared/osarch"
"github.com/lxc/incus/v6/shared/revert"
localtls "github.com/lxc/incus/v6/shared/tls"
"github.com/lxc/incus/v6/shared/util"
)

var api10Cmd = APIEndpoint{
Expand Down Expand Up @@ -459,10 +460,7 @@ func api10Put(d *Daemon, r *http.Request) response.Response {
// for reacting to the values that changed.
if isClusterNotification(r) {
logger.Debug("Handling config changed notification")
changed := make(map[string]string)
for key, value := range req.Config {
changed[key] = value
}
changed := util.CloneMap(req.Config)

// Get the current (updated) config.
var config *clusterConfig.Config
Expand Down Expand Up @@ -587,7 +585,7 @@ func doApi10Update(d *Daemon, r *http.Request, req api.ServerPut, patch bool) re

nodeChanged := map[string]string{}
var newNodeConfig *node.Config
oldNodeConfig := make(map[string]string)
var oldNodeConfig map[string]string

err := s.DB.Node.Transaction(r.Context(), func(ctx context.Context, tx *db.NodeTx) error {
var err error
Expand All @@ -597,9 +595,7 @@ func doApi10Update(d *Daemon, r *http.Request, req api.ServerPut, patch bool) re
}

// Keep old config around in case something goes wrong. In that case the config will be reverted.
for k, v := range newNodeConfig.Dump() {
oldNodeConfig[k] = v
}
oldNodeConfig = util.CloneMap(newNodeConfig.Dump())

// We currently don't allow changing the cluster.https_address once it's set.
if s.ServerClustered {
Expand Down Expand Up @@ -687,7 +683,7 @@ func doApi10Update(d *Daemon, r *http.Request, req api.ServerPut, patch bool) re
// Then deal with cluster wide configuration
var clusterChanged map[string]string
var newClusterConfig *clusterConfig.Config
oldClusterConfig := make(map[string]string)
var oldClusterConfig map[string]string

err = s.DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error {
var err error
Expand All @@ -697,9 +693,7 @@ func doApi10Update(d *Daemon, r *http.Request, req api.ServerPut, patch bool) re
}

// Keep old config around in case something goes wrong. In that case the config will be reverted.
for k, v := range newClusterConfig.Dump() {
oldClusterConfig[k] = v
}
oldClusterConfig = util.CloneMap(newClusterConfig.Dump())

if patch {
clusterChanged, err = newClusterConfig.Patch(req.Config)
Expand Down Expand Up @@ -760,11 +754,8 @@ func doApi10Update(d *Daemon, r *http.Request, req api.ServerPut, patch bool) re
}

serverPut := server.Writable()
serverPut.Config = make(map[string]string)
// Only propagated cluster-wide changes
for key, value := range clusterChanged {
serverPut.Config[key] = value
}
serverPut.Config = util.CloneMap(clusterChanged)

return client.UpdateServer(serverPut, etag)
})
Expand Down
6 changes: 1 addition & 5 deletions cmd/incusd/api_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -759,11 +759,7 @@ func clusterPutJoin(d *Daemon, r *http.Request, req api.ClusterPut) response.Res
d.globalConfig = currentClusterConfig
d.globalConfigMu.Unlock()

existingConfigDump := currentClusterConfig.Dump()
changes := make(map[string]string, len(existingConfigDump))
for k, v := range existingConfigDump {
changes[k] = v
}
changes := util.CloneMap(currentClusterConfig.Dump())

err = doApi10UpdateTriggers(d, nil, changes, nodeConfig, currentClusterConfig)
if err != nil {
Expand Down
5 changes: 1 addition & 4 deletions cmd/incusd/networks.go
Original file line number Diff line number Diff line change
Expand Up @@ -716,10 +716,7 @@ func networksPostCluster(ctx context.Context, s *state.State, projectName string

// Clone the network config for this node so we don't modify it and potentially end up sending
// this node's config to another node.
nodeConfig := make(map[string]string, len(netConfig))
for k, v := range netConfig {
nodeConfig[k] = v
}
nodeConfig := util.CloneMap(netConfig)

// Merge node specific config items into global config.
for key, value := range nodeConfigs[server.Environment.ServerName] {
Expand Down
6 changes: 2 additions & 4 deletions cmd/incusd/storage_pools.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/lxc/incus/v6/internal/version"
"github.com/lxc/incus/v6/shared/api"
"github.com/lxc/incus/v6/shared/logger"
"github.com/lxc/incus/v6/shared/util"
)

// Lock to prevent concurent storage pools creation.
Expand Down Expand Up @@ -554,10 +555,7 @@ func storagePoolsPostCluster(ctx context.Context, s *state.State, pool *api.Stor

// Clone fresh node config so we don't modify req.Config with this node's specific config which
// could result in it being sent to other nodes later.
nodeReq.Config = make(map[string]string, len(req.Config))
for k, v := range req.Config {
nodeReq.Config[k] = v
}
nodeReq.Config = util.CloneMap(req.Config)

// Merge node specific config items into global config.
for key, value := range configs[server.Environment.ServerName] {
Expand Down
9 changes: 2 additions & 7 deletions internal/server/device/config/devices.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,15 @@ import (
"strings"

"github.com/lxc/incus/v6/shared/api"
"github.com/lxc/incus/v6/shared/util"
)

// Device represents an instance device.
type Device map[string]string

// Clone returns a copy of the Device.
func (device Device) Clone() Device {
copy := make(map[string]string, len(device))

for k, v := range device {
copy[k] = v
}

return copy
return util.CloneMap(device)
}

// Validate accepts a map of field/validation functions to run against the device's config.
Expand Down
8 changes: 2 additions & 6 deletions internal/server/operations/operations.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/lxc/incus/v6/shared/api"
"github.com/lxc/incus/v6/shared/cancel"
"github.com/lxc/incus/v6/shared/logger"
"github.com/lxc/incus/v6/shared/util"
)

var debug bool
Expand Down Expand Up @@ -66,12 +67,7 @@ func Clone() map[string]*Operation {
operationsLock.Lock()
defer operationsLock.Unlock()

localOperations := make(map[string]*Operation, len(operations))
for k, v := range operations {
localOperations[k] = v
}

return localOperations
return util.CloneMap(operations)
}

// OperationGetInternal returns the operation with the given id. It returns an
Expand Down
25 changes: 5 additions & 20 deletions internal/server/storage/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -4032,10 +4032,7 @@ func (b *backend) ImportBucket(projectName string, poolVol *backupConfig.Config,
defer revert.Fail()

// Copy bucket config from backup file if present (so BucketDBCreate can safely modify the copy if needed).
bucketConfig := make(map[string]string, len(poolVol.Bucket.Config))
for k, v := range poolVol.Bucket.Config {
bucketConfig[k] = v
}
bucketConfig := util.CloneMap(poolVol.Bucket.Config)

bucket := &api.StorageBucketsPost{
Name: poolVol.Bucket.Name,
Expand Down Expand Up @@ -5810,10 +5807,7 @@ func (b *backend) ImportCustomVolume(projectName string, poolVol *backupConfig.C
defer revert.Fail()

// Copy volume config from backup file if present (so VolumeDBCreate can safely modify the copy if needed).
volumeConfig := make(map[string]string, len(poolVol.Volume.Config))
for k, v := range poolVol.Volume.Config {
volumeConfig[k] = v
}
volumeConfig := util.CloneMap(poolVol.Volume.Config)

// Validate config and create database entry for restored storage volume.
err := VolumeDBCreate(b, projectName, poolVol.Volume.Name, poolVol.Volume.Description, drivers.VolumeTypeCustom, false, volumeConfig, poolVol.Volume.CreatedAt, time.Time{}, drivers.ContentType(poolVol.Volume.ContentType), false, true)
Expand All @@ -5829,10 +5823,7 @@ func (b *backend) ImportCustomVolume(projectName string, poolVol *backupConfig.C

// Copy volume config from backup file if present
// (so VolumeDBCreate can safely modify the copy if needed).
snapVolumeConfig := make(map[string]string, len(poolVolSnap.Config))
for k, v := range poolVolSnap.Config {
snapVolumeConfig[k] = v
}
snapVolumeConfig := util.CloneMap(poolVolSnap.Config)

// Validate config and create database entry for restored storage volume.
err = VolumeDBCreate(b, projectName, fullSnapName, poolVolSnap.Description, drivers.VolumeTypeCustom, true, snapVolumeConfig, poolVolSnap.CreatedAt, time.Time{}, drivers.ContentType(poolVolSnap.ContentType), false, true)
Expand Down Expand Up @@ -6881,10 +6872,7 @@ func (b *backend) ImportInstance(inst instance.Instance, poolVol *backupConfig.C
// Copy volume config from backup file config if present,
// so VolumeDBCreate can safely modify the copy if needed.
if poolVol.Volume != nil {
volumeConfig = make(map[string]string, len(poolVol.Volume.Config))
for k, v := range poolVol.Volume.Config {
volumeConfig[k] = v
}
volumeConfig = util.CloneMap(poolVol.Volume.Config)

if !poolVol.Volume.CreatedAt.IsZero() {
creationDate = poolVol.Volume.CreatedAt
Expand All @@ -6906,10 +6894,7 @@ func (b *backend) ImportInstance(inst instance.Instance, poolVol *backupConfig.C

// Copy volume config from backup file if present,
// so VolumeDBCreate can safely modify the copy if needed.
snapVolumeConfig := make(map[string]string, len(poolVolSnap.Config))
for k, v := range poolVolSnap.Config {
snapVolumeConfig[k] = v
}
snapVolumeConfig := util.CloneMap(poolVolSnap.Config)

// Validate config and create database entry for recovered storage volume.
err = VolumeDBCreate(b, inst.Project().Name, fullSnapName, poolVolSnap.Description, volType, true, snapVolumeConfig, poolVolSnap.CreatedAt, time.Time{}, contentType, false, true)
Expand Down
7 changes: 1 addition & 6 deletions internal/server/storage/drivers/driver_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,7 @@ func (d *common) Logger() logger.Logger {

// Config returns the storage pool config (as a copy, so not modifiable).
func (d *common) Config() map[string]string {
confCopy := make(map[string]string, len(d.config))
for k, v := range d.config {
confCopy[k] = v
}

return confCopy
return util.CloneMap(d.config)
}

// ApplyPatch looks for a suitable patch and runs it.
Expand Down
10 changes: 2 additions & 8 deletions internal/server/storage/drivers/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -577,16 +577,10 @@ func (v *Volume) SetHasSource(hasSource bool) {
// Clone returns a copy of the volume.
func (v Volume) Clone() Volume {
// Copy the config map to avoid internal modifications affecting external state.
newConfig := make(map[string]string, len(v.config))
for k, v := range v.config {
newConfig[k] = v
}
newConfig := util.CloneMap(v.config)

// Copy the pool config map to avoid internal modifications affecting external state.
newPoolConfig := make(map[string]string, len(v.poolConfig))
for k, v := range v.poolConfig {
newPoolConfig[k] = v
}
newPoolConfig := util.CloneMap(v.poolConfig)

return NewVolume(v.driver, v.pool, v.volType, v.contentType, v.name, newConfig, newPoolConfig)
}
9 changes: 3 additions & 6 deletions internal/server/util/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"slices"
"sort"
"strings"

"github.com/lxc/incus/v6/shared/util"
)

// CompareConfigs compares two config maps and returns an error if they differ.
Expand Down Expand Up @@ -52,10 +54,5 @@ func CompareConfigs(config1, config2 map[string]string, exclude []string) error

// CopyConfig creates a new map with a copy of the given config.
func CopyConfig(config map[string]string) map[string]string {
copy := make(map[string]string, len(config))
for key, value := range config {
copy[key] = value
}

return copy
return util.CloneMap(config)
}
12 changes: 5 additions & 7 deletions shared/cliconfig/default.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
package cliconfig

import (
"github.com/lxc/incus/v6/shared/util"
)

// LocalRemote is the default local remote (over the unix socket).
var LocalRemote = Remote{
Addr: "unix://",
Expand Down Expand Up @@ -28,14 +32,8 @@ var DefaultRemotes = map[string]Remote{

// DefaultConfig returns the default configuration.
func DefaultConfig() *Config {
// Duplicate remotes from DefaultRemotes.
defaultRoutes := make(map[string]Remote, len(DefaultRemotes))
for k, v := range DefaultRemotes {
defaultRoutes[k] = v
}

return &Config{
Remotes: defaultRoutes,
Remotes: util.CloneMap(DefaultRemotes),
Aliases: make(map[string]string),
DefaultRemote: "local",
}
Expand Down
15 changes: 15 additions & 0 deletions shared/util/map.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package util

import (
"maps"
)

// CloneMap returns a copy of m. This is a shallow clone: the new keys
// and values are set using ordinary assignment.
func CloneMap[M ~map[K]V, K comparable, V any](m M) M {
if m == nil {
return make(map[K]V)
}

return maps.Clone(m)
}
Loading