search

Ullaakut / nmap

Idiomatic nmap library for go developers
MIT License
922 stars 102 forks source link

Add modifyCmdFunc to Scanner and WithModifyCmdFunc option #103

Closed process0 closed 1 year ago

process0 commented 2 years ago

This PR adds support for modifying the exec.Cmd instance

Ullaakut commented 2 years ago

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?

process0 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 1 year ago

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

elivlo commented 1 year ago

Alright. I'll approve it.