-
Notifications
You must be signed in to change notification settings - Fork 380
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
Create temp directories for all tests in kprobes #255
Conversation
FIXES: #238 Signed-off-by: Anastasios Papagiannis <[email protected]>
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.
Looks good, just one minor nit from me.
kpChecker := ec.NewProcessKprobeChecker(). | ||
WithFunctionName(sm.Full("__x64_sys_openat")). | ||
WithArgs(ec.NewKprobeArgumentListMatcher(). | ||
WithOperator(lc.Ordered). | ||
WithValues( | ||
ec.NewKprobeArgumentChecker().WithIntArg(-100), | ||
ec.NewKprobeArgumentChecker().WithStringArg(sm.Full(mountPath+"/testfile")), | ||
ec.NewKprobeArgumentChecker().WithStringArg(sm.Full(filepath.Join(dir, "testfile"))), |
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.
This will fail if someone passes a non-absolute path here. I would suggested using a suffix matcher here, possibly stripping the leading periods if they exist.
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.
Good catch! So this will fail in the case where we use getOpenatChecker()
in a testcase where we don't use the correct path in __x64_sys_openat
. I propose to just be aware of that (i.e. add a comment/issue) when we add new tests.
I believe that we need more work to make that generic enough to handle all cases in __x64_sys_openat
. WDYT?
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.
Documenting the limitation is a fine approach IMO.
@tpapagian follow up with some documentation per review where appropriate. |
Before that tests that fail to create a new directory (i.e. already exists) just skipped.
FIXES: #238
Signed-off-by: Anastasios Papagiannis [email protected]