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

exec: inconsistent Args semantics #793

Open
hades opened this issue Feb 20, 2025 · 3 comments
Open

exec: inconsistent Args semantics #793

hades opened this issue Feb 20, 2025 · 3 comments

Comments

@hades
Copy link
Contributor

hades commented Feb 20, 2025

As discussed in cnfgmngmntcmp.

Currently the Args parameter of the exec resource[1] is inconsistent with both the Golang's os.exec[2] and with C's exec(3) family of methods. For example, to call an equivalent of shell command "/usr/bin/ls -l /tmp" you would make the following calls:

In C:

const char* program = "/usr/bin/ls";
char* args[] = { "/usr/bin/ls", "-l", "/tmp" }
execv(program, args);

In golang:

cmd := &exec.Cmd{
	Path: "/usr/bin/ls",
	Args: []string{"/usr/bin/ls", "-l", "/tmp"},
}
cmd.Run()

(Note that golang also provides the exec.Command() convenience method that populates Cmd.Path with the first element of Cmd.Args, however the structure of the exec resource matches the structure of the exec.Cmd struct, which may cause (and has caused) confusion).

My suggestion is to replace the Args and Cmd parameters with a single Command parameter that behaves exactly as the exec.Command in golang, and does not cause additional confusion. The use of Cmd is also limited and, as also discussed before (and as I will reiterate in my next issue), flawed.

[1] https://mgmtconfig.com/docs/resources/#exec
[2] https://pkg.go.dev/os/exec

@purpleidea
Copy link
Owner

AIUI, and I remember we all discussed this at the conference, the main reason this is allowed, is so that a user can "customize" the displayed process name.

If that's the case, instead we could add a display => "whatever" field the the exec resource to gain that behaviour.

The important thing is that we don't want the user to have to specify argv[0] twice.

To avoid ambiguity, I ask that you perhaps make an mcl pseudo-code example of what you would consider "correct" behaviour.

Thanks!

@hades
Copy link
Contributor Author

hades commented Feb 22, 2025

Sure. Here's an mcl code for the current implementation:

# BAD
exec "exec1" {
  cmd => "/usr/bin/ls",
  args => ["-l", "/tmp",],
}

Here's the code for the proposed change:

# GOOD
exec "exec1" {
  command => ["/usr/bin/ls", "-l", "/tmp",],
}

Note that removing cmd is also aligned with #794

The use-case of overriding the display name of the process is dubious to me. However, if you really want to do it, I would suggest to go the other way, and to override the executable binary path, again, to be consistent with the usual exec semantics:

# GOOD (but with binary override)
exec "exec1" {
  command => ["totally not ls", "-l", "/tmp",],
  exec_binary => "/usr/bin/ls",
}

@purpleidea
Copy link
Owner

I guess this is kind of dependent on: #794

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

2 participants