fix: handle dead VMM and scan all urunc TAP devices during cleanup#476
Conversation
✅ Deploy Preview for urunc canceled.
|
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
Hello @goyalpalak18 ,
the changes look good. I have only left two small comments. Do not forget to fix the linting issues and rebase over main.
goyalpalak18
force-pushed
the
fix/network-leak-cleanup
branch
from
cce0f5c to
cfeaadb
Compare
|
Thanks @cmainas ! Just pushed these updates and rebased over main. Let me know if everything looks good now. |
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
Thank you @goyalpalak18 for the changes. One thing I noticed is that if something fails during a cleanup of a single device then the function returns ignoring the rest. So it would be better to log the error but not return immediately but later at the end of the loop. Also, please rebase over main, so we can merge.
|
@cmainas |
|
Hello @goyalpalak18 , it seems you have not signed your commits. Also, make sure to rebase and squash your commits so we can merge this PR. |
goyalpalak18
force-pushed
the
fix/network-leak-cleanup
branch
from
a256fcb to
927cada
Compare
|
Hey @cmainas , done! Squashed the commits into one, added the sign-off, and rebased on top of main. Let me know if anything else needs to be changed. |
|
Hello @goyalpalak18 , the commit linter is complaining about |
goyalpalak18
force-pushed
the
fix/network-leak-cleanup
branch
from
927cada to
c27eb2c
Compare
|
@cmainas Done! Added ESRCH to the dictionary file. |
Treat ESRCH from killProcess as success since the process is already gone, allowing network cleanup to proceed instead of failing silently. Refactored network.Cleanup to scan for all tap.*_urunc interfaces rather than relying on a hardcoded name. Continue processing remaining devices on per-device errors and return a joined error at the end. Fixes urunc-dev#408 Integrates urunc-dev#407 Signed-off-by: goyalpalak18 <goyalpalak1806@gmail.com>
goyalpalak18
force-pushed
the
fix/network-leak-cleanup
branch
from
c27eb2c to
30e590f
Compare
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
Thank you @goyalpalak18 for this fix.
Treat ESRCH from killProcess as success since the process is already gone, allowing network cleanup to proceed instead of failing silently. Refactored network.Cleanup to scan for all tap.*_urunc interfaces rather than relying on a hardcoded name. Continue processing remaining devices on per-device errors and return a joined error at the end. Fixes #408 Integrates #407 PR: #476 Signed-off-by: goyalpalak18 <goyalpalak1806@gmail.com> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>
Treat ESRCH from killProcess as success since the process is already gone, allowing network cleanup to proceed instead of failing silently. Refactored network.Cleanup to scan for all tap.*_urunc interfaces rather than relying on a hardcoded name. Continue processing remaining devices on per-device errors and return a joined error at the end. Fixes #408 Integrates #407 PR: #476 Signed-off-by: goyalpalak18 <goyalpalak1806@gmail.com> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>
2 participants
Description
Fixed the issue where
vmm.Stop()was returning an error if the process was already dead (like from a crash), which prevented network cleanup from running.I updated
killProcessinutils.goto treatESRCH(process not found) as a success. If it's gone, it's gone, so we can proceed.Also refactored
network.Cleanupto actually scan for anytap.*_uruncinterfaces instead of looking for a hardcoded string. This handles leftover devices much better.Huge thanks to @sidneychang for the scanning logic in #407 — I integrated that here.
Related issues
Fixes #408
Integrates #407
How was this tested?
make lintpasses.