lucavallin / barco

Linux containers from scratch in C.
https://lucavall.in
MIT License
1.48k stars 87 forks source link

Support multi args from -a #4

Closed ghost closed 12 months ago

ghost commented 1 year ago

Currently barco just support one argument, it's hard to test with some complex command, this PR want to solve this

sudo ./bin/barco -u 0 -m / -c /bin/ls -a . -a -l -a -h -a -i -v
lucavallin commented 1 year ago

Hey @swordli-dev! Thanks for the contribution. This would be a nice addition to barco ! I left a few comments ;)

ghost commented 1 year ago

Hey @swordli-dev! Thanks for the contribution. This would be a nice addition to barco ! I left a few comments ;)

Thanks for your quick REVIEW, just refactored a little, please take a look when you have a chance

lucavallin commented 1 year ago

Hey @swordli-dev! It's looking good. Something I thought about however: wouldn't it be more practical to support multiple args with something like the following?

sudo ./bin/barco -u 0 -m / -c /bin/ls -a "-lhi" -v

Having one -a for each parameter to the process running in the container might make the command a bit hard to read. Also, what happens if we add a param like -l to barco? Will the -a -l be interpreted as -l for /bin/sh or as -l for barco?

ghost commented 1 year ago

Yes, sometime we could use union parameter like -lhi, but I think most of programs need to be specified mutiple arguments like

sudo ./bin/barco -u 0 -m / -c $(which find) -a /usr/include -a -name -a limits.h -v
lucavallin commented 1 year ago

@swordli-dev That makes sense, but is there maybe a way to do like -a "ls -l -h -i " (pass everything as a single string) ?

ghost commented 1 year ago

-a "ls -l -h -i "

I think the argv of execve does support parsing "ls -l -h -i" to ["ls", "-l", "-h", "-i"]. You can try it. I don't recommand we should parse multi arguments by a union-parameter even though we could parse ls -l -h -i by ourself. @lucavallin

ghost commented 1 year ago

Thanks, I saw you deleted the comments Dancing3dGIF

lucavallin commented 1 year ago

Hey @swordli-dev! Thanks for the ping. About your last comment, what is the final decision with those flags?

ghost commented 1 year ago

Hey @swordli-dev! Thanks for the ping. About your last comment, what is the final decision with those flags?

As I mentioned before, if you want to support more than one parameter like a example find /usr/include -name limits.h, we need to use -a for each parameter -> -c find -a "/usr/include" -a "-name" -a "limits.h", is it valid if it run as -c find -a "/usr/include -name limits.h"?

lucavallin commented 1 year ago

@swordli-dev I haven't tested it, but it from a UX perspective, a single -a param (the second example you made) is better for users. Eventually we could also just have the -c and split command and params internally.

lucavallin commented 1 year ago

Hey @swordli-dev! What do you think of the last comment?

ghost commented 12 months ago

Hey @swordli-dev! What do you think of the last comment?

Sorry, recently PTO. Year, I agree with you, I think we could make another PR for multiple arguments with in one -a flag. Let's close this PR now.