Skip to content
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

Please replace command string with array #5

Open
safinaskar opened this issue May 12, 2023 · 3 comments
Open

Please replace command string with array #5

safinaskar opened this issue May 12, 2023 · 3 comments

Comments

@safinaskar
Copy link

Hi. I just read this config in your blog:

[[target]]
name = "check if kernel is 6.2"
kernel = "./bzImage-v6.2"
command = "/bin/bash -c 'uname -r | grep -e ^6.2.0$'"

I don't like this config. It seems that command key is meant to be passed directly to execve function (as opposed to command, which needs to be interpreted by shell). But then command is conceptually an array, not a string! So, please, replace with this (toml syntax allows this):

command = ["/bin/bash", "-c", "uname -r | grep -e ^6.2.0$"]
@xfbs
Copy link

xfbs commented May 12, 2023

I think the relevant line is here. Currently it's defined as a string.

I believe you can get this to accept either a string or an array by defining a utility type like this:

#[derive(Deserialize)]
#[serde(untagged)]
pub enum Command {
    Simple(String),
    Array(Vec<String>),
}

and then using this Command type in the Config struct instead of a String. That way, you do not break existing configs (backwards-compatible). More information on how this works here.

Awesome work tho, I really love QEMU for testing and I'm super lazy to remember all the command-line options it takes!

@danobi
Copy link
Owner

danobi commented May 12, 2023

Hi, thanks for the feedback. I think this request makes sense — internally we are splitting the string into tokens like this anyways. But I’d also like to keep the current string format as a second option. It will be important for #3. So the above suggestion :).

There is precedent for multiple syntax forms (see Dockerfile CMD: https://docs.docker.com/engine/reference/builder/#cmd)

@safinaskar
Copy link
Author

If you really want to support two forms, then please make string form accept bash command, i. e. command for processing by shell. This is exactly what docker's CMD does

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants