pkg/specgenutil: rework volume/mount parsing

Use a helper struct to hold the mounts instead of returning 5+ return
values from the functions. This allows use to easily add more volume
types without having to update all return lines every time in the
future. And 5+ return values are really not readable anymore so this
should make it easier to follow the code.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

pkg/specgenutil/specgen.go

@@ -762,15 +762,15 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions
 
 	// Only add read-only tmpfs mounts in case that we are read-only and the
 	// read-only tmpfs flag has been set.
-	mounts, volumes, overlayVolumes, imageVolumes, err := parseVolumes(rtc, c.Volume, c.Mount, c.TmpFS)
+	containerMounts, err := parseVolumes(rtc, c.Volume, c.Mount, c.TmpFS)
 	if err != nil {
 		return err
 	}
 	if len(s.Mounts) == 0 || len(c.Mount) != 0 {
-		s.Mounts = mounts
+		s.Mounts = containerMounts.mounts
 	}
 	if len(s.Volumes) == 0 || len(c.Volume) != 0 {
-		s.Volumes = volumes
+		s.Volumes = containerMounts.volumes
 	}
 
 	if s.LabelNested != nil && *s.LabelNested {
@@ -785,10 +785,10 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions
 	}
 	// TODO make sure these work in clone
 	if len(s.OverlayVolumes) == 0 {
-		s.OverlayVolumes = overlayVolumes
+		s.OverlayVolumes = containerMounts.overlayVolumes
 	}
 	if len(s.ImageVolumes) == 0 {
-		s.ImageVolumes = imageVolumes
+		s.ImageVolumes = containerMounts.imageVolumes
 	}
 
 	devices := c.Devices

pkg/specgenutil/volumes.go

@@ -22,6 +22,20 @@ var (
 	errNoDest    = errors.New("must set volume destination")
 )
 
+type containerMountSlice struct {
+	mounts         []spec.Mount
+	volumes        []*specgen.NamedVolume
+	overlayVolumes []*specgen.OverlayVolume
+	imageVolumes   []*specgen.ImageVolume
+}
+
+// containerMountMap contains the container mounts with the destination path as map key
+type containerMountMap struct {
+	mounts       map[string]spec.Mount
+	volumes      map[string]*specgen.NamedVolume
+	imageVolumes map[string]*specgen.ImageVolume
+}
+
 type universalMount struct {
 	mount spec.Mount
 	// Used only with Named Volume type mounts
@@ -34,57 +48,57 @@ type universalMount struct {
 // Does not handle image volumes, init, and --volumes-from flags.
 // Can also add tmpfs mounts from read-only tmpfs.
 // TODO: handle options parsing/processing via containers/storage/pkg/mount
-func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) ([]spec.Mount, []*specgen.NamedVolume, []*specgen.OverlayVolume, []*specgen.ImageVolume, error) {
+func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) (*containerMountSlice, error) {
 	// Get mounts from the --mounts flag.
 	// TODO: The runtime config part of this needs to move into pkg/specgen/generate to avoid querying containers.conf on the client.
-	unifiedMounts, unifiedVolumes, unifiedImageVolumes, err := mounts(mountFlag, rtc.Mounts())
+	unifiedContainerMounts, err := mounts(mountFlag, rtc.Mounts())
 	if err != nil {
-		return nil, nil, nil, nil, err
+		return nil, err
 	}
 
 	// Next --volumes flag.
 	volumeMounts, volumeVolumes, overlayVolumes, err := specgen.GenVolumeMounts(volumeFlag)
 	if err != nil {
-		return nil, nil, nil, nil, err
+		return nil, err
 	}
 
 	// Next --tmpfs flag.
 	tmpfsMounts, err := getTmpfsMounts(tmpfsFlag)
 	if err != nil {
-		return nil, nil, nil, nil, err
+		return nil, err
 	}
 
 	// Unify mounts from --mount, --volume, --tmpfs.
 	// Start with --volume.
 	for dest, mount := range volumeMounts {
-		if vol, ok := unifiedMounts[dest]; ok {
+		if vol, ok := unifiedContainerMounts.mounts[dest]; ok {
 			if mount.Source == vol.Source &&
 				specgen.StringSlicesEqual(vol.Options, mount.Options) {
 				continue
 			}
-			return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
+			return nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
 		}
-		unifiedMounts[dest] = mount
+		unifiedContainerMounts.mounts[dest] = mount
 	}
 	for dest, volume := range volumeVolumes {
-		if vol, ok := unifiedVolumes[dest]; ok {
+		if vol, ok := unifiedContainerMounts.volumes[dest]; ok {
 			if volume.Name == vol.Name &&
 				specgen.StringSlicesEqual(vol.Options, volume.Options) {
 				continue
 			}
-			return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
+			return nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
 		}
-		unifiedVolumes[dest] = volume
+		unifiedContainerMounts.volumes[dest] = volume
 	}
 	// Now --tmpfs
 	for dest, tmpfs := range tmpfsMounts {
-		if vol, ok := unifiedMounts[dest]; ok {
+		if vol, ok := unifiedContainerMounts.mounts[dest]; ok {
 			if vol.Type != define.TypeTmpfs {
-				return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
+				return nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
 			}
 			continue
 		}
-		unifiedMounts[dest] = tmpfs
+		unifiedContainerMounts.mounts[dest] = tmpfs
 	}
 
 	// Check for conflicts between named volumes, overlay & image volumes,
@@ -97,53 +111,58 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string)
 		allMounts[dest] = true
 		return nil
 	}
-	for dest := range unifiedMounts {
+	for dest := range unifiedContainerMounts.mounts {
 		if err := testAndSet(dest); err != nil {
-			return nil, nil, nil, nil, err
+			return nil, err
 		}
 	}
-	for dest := range unifiedVolumes {
+	for dest := range unifiedContainerMounts.volumes {
 		if err := testAndSet(dest); err != nil {
-			return nil, nil, nil, nil, err
+			return nil, err
 		}
 	}
 	for dest := range overlayVolumes {
 		if err := testAndSet(dest); err != nil {
-			return nil, nil, nil, nil, err
+			return nil, err
 		}
 	}
-	for dest := range unifiedImageVolumes {
+	for dest := range unifiedContainerMounts.imageVolumes {
 		if err := testAndSet(dest); err != nil {
-			return nil, nil, nil, nil, err
+			return nil, err
 		}
 	}
 
 	// Final step: maps to arrays
-	finalMounts := make([]spec.Mount, 0, len(unifiedMounts))
+	finalMounts := make([]spec.Mount, 0, len(unifiedContainerMounts.mounts))
-	for _, mount := range unifiedMounts {
+	for _, mount := range unifiedContainerMounts.mounts {
 		if mount.Type == define.TypeBind {
 			absSrc, err := specgen.ConvertWinMountPath(mount.Source)
 			if err != nil {
-				return nil, nil, nil, nil, fmt.Errorf("getting absolute path of %s: %w", mount.Source, err)
+				return nil, fmt.Errorf("getting absolute path of %s: %w", mount.Source, err)
 			}
 			mount.Source = absSrc
 		}
 		finalMounts = append(finalMounts, mount)
 	}
-	finalVolumes := make([]*specgen.NamedVolume, 0, len(unifiedVolumes))
+	finalVolumes := make([]*specgen.NamedVolume, 0, len(unifiedContainerMounts.volumes))
-	for _, volume := range unifiedVolumes {
+	for _, volume := range unifiedContainerMounts.volumes {
 		finalVolumes = append(finalVolumes, volume)
 	}
-	finalOverlayVolume := make([]*specgen.OverlayVolume, 0)
+	finalOverlayVolume := make([]*specgen.OverlayVolume, 0, len(overlayVolumes))
 	for _, volume := range overlayVolumes {
 		finalOverlayVolume = append(finalOverlayVolume, volume)
 	}
-	finalImageVolumes := make([]*specgen.ImageVolume, 0, len(unifiedImageVolumes))
+	finalImageVolumes := make([]*specgen.ImageVolume, 0, len(unifiedContainerMounts.imageVolumes))
-	for _, volume := range unifiedImageVolumes {
+	for _, volume := range unifiedContainerMounts.imageVolumes {
 		finalImageVolumes = append(finalImageVolumes, volume)
 	}
 
-	return finalMounts, finalVolumes, finalOverlayVolume, finalImageVolumes, nil
+	return &containerMountSlice{
+		mounts:         finalMounts,
+		volumes:        finalVolumes,
+		overlayVolumes: finalOverlayVolume,
+		imageVolumes:   finalImageVolumes,
+	}, nil
 }
 
 // mounts takes user-provided input from the --mount flag as well as mounts
@@ -151,7 +170,7 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string)
 // podman run --mount type=bind,src=/etc/resolv.conf,target=/etc/resolv.conf ...
 // podman run --mount type=tmpfs,target=/dev/shm ...
 // podman run --mount type=volume,source=test-volume, ...
-func mounts(mountFlag []string, configMounts []string) (map[string]spec.Mount, map[string]*specgen.NamedVolume, map[string]*specgen.ImageVolume, error) {
+func mounts(mountFlag []string, configMounts []string) (*containerMountMap, error) {
 	finalMounts := make(map[string]spec.Mount)
 	finalNamedVolumes := make(map[string]*specgen.NamedVolume)
 	finalImageVolumes := make(map[string]*specgen.ImageVolume)
@@ -246,17 +265,21 @@ func mounts(mountFlag []string, configMounts []string) (map[string]spec.Mount, m
 
 	// Parse mounts passed in from the user
 	if err := parseMounts(mountFlag, false); err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 
 	// If user specified a mount flag that conflicts with a containers.conf flag, then ignore
 	// the duplicate. This means that the parsing of the containers.conf configMounts should always
 	// happen second.
 	if err := parseMounts(configMounts, true); err != nil {
-		return nil, nil, nil, fmt.Errorf("parsing containers.conf mounts: %w", err)
+		return nil, fmt.Errorf("parsing containers.conf mounts: %w", err)
 	}
 
-	return finalMounts, finalNamedVolumes, finalImageVolumes, nil
+	return &containerMountMap{
+		mounts:       finalMounts,
+		volumes:      finalNamedVolumes,
+		imageVolumes: finalImageVolumes,
+	}, nil
 }
 
 func parseMountOptions(mountType string, args []string) (*universalMount, error) {