[bug] Subshell error handling in build.func is incorrect

[cricalix]

Not sure if this is a discussion as such, but the Issues filtering only seems to accept issues with specific scripts, not a general problem.

The syntax bash -c "$(wget -qLO - https://www....com/path/to/file)" || exit will not exit correctly if the wget (or any other command in the subshell) fails.

There are a few options to correct this:

1. Put an explicit echo exit $? in the subcommand; this ensures bash -c has something to execute instead of empty string, and the error value will be propagated to the || exit error handling, aborting the script.

2. Expand the code from a one-liner to something like

if ! wget -qLO - http://some.website/file.txt > /tmp/script.sh; then
    echo "Error: Failed to download the script." >&2
    exit 1
fi

if ! bash /tmp/script.sh; then
    echo "Error: Script execution failed." >&2
    exit 1
fi

3. Pick a middle-ground between code golf and fully verbose

wget -qL http://some.website/file.txt -O /tmp/file.txt || exit 1
bash /tmp/file.txt || exit 1

Happy to put up a PR for any of those approaches; the first will work in both the build.func execution, and the execution it does via the lxc CLI. Options 2 and 3 work for the inline call to fetch create_lxc.sh, but would be trickier for the lxc call.

There's also a snag when Proxmox upgrades to a Debian version that defaults to wget2 over the existing wget (which is what Fedora did in v40) - wget2 doesn't exit with an error code greater than 0 if it cannot handle the protocol of the URL, while wget1 does. This pushes me towards option 2, because the wget command is not going to fail, so the size of the downloaded shell script will need to be checked to see if it's zero bytes (the -q for wget will hide any failure messages), and exit if it is. Alternately, all cases of wget should be swapped out for curl, which handles All The Protocols (for some value of All), and then the basic error handling fix is enough.

Demo

# echo something to prove the file was called (it never will be)
root@pve:~# cat i.sh
echo "Hello, World!"

# Demonstrate that wget does not like file:/// URLs, but the `||` handler isn't called
root@pve:~# bash -c "$(wget -LO - file:///root/t/ProxmoxVE/i.sh)" || echo "failed"
file:///root/t/ProxmoxVE/i.sh: Unsupported scheme ‘file’.
root@pve:~# echo $?
0

# Now do it quietly, so everything looks file, the way it's called in build.func
root@pve:~# bash -c "$(wget -qLO - file:///root/t/ProxmoxVE/i.sh)" || echo "failed"
root@pve:~# echo $?
0

# Demonstrate that the inner error handler can be called, and must emit valid shell.. and that the outer error handler doesn't fire
root@pve:~# bash -c "$(wget -qLO - file:///root/t/ProxmoxVE/i.sh || echo 'echo inner failed')" || echo "failed"
inner failed
root@pve:~# echo $?
0

# Inner handle "raises" the exit code of the wget command, and the outer handler sees it
root@pve:~# bash -c "$(wget -qLO - file:///root/t/ProxmoxVE/i.sh || echo exit $?)" || echo "failed $?"
failed 1

[MickLesk]

currently we dont support subshell, only the bash commands like yet.

I also see no reason why you should need subshells? I also see absolutely no need or necessity in your explanations.

[cricalix]

The existing $() acts like a subshell. Any failure inside of it to fetch the remote file is swallowed, and the exit call will not fire. The subsequent attempt to access the generated lxc config file will fail, because the file cannot be found. It is not immediately obvious why the file cannot be found.

I'm merely suggesting that the remote fetch should have an inner || handler that produces an exit code that bash -c can execute, causing the outer || exit to actually work.

[MickLesk]

Still see no reason or problem. If you see it as a reason or Issue, do a proper PR