-
Notifications
You must be signed in to change notification settings - Fork 425
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
Fix checkpid on windows #2031
Fix checkpid on windows #2031
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hurricanehrndz for the contribution. Left one request.
} | ||
} | ||
// Prepare the command | ||
cmd := exec.Command("tasklist", "/FI", fmt.Sprintf("PID eq %d", pid)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use a function similar to GetSystem32Command to avoid path issues?
if strings.Contains(line, fmt.Sprintf("%d", pid)) { | ||
return true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you handle scanner.Err()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If scanner.Err actually returns an error the overall return of the function would be false, the only benefit to handling scanner.Err is to log an issue with the scanning, but it is not like the log is written to disk or anything so ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but if there's an issue that would at least give us a hint when debugging the application from CLI
Co-authored-by: Viktor Liu <vma@randomcrap.eu>
Co-authored-by: Viktor Liu <vma@randomcrap.eu>
Sounds good, I will updates it accordingly
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Viktor Liu ***@***.***>
Sent: Thursday, May 23, 2024 3:43:25 PM
To: netbirdio/netbird ***@***.***>
Cc: Carlos Hernandez ***@***.***>; Mention ***@***.***>
Subject: Re: [netbirdio/netbird] Fix checkpid on windows (PR #2031)
@lixmal commented on this pull request.
________________________________
In client/ui/client_ui.go<#2031 (comment)>:
+ cmd := exec.Command("tasklist", "/FI", fmt.Sprintf("PID eq %d", pid))
+
+ // Get the output
+ output, err := cmd.Output()
+ if err != nil {
+ return false
+ }
+
+ // Parse the output
+ scanner := bufio.NewScanner(strings.NewReader(string(output)))
+ for scanner.Scan() {
+ line := scanner.Text()
+ if strings.Contains(line, fmt.Sprintf("%d", pid)) {
+ return true
+ }
+ }
Right, but if there's an issue that would at least give us a hint when debugging the application from CLI
—
Reply to this email directly, view it on GitHub<#2031 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABMJBTMGPUWD5LZPKZ5GWRDZDZPH3AVCNFSM6AAAAABIDTY5BGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZVGE2DEMRXG4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Although this patch somewhat works, I am not entirely happy, since a miss click will run multiple apps. I will close this for now, and reopen and when I have a better solution |
Describe your changes
syscall.signal 0 does not work on windows as it is not implemented
Issue ticket number and link
Checklist