From acdaa5372eb9e744ef417e43a6102139bdf069a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 4 Mar 2026 00:14:37 +0100 Subject: [PATCH 1/3] Don't use strings.Split(fmt.Sprintf("--a b ...", ...), " ") MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we have the _precise_ knowledge of where the parameter boundaries are, and an API that allows us to express that, just _do that_ instead of completely unnecessarily worrying about spaces in parameter values. Also, this allows us to format the code to make the option and value correspondence much easier to see. Signed-off-by: Miloslav Trmač --- pkg/domain/utils/scp.go | 22 ++++++++++++---------- test/e2e/common_test.go | 16 ++++++++++++---- test/e2e/libpod_suite_remote_test.go | 12 ++++++++++-- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/pkg/domain/utils/scp.go b/pkg/domain/utils/scp.go index 6c2a8406d5..f23b9400ac 100644 --- a/pkg/domain/utils/scp.go +++ b/pkg/domain/utils/scp.go @@ -333,18 +333,20 @@ func ExecPodman(dest entities.ScpTransferImageOptions, podman string, command [] // CreateCommands forms the podman save and load commands used by SCP func CreateCommands(source entities.ScpTransferImageOptions, dest entities.ScpTransferImageOptions, opts entities.ScpCreateCommandsOptions) ([]string, []string) { - var parentString string - quiet := "" - if source.Quiet { - quiet = "-q " - } + loadCmd := []string{opts.Podman} + saveCmd := []string{opts.Podman} if len(opts.ParentFlags) > 0 { - parentString = strings.Join(opts.ParentFlags, " ") + " " // if there are parent args, an extra space needs to be added - } else { - parentString = strings.Join(opts.ParentFlags, " ") + loadCmd = append(loadCmd, opts.ParentFlags...) + saveCmd = append(saveCmd, opts.ParentFlags...) } - loadCmd := strings.Split(fmt.Sprintf("%s %sload %s--input %s", opts.Podman, parentString, quiet, dest.File), " ") - saveCmd := strings.Split(fmt.Sprintf("%s %vsave %s--output %s %s", opts.Podman, parentString, quiet, source.File, source.Image), " ") + loadCmd = append(loadCmd, "load") + saveCmd = append(saveCmd, "save") + if source.Quiet { + loadCmd = append(loadCmd, "-q") + saveCmd = append(saveCmd, "-q") + } + loadCmd = append(loadCmd, "--input", dest.File) + saveCmd = append(saveCmd, "--output", source.File, source.Image) return saveCmd, loadCmd } diff --git a/test/e2e/common_test.go b/test/e2e/common_test.go index 45fb0c157c..abf985984e 100644 --- a/test/e2e/common_test.go +++ b/test/e2e/common_test.go @@ -1314,9 +1314,9 @@ func (p *PodmanTestIntegration) makeOptions(args []string, options PodmanExecOpt return args } - var debug string + podmanOptions := []string{} if _, ok := os.LookupEnv("E2E_DEBUG"); ok { - debug = "--log-level=debug --syslog=true " + podmanOptions = append(podmanOptions, "--log-level=debug", "--syslog=true") } eventsType := "file" @@ -1324,8 +1324,16 @@ func (p *PodmanTestIntegration) makeOptions(args []string, options PodmanExecOpt eventsType = "none" } - podmanOptions := strings.Split(fmt.Sprintf("%s--root %s --runroot %s --runtime %s --conmon %s --network-config-dir %s --cgroup-manager %s --tmpdir %s --events-backend %s", - debug, p.Root, p.RunRoot, p.OCIRuntime, p.ConmonBinary, p.NetworkConfigDir, p.CgroupManager, p.TmpDir, eventsType), " ") + podmanOptions = append(podmanOptions, + "--root", p.Root, + "--runroot", p.RunRoot, + "--runtime", p.OCIRuntime, + "--conmon", p.ConmonBinary, + "--network-config-dir", p.NetworkConfigDir, + "--cgroup-manager", p.CgroupManager, + "--tmpdir", p.TmpDir, + "--events-backend", eventsType, + ) podmanOptions = append(podmanOptions, strings.Split(p.StorageOptions, " ")...) if !options.NoCache { diff --git a/test/e2e/libpod_suite_remote_test.go b/test/e2e/libpod_suite_remote_test.go index c5eea820de..74281c0214 100644 --- a/test/e2e/libpod_suite_remote_test.go +++ b/test/e2e/libpod_suite_remote_test.go @@ -122,8 +122,16 @@ func (p *PodmanTestIntegration) StopRemoteService() { // getRemoteOptions assembles all the podman main options func getRemoteOptions(p *PodmanTestIntegration, args []string) []string { networkDir := p.NetworkConfigDir - podmanOptions := strings.Split(fmt.Sprintf("--root %s --runroot %s --runtime %s --conmon %s --network-config-dir %s --cgroup-manager %s --tmpdir %s --events-backend %s", - p.Root, p.RunRoot, p.OCIRuntime, p.ConmonBinary, networkDir, p.CgroupManager, p.TmpDir, "file"), " ") + podmanOptions := []string{ + "--root", p.Root, + "--runroot", p.RunRoot, + "--runtime", p.OCIRuntime, + "--conmon", p.ConmonBinary, + "--network-config-dir", networkDir, + "--cgroup-manager", p.CgroupManager, + "--tmpdir", p.TmpDir, + "--events-backend", "file", + } podmanOptions = append(podmanOptions, strings.Split(p.StorageOptions, " ")...) podmanOptions = append(podmanOptions, args...) From a725f55ff1b0f6b5d241d45edfb65570e296d00d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 4 Mar 2026 00:20:43 +0100 Subject: [PATCH 2/3] Make CreateCommands and ScpCreateCommandsOptions private MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They are entirely private, and the type has no reason to exist in the API definitions. Signed-off-by: Miloslav Trmač --- pkg/domain/entities/scp.go | 7 ------- pkg/domain/utils/scp.go | 29 ++++++++++++++++++----------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/pkg/domain/entities/scp.go b/pkg/domain/entities/scp.go index d15a7118f6..7fb1ba851e 100644 --- a/pkg/domain/entities/scp.go +++ b/pkg/domain/entities/scp.go @@ -88,10 +88,3 @@ type ScpSaveToRemoteOptions struct { } type ScpSaveToRemoteReport struct{} - -type ScpCreateCommandsOptions struct { - // ParentFlags are the arguments to apply to the parent podman command when called via ssh - ParentFlags []string - // Podman is the path to the local podman executable - Podman string -} diff --git a/pkg/domain/utils/scp.go b/pkg/domain/utils/scp.go index f23b9400ac..3c5a640a23 100644 --- a/pkg/domain/utils/scp.go +++ b/pkg/domain/utils/scp.go @@ -88,10 +88,10 @@ func ExecuteTransfer(src, dst string, opts entities.ScpExecuteTransferOptions) ( return nil, err } - createCommandOpts := entities.ScpCreateCommandsOptions{} - createCommandOpts.ParentFlags = opts.ParentFlags - createCommandOpts.Podman = podman - saveCmd, loadCmd := CreateCommands(source, dest, createCommandOpts) + createCommandOpts := scpCreateCommandsOptions{} + createCommandOpts.parentFlags = opts.ParentFlags + createCommandOpts.podman = podman + saveCmd, loadCmd := createCommands(source, dest, createCommandOpts) switch { case source.Remote: // if we want to load FROM the remote, dest can either be local or remote in this case @@ -331,13 +331,20 @@ func ExecPodman(dest entities.ScpTransferImageOptions, podman string, command [] return "", cmd.Run() } -// CreateCommands forms the podman save and load commands used by SCP -func CreateCommands(source entities.ScpTransferImageOptions, dest entities.ScpTransferImageOptions, opts entities.ScpCreateCommandsOptions) ([]string, []string) { - loadCmd := []string{opts.Podman} - saveCmd := []string{opts.Podman} - if len(opts.ParentFlags) > 0 { - loadCmd = append(loadCmd, opts.ParentFlags...) - saveCmd = append(saveCmd, opts.ParentFlags...) +type scpCreateCommandsOptions struct { + // parentFlags are the arguments to apply to the parent podman command when called via ssh + parentFlags []string + // podman is the path to the local podman executable + podman string +} + +// createCommands forms the podman save and load commands used by SCP +func createCommands(source entities.ScpTransferImageOptions, dest entities.ScpTransferImageOptions, opts scpCreateCommandsOptions) ([]string, []string) { + loadCmd := []string{opts.podman} + saveCmd := []string{opts.podman} + if len(opts.parentFlags) > 0 { + loadCmd = append(loadCmd, opts.parentFlags...) + saveCmd = append(saveCmd, opts.parentFlags...) } loadCmd = append(loadCmd, "load") saveCmd = append(saveCmd, "save") From b2d381c7a28804408184a9e8c7896886f63bf06e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 4 Mar 2026 00:26:10 +0100 Subject: [PATCH 3/3] Inline createCommands into the caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is exactly one caller, with two code paths, and each only needs _half_ of the function - and they really only share the parentFlags and Quiet logic. It's easier to do things directly. Signed-off-by: Miloslav Trmač --- pkg/domain/utils/scp.go | 45 +++++++++++++---------------------------- 1 file changed, 14 insertions(+), 31 deletions(-) diff --git a/pkg/domain/utils/scp.go b/pkg/domain/utils/scp.go index 3c5a640a23..370d159c4a 100644 --- a/pkg/domain/utils/scp.go +++ b/pkg/domain/utils/scp.go @@ -88,11 +88,6 @@ func ExecuteTransfer(src, dst string, opts entities.ScpExecuteTransferOptions) ( return nil, err } - createCommandOpts := scpCreateCommandsOptions{} - createCommandOpts.parentFlags = opts.ParentFlags - createCommandOpts.podman = podman - saveCmd, loadCmd := createCommands(source, dest, createCommandOpts) - switch { case source.Remote: // if we want to load FROM the remote, dest can either be local or remote in this case saveToRemoteOpts := entities.ScpSaveToRemoteOptions{} @@ -126,6 +121,13 @@ func ExecuteTransfer(src, dst string, opts entities.ScpExecuteTransferOptions) ( } break } + loadCmd := []string{podman} + loadCmd = append(loadCmd, opts.ParentFlags...) + loadCmd = append(loadCmd, "load") + if source.Quiet { + loadCmd = append(loadCmd, "-q") + } + loadCmd = append(loadCmd, "--input", dest.File) id, err := ExecPodman(dest, podman, loadCmd) if err != nil { return nil, err @@ -134,6 +136,13 @@ func ExecuteTransfer(src, dst string, opts entities.ScpExecuteTransferOptions) ( loadReport.Names = append(loadReport.Names, id) } case dest.Remote: // remote host load, implies source is local + saveCmd := []string{podman} + saveCmd = append(saveCmd, opts.ParentFlags...) + saveCmd = append(saveCmd, "save") + if source.Quiet { + saveCmd = append(saveCmd, "-q") + } + saveCmd = append(saveCmd, "--output", source.File, source.Image) _, err = ExecPodman(dest, podman, saveCmd) if err != nil { return nil, err @@ -331,32 +340,6 @@ func ExecPodman(dest entities.ScpTransferImageOptions, podman string, command [] return "", cmd.Run() } -type scpCreateCommandsOptions struct { - // parentFlags are the arguments to apply to the parent podman command when called via ssh - parentFlags []string - // podman is the path to the local podman executable - podman string -} - -// createCommands forms the podman save and load commands used by SCP -func createCommands(source entities.ScpTransferImageOptions, dest entities.ScpTransferImageOptions, opts scpCreateCommandsOptions) ([]string, []string) { - loadCmd := []string{opts.podman} - saveCmd := []string{opts.podman} - if len(opts.parentFlags) > 0 { - loadCmd = append(loadCmd, opts.parentFlags...) - saveCmd = append(saveCmd, opts.parentFlags...) - } - loadCmd = append(loadCmd, "load") - saveCmd = append(saveCmd, "save") - if source.Quiet { - loadCmd = append(loadCmd, "-q") - saveCmd = append(saveCmd, "-q") - } - loadCmd = append(loadCmd, "--input", dest.File) - saveCmd = append(saveCmd, "--output", source.File, source.Image) - return saveCmd, loadCmd -} - // parseImageSCPArg returns the valid connection, and source/destination data based off of the information provided by the user // arg is a string containing one of the cli arguments returned is a filled out source/destination options structs as well as a connections array and an error if applicable func ParseImageSCPArg(arg string) (*entities.ScpTransferImageOptions, []string, error) {