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

Added Version to Scanner, fixed ScanRunner interface bug #125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thmeitz
Copy link

@thmeitz thmeitz commented Nov 12, 2023

No description provided.

Copy link
Owner

@Ullaakut Ullaakut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @thmeitz ! Thanks very much for your contribution and sorry that it took me so long to take a look at it :) Hope you have a good day!

@@ -94,6 +98,55 @@ func (s *Scanner) Streamer(stream io.Writer) *Scanner {
return s
}

// Version returns the version of the installed nmap binary
func (s *Scanner) Version() (result any, err error) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (s *Scanner) Version() (result any, err error) {
func (s *Scanner) Version() (string, error) {

I recommend never using named return parameters when the function signature is implicitly clear enough without.

@@ -94,6 +98,55 @@ func (s *Scanner) Streamer(stream io.Writer) *Scanner {
return s
}

// Version returns the version of the installed nmap binary
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Version returns the version of the installed nmap binary
// Version returns the version of the used nmap binary.

This is more correct, since multiple nmap binaries can be installed, but only one is in use by the library at any given time.

Comment on lines +126 to +127
result = splitted[2]
break
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result = splitted[2]
break
return res[2], nil

Comment on lines +124 to +125
splitted := strings.Split(m, " ")
if len(splitted) >= 2 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
splitted := strings.Split(m, " ")
if len(splitted) >= 2 {
res:= strings.Split(m, " ")
if len(res) >= 2 {

Comment on lines +131 to +145
errScanner := bufio.NewScanner(stderr)
var errorOutput = []string{}
for errScanner.Scan() {
m := errScanner.Text()
errorOutput = append(errorOutput, m)
}

err = cmd.Wait()
if err != nil {
return
}

if len(errorOutput) > 0 {
return nil, fmt.Errorf("%v", errorOutput)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this code? To check that we can run the binary without errors on top of getting the version info? If yes, these should be two separate functions IMO.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check all your comments and will make the proposed changes in the first calendar week of december.
I'm really busy at the moment.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No rush, take all the time you need! :) Thanks again!

@Ullaakut Ullaakut added the enhancement New feature or request label Nov 21, 2023
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.

None yet

2 participants