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

Add modifyCmdFunc to Scanner and WithModifyCmdFunc option #103

Merged
merged 1 commit into from
Oct 11, 2022
Merged

Add modifyCmdFunc to Scanner and WithModifyCmdFunc option #103

merged 1 commit into from
Oct 11, 2022

Conversation

process0
Copy link
Contributor

@process0 process0 commented Oct 1, 2022

This PR adds support for modifying the exec.Cmd instance

@Ullaakut
Copy link
Owner

Ullaakut commented Oct 3, 2022

Hi @process0 !

Thanks for this PR. Could you explain what is your use case here? What changes do you need to apply to exec.Cmd?

I'm fine with the idea but I feel like the naming of WithModifyCmdFunc might be too long/complex. WDYT? Do you have suggestions for another name for it?

@Ullaakut Ullaakut added the enhancement New feature or request label Oct 3, 2022
@process0
Copy link
Contributor Author

process0 commented Oct 3, 2022

Use cases for me are setting SysProcAttr, i.e. cmd.SysProcAttr.Pdeathsig = syscall.SIGKILL, which forces killing the child process when the parent process dies. There are likely other use cases here beyond other SysProcAttr serttings (env vars, dir, etc), so a modify function fits best imo.

I can change the name to WithModifyCmd if that is simpler.

@elivlo
Copy link
Collaborator

elivlo commented Oct 4, 2022

Hi @process0 @Ullaakut

For me it was quite confusing at the first time I looked at the function name. Nearly all current functions that begin with With... only change nmap cli arguments. Nevertheless I think we should rename the function to something like WithCustomSysProcAttrs. I have nothing against modifying the exec.Cmd but we need to make sure that nobody misunderstand what it does.

Additionally we should only change this single attribute SysProcAttrs to prevent users accidentally overriding arguments or other options.

@process0
Copy link
Contributor Author

process0 commented Oct 4, 2022

@elivlo There are WithFilterHost, WithFilterPort, WithBinaryPath, WithContext that do more than set arguments. I was under the assumption that the With prefix is used due to the functional arguments choice.

Limiting functionality to SysProcAttr is fine for me, though i wonder if there would be any need for setting env vars or a working directory, or even having access to the process itself. I do think a strong warning should be added, like in WithCustomArgs.

@elivlo
Copy link
Collaborator

elivlo commented Oct 5, 2022

@process0 You're right. I just wanted to point out that I became a little perplexed. But it is fine for me to name the function like this. My opinion is that we should cover the command as best we can when there is currently no need for it.

I am fine with functions that modify SysProcAttr, Env and Dir so feel free to add them :)
If someone later needs to access more than these args, we'll open a new PR.

@process0
Copy link
Contributor Author

process0 commented Oct 7, 2022

@elivlo Understood. I've updated the PR to support custom SysProcAttr only, as that is my need.

@elivlo
Copy link
Collaborator

elivlo commented Oct 11, 2022

Alright. I'll approve it.

@elivlo elivlo merged commit b2a0803 into Ullaakut:master Oct 11, 2022
@process0 process0 deleted the add-modifycmdfunc branch October 15, 2022 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants