Fix command encoding by using shlex/eval

[vxgmichel]

At the moment the command provided to the container is not properly encoded, which causes commands that use quotes to fail unexpectedly:

± xcp-ng-dev container run 8.3 -- ls "my file.txt"
[...]
ls: cannot access my: No such file or directory
ls: cannot access file.txt: No such file or directory

This PR fixes this:

± xcp-ng-dev container run 8.3 -- ls "my file.txt"
[...]
ls: cannot access my file.txt: No such file or directory

Crucially, this allows for running shell commands:

± xcp-ng-dev container run 8.3 -- bash -c "echo 1 && echo 2"
[...]
1
2

[vxgmichel]

As an alternative, it's also possible to get rid of encoding altogether by passing the command to init-container.sh directly:

diff --git a/container/files/init-container.sh b/container/files/init-container.sh
index 7272a68..6c8b231 100755
--- a/container/files/init-container.sh
+++ b/container/files/init-container.sh
@@ -115,8 +115,8 @@ if [ -n "$BUILD_LOCAL" ]; then
             fi
         fi
     )
-elif [ -n "$COMMAND" ]; then
-    $COMMAND
+elif [ $# -gt 0 ]; then
+    exec "$@"
 else
     /bin/bash --login || true
 fi
diff --git a/src/xcp_ng_dev/cli.py b/src/xcp_ng_dev/cli.py
index 08edaee..9d9a461 100755
--- a/src/xcp_ng_dev/cli.py
+++ b/src/xcp_ng_dev/cli.py
@@ -165,6 +165,7 @@ def buildparser():
 
 def container(args):
     docker_args = [RUNNER, "run"]
+    command = []
 
     if is_podman(RUNNER):
         # With podman we use the `--userns` option to map the builder user to the user on the system.
@@ -273,7 +274,7 @@ def container(args):
             print(f"Building directory {build_dir}", file=sys.stderr)
 
         case 'run':
-            docker_args += ["-e", "COMMAND=%s" % ' '.join(args.command)]
+            command = args.command
 
         case 'shell':
             wants_interactive = True
@@ -283,7 +284,9 @@ def container(args):
 
     # exec "docker run"
     docker_args += [f"{CONTAINER_PREFIX}:{args.container_version}",
-                    "/usr/local/bin/init-container.sh"]
+                    "/usr/local/bin/init-container.sh",
+                    *command,
+                    ]
     print("Launching docker with args %s" % docker_args, file=sys.stderr)
     return subprocess.call(docker_args)

[ydirson]

As an alternative, it's also possible to get rid of encoding altogether by passing the command to init-container.sh directly:

Both solutions are OK with me. Note that both change the interface between the script and the container, and --pull=always will likely be needed by everyone after updating to the latest script version.

The alternative using init-container parameters likely makes more sense, and actually a number of things passed through variables would make sense to migrate to optional or mandatory parameters. This would help to catch interface changes such as this one in the future. And to prepare for this, it would make sense to use a -- separator to introduce this command, so a future init-container consuming option arguments does no attempt to interpret those meant for the command.

[glehmann]

Why exec here? It wasn't there before

[vxgmichel]

It's considered good practice.
In particular, exec causes the command to replace the init-container.sh process so that it inherits from PID 1:

± xcp-ng-dev container run 8.3 ps
[...]
    PID TTY          TIME CMD
      1 pts/0    00:00:00 ps

I don't mind removing it tho.

[glehmann]

Oh, I thought it might be related to the eval usage.
It might be worth adding a note in the commit message :)

[vxgmichel]

It might be worth adding a note in the commit message

Added: 0d93542

[ydirson]

Actually, adding exec will be break --no-exit behaviour, as the script trap handler will be not be here any more to run a shell on failure.

[vxgmichel]

Removed: e644824
Yes I completely missed the trap "/bin/bash --login" EXIT at the beginning of the file, oopsie 😬

[glehmann]

Note that both change the interface between the script and the container, and --pull=always will likely be needed by everyone after updating to the latest script version.

Wouldn't it work with the primary version? For the simple cases, I think shlex would just generate a space-separated string as before, so we should stay compatible.

>>> import shlex
>>> shlex.join(['ls', '/tmp'])
'ls /tmp'

[vxgmichel]

@ydirson

Both solutions are OK with me. Note that both change the interface between the script and the container, and --pull=always will likely be needed by everyone after updating to the latest script version.

@glehmann is right, the approach used in this PR remains compatible both ways:

• vml/fix-command-encoding container with master cli.py: it works the same as before
• master container with vml/fix-command-encoding container: it works the same as before

So both part of the fix are required, but getting only one of them should not break previous usage.

[ydirson]

Actually, adding exec will be break --no-exit behaviour, as the script trap handler will be not be here any more to run a shell on failure.

[vxgmichel]

@ydirson

Actually, adding exec will be break --no-exit behaviour, as the script trap handler will be not be here any more to run a shell on failure.

Great catch I completely missed the trap handler, I should have been more careful.

[ydirson]

lgtm