From 0c716ff0ac0dbe88a747a4ddda6d7e737ba42927 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Oct 2022 02:31:03 +0200 Subject: [PATCH] Simplify the interface of GetCacheMount and getCacheMount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It can return at most one lock, so don't return an array. Should not change behavior right now, but it will simplify cleanup. Signed-off-by: Miloslav Trmač --- internal/parse/parse.go | 54 +++++++++++++++++++++-------------------- run_common.go | 4 ++- run_freebsd.go | 2 +- run_linux.go | 11 +++++---- 4 files changed, 38 insertions(+), 33 deletions(-) diff --git a/internal/parse/parse.go b/internal/parse/parse.go index f0a851a5187..6c2cb4234b6 100644 --- a/internal/parse/parse.go +++ b/internal/parse/parse.go @@ -185,11 +185,10 @@ func GetBindMount(ctx *types.SystemContext, args []string, contextDir string, st // GetCacheMount parses a single cache mount entry from the --mount flag. // -// The caller should eventually unlock all of the returned lockfile.Locker locks, even if this function fails. -func GetCacheMount(args []string, store storage.Store, imageMountLabel string, additionalMountPoints map[string]internal.StageMountDetails) (specs.Mount, []lockfile.Locker, error) { +// The caller should eventually unlock returned lockfile.Locker, if it is non-nil, even if this function fails. +func GetCacheMount(args []string, store storage.Store, imageMountLabel string, additionalMountPoints map[string]internal.StageMountDetails) (specs.Mount, lockfile.Locker, error) { var err error var mode uint64 - targetLocks := make([]lockfile.Locker, 0) var ( setDest bool setShared bool @@ -237,59 +236,59 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a sharing = kv[1] case "bind-propagation": if len(kv) == 1 { - return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) + return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) } newMount.Options = append(newMount.Options, kv[1]) case "id": if len(kv) == 1 { - return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) + return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) } id = kv[1] case "from": if len(kv) == 1 { - return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) + return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) } fromStage = kv[1] case "target", "dst", "destination": if len(kv) == 1 { - return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) + return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) } if err := parse.ValidateVolumeCtrDir(kv[1]); err != nil { - return newMount, targetLocks, err + return newMount, nil, err } newMount.Destination = kv[1] setDest = true case "src", "source": if len(kv) == 1 { - return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) + return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) } newMount.Source = kv[1] case "mode": if len(kv) == 1 { - return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) + return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) } mode, err = strconv.ParseUint(kv[1], 8, 32) if err != nil { - return newMount, targetLocks, fmt.Errorf("unable to parse cache mode: %w", err) + return newMount, nil, fmt.Errorf("unable to parse cache mode: %w", err) } case "uid": if len(kv) == 1 { - return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) + return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) } uid, err = strconv.Atoi(kv[1]) if err != nil { - return newMount, targetLocks, fmt.Errorf("unable to parse cache uid: %w", err) + return newMount, nil, fmt.Errorf("unable to parse cache uid: %w", err) } case "gid": if len(kv) == 1 { - return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) + return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg) } gid, err = strconv.Atoi(kv[1]) if err != nil { - return newMount, targetLocks, fmt.Errorf("unable to parse cache gid: %w", err) + return newMount, nil, fmt.Errorf("unable to parse cache gid: %w", err) } default: - return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadMntOption) + return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadMntOption) } } @@ -300,7 +299,7 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a } if !setDest { - return newMount, targetLocks, errBadVolDest + return newMount, nil, errBadVolDest } if fromStage != "" { @@ -317,7 +316,7 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a // Cache does not supports using image so if not stage found // return with error if mountPoint == "" { - return newMount, targetLocks, fmt.Errorf("no stage found with name %s", fromStage) + return newMount, nil, fmt.Errorf("no stage found with name %s", fromStage) } // path should be /contextDir/specified path newMount.Source = filepath.Join(mountPoint, filepath.Clean(string(filepath.Separator)+newMount.Source)) @@ -333,7 +332,7 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a // create cache on host if not present err = os.MkdirAll(cacheParent, os.FileMode(0755)) if err != nil { - return newMount, targetLocks, fmt.Errorf("unable to create build cache directory: %w", err) + return newMount, nil, fmt.Errorf("unable to create build cache directory: %w", err) } if id != "" { @@ -348,26 +347,27 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a //buildkit parity: change uid and gid if specified otheriwise keep `0` err = idtools.MkdirAllAndChownNew(newMount.Source, os.FileMode(mode), idPair) if err != nil { - return newMount, targetLocks, fmt.Errorf("unable to change uid,gid of cache directory: %w", err) + return newMount, nil, fmt.Errorf("unable to change uid,gid of cache directory: %w", err) } } + var targetLock lockfile.Locker // = nil switch sharing { case "locked": // lock parent cache lockfile, err := lockfile.GetLockfile(filepath.Join(newMount.Source, BuildahCacheLockfile)) if err != nil { - return newMount, targetLocks, fmt.Errorf("unable to acquire lock when sharing mode is locked: %w", err) + return newMount, targetLock, fmt.Errorf("unable to acquire lock when sharing mode is locked: %w", err) } // Will be unlocked after the RUN step is executed. lockfile.Lock() - targetLocks = append(targetLocks, lockfile) + targetLock = lockfile case "shared": // do nothing since default is `shared` break default: // error out for unknown values - return newMount, targetLocks, fmt.Errorf("unrecognized value %q for field `sharing`: %w", sharing, err) + return newMount, targetLock, fmt.Errorf("unrecognized value %q for field `sharing`: %w", sharing, err) } // buildkit parity: default sharing should be shared @@ -385,11 +385,11 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a opts, err := parse.ValidateVolumeOpts(newMount.Options) if err != nil { - return newMount, targetLocks, err + return newMount, targetLock, err } newMount.Options = opts - return newMount, targetLocks, nil + return newMount, targetLock, nil } // ValidateVolumeMountHostDir validates the host path of buildah --volume @@ -547,7 +547,9 @@ func getMounts(ctx *types.SystemContext, store storage.Store, mounts []string, c mountedImages = append(mountedImages, image) case TypeCache: mount, tl, err := GetCacheMount(tokens, store, "", nil) - targetLocks = append(targetLocks, tl...) + if tl != nil { + targetLocks = append(targetLocks, tl) + } if err != nil { return nil, mountedImages, targetLocks, err } diff --git a/run_common.go b/run_common.go index 4221238ab56..b4129b63bf9 100644 --- a/run_common.go +++ b/run_common.go @@ -1519,7 +1519,9 @@ func (b *Builder) runSetupRunMounts(mounts []string, sources runMountInfo, idMap } finalMounts = append(finalMounts, *mount) mountTargets = append(mountTargets, mount.Destination) - targetLocks = append(targetLocks, tl...) + if tl != nil { + targetLocks = append(targetLocks, tl) + } default: return nil, nil, fmt.Errorf("invalid mount type %q", mountType) } diff --git a/run_freebsd.go b/run_freebsd.go index 58f9985b014..3a3eb39ba60 100644 --- a/run_freebsd.go +++ b/run_freebsd.go @@ -308,7 +308,7 @@ func setupSpecialMountSpecChanges(spec *spec.Spec, shmSize string) ([]specs.Moun return spec.Mounts, nil } -func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, []lockfile.Locker, error) { +func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, lockfile.Locker, error) { return nil, nil, errors.New("cache mounts not supported on freebsd") } diff --git a/run_linux.go b/run_linux.go index 2c715219c20..a89f8c1774c 100644 --- a/run_linux.go +++ b/run_linux.go @@ -1196,18 +1196,19 @@ func checkIdsGreaterThan5(ids []spec.LinuxIDMapping) bool { return false } -func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, []lockfile.Locker, error) { +// The caller should eventually unlock returned lockfile.Locker, if it is non-nil, even if this function fails. +func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, lockfile.Locker, error) { var optionMounts []specs.Mount - mount, targetLocks, err := internalParse.GetCacheMount(tokens, b.store, b.MountLabel, stageMountPoints) + mount, targetLock, err := internalParse.GetCacheMount(tokens, b.store, b.MountLabel, stageMountPoints) if err != nil { - return nil, targetLocks, err + return nil, targetLock, err } optionMounts = append(optionMounts, mount) volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps) if err != nil { - return nil, targetLocks, err + return nil, targetLock, err } - return &volumes[0], targetLocks, nil + return &volumes[0], targetLock, nil } // setPdeathsig sets a parent-death signal for the process