Skip to content

Update entrypoint for argument quoting #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Jan 22, 2021

Update entrypoint for argument quoting so that word splitting does not lose spaces in arguments.

Also, release memory from shell process. Besides releasing resources (memory), this is very important in docker containers so that signals get routed properly to PID=1.

Q A
Documentation yes/no
Bugfix yes
BC Break yes/no
New Feature yes/no
RFC yes/no
QA yes

Description

Also, release memory from shell process.

Signed-off-by: Elan Ruusamäe <[email protected]>
@Ocramius
Copy link
Member

Ocramius commented Jan 22, 2021

so that word splitting does not lose spaces in arguments.

My bash-fu is weak, but would -v --something-else work then? Wouldn't that break when quoting?

EDIT: the exec bit for signaling is unclear: isn't bin/console.php treated as a child process that signals back recursively?

@Ocramius Ocramius added the Bug Something isn't working label Jan 22, 2021
@Ocramius Ocramius added this to the 1.9.1 milestone Jan 22, 2021
@glensc
Copy link
Contributor Author

glensc commented Jan 22, 2021

without exec ps would look like:

bash-5.1# ps axwuf
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root         1  0.0  0.1   2564  2068 pts/0    S    10:26   0:00 bash
root        28  0.0  0.0   1672   776 pts/0    R+   10:26   0:00  \_ bin/console

with exec the bash process is eliminated:

bash-5.1# ps axwuf
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root         1  0.1  0.0   1660   964 pts/0    Ss   10:25   0:00 bin/console

@glensc
Copy link
Contributor Author

glensc commented Jan 22, 2021

  • $@ would expand "i am a wizard" as "i" "am" "a" "wizard"
  • while "$@" would not do the word splitting and keep space in argument value: "i am a wizard"

lmgtfy:

@Ocramius
Copy link
Member

Ocramius commented Jan 22, 2021

Right, and if I pass in "-v --something-else" I would expect "-v" and "--something-else".

As for with/without exec: it is very clear to me that we have PID 1, then PID <N> (as a child), but what I'm saying is that this should be OK if not even wished.

@glensc
Copy link
Contributor Author

glensc commented Jan 24, 2021

You don't want or need the child process here. Child process is useful if there's code after the child process, but here it is not, child is the final runtime of the script (container). Simplified explanation: when using exec, instead of spawning a new child and monitoring its status in the main process, the main process is just replaced by the child. And in fact, bash doesn't even do proper monitoring that init process ought to do, therefore projects like https://github.com/krallin/tini are created.

Do you want me to extract two changes to separate PRs or why this is still on hold?

@Ocramius
Copy link
Member

or why this is still on hold?

It's on hold because I don't understand the details, and not understanding the details means I won't merge it until I do, and am ready to maintain it afterwards: there's no need to be passive-aggressive about it.

And in fact, bash doesn't even do proper monitoring that init process ought to do

Sounds like the best solution would be to get rid of bash overall: what about replacing the entrypoint with the PHP script entirely?

but here it is not, child is the final runtime of the script (container).

We still spawn git and gpg processes from it, but via symfony/console, which is much stricter about their execution.

@glensc
Copy link
Contributor Author

glensc commented Jan 24, 2021

Right, and if I pass in "-v --something-else" I would expect "-v" and "--something-else".

what do you mean by "pass", where? you need to realize that where you type the command in has it's own quoting styles.

Here's example from bash with my patch:

# gets $argv[1]="-v", $argv[2]="a value"
docker run --rm yourapp -v --something="a value"
# gets $argv[1]="-v", $argv[2]="a value"
docker run --rm yourapp -v --something='a value'
# gets $argv[1]="-v --something=false"
docker run --rm yourapp "-v --something=false"

without my patch you would get:

# gets $argv[1]="-v", $argv[2]="a", $argv[3]="value"
docker run --rm yourapp -v --something="a value"
# gets $argv[1]="-v", $argv[2]="--something=false"
docker run --rm yourapp "-v --something=false"

@glensc
Copy link
Contributor Author

glensc commented Jan 24, 2021

but here it is not, child is the final runtime of the script (container).

We still spawn git and gpg processes from it, but via symfony/console, which is much stricter about their execution.

none of that is related (read: nothing changes for them).

@glensc
Copy link
Contributor Author

glensc commented Jan 24, 2021

And in fact, bash doesn't even do proper monitoring that init process ought to do

Sounds like the best solution would be to get rid of bash overall: what about replacing the entrypoint with the PHP script entirely?

Right, no need to bash wrapper at all, so here's the PR:

@glensc glensc closed this Jan 24, 2021
@Ocramius Ocramius added the Duplicate This issue or pull request already exists label Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants