Parsing syscall flag helper and constants #89
Conversation
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
helpers/syscall_args.go
Outdated
| // Typically linux syscalls have multiple options specified in a single | ||
| // argument via bitmasks, which this function checks for. | ||
| // It is meant to be used with the constants redefined in this package. | ||
| func EventArgumentContainsOption(option, rawArgument uint64) bool { |
There was a problem hiding this comment.
might be nice to accept multiple options (could be variadic if you swap the order)
helpers/syscall_args.go
Outdated
| return eventArgumentContainsOption(option, rawArgument) | ||
| } | ||
|
|
||
| func eventArgumentContainsOption(option, rawArgument uint64) bool { |
There was a problem hiding this comment.
sorry for nitpicking, but why the extra function call?
There was a problem hiding this comment.
It was for more flexibility in case we needed to change something without breaking API. In general I don't like having exported API functions call other exported API functions directly. But this doesn't apply after my latest rework.
helpers/argumentParsers.go
Outdated
| @@ -107,61 +107,61 @@ func ParseOpenFlags(flags uint32) string { | |||
|
|
|||
| // access mode | |||
| switch { | |||
| case flags&01 == 01: | |||
| case eventArgumentContainsOption(uint64(flags), O_WRONLY): | |||
There was a problem hiding this comment.
Since we are now expecting external users to use these consts (Tracee is emitting raw values), maybe it would be better to strongly-type them and to implement the stringer interface on the types: https://pkg.go.dev/fmt#Stringer
This way we (and others) can just fmt.Println(O_WRONLY) instead of the developer having to check the const value and then write the const name as string, like seen in this code block.
Possibly we can use the stringer utility to automate this: https://pkg.go.dev/golang.org/x/tools/cmd/stringer
There was a problem hiding this comment.
In other words, I'm saying these identifier-to-string mapping could be moved to where we define the consts, instead of where we print them, and by that they would become reusable
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
grantseltzer
force-pushed
the
flag-parse-helpers
branch
from
6177e2c
to
bc20af2
Compare
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
|
@rafaeldtinoco @mtcherni95 can I ask for either of your review here? |
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
|
I pushed a couple of changes - I realized after writing actual tests that the way I had it wouldn't be applicable to how Tracee would want to consume the library. I will work on this after the release, I think there's less of a need to push it through now. |
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
|
@yanivagman This PR makes changes to the parse helpers. I'd like to merge these before my patch for #1029 that I have ready waiting in the wings. Can you take a look at this? |
|
|
||
| func (c CloneFlagArgument) Value() uint64 { return c.rawValue } | ||
| func (c CloneFlagArgument) String() string { return c.stringValue } | ||
|
|
There was a problem hiding this comment.
It might be clearer if we put the CloneFlagArgument values defined above here so it is grouped in the same place.
Same for all other types.
WDYT?
There was a problem hiding this comment.
Sure, I can organize it that way
| f = append(f, CLONE_IO.String()) | ||
| } | ||
| if len(f) == 0 { | ||
| return CloneFlagArgument{}, fmt.Errorf("no valid clone flag values present in raw value: 0x%x", rawValue) |
There was a problem hiding this comment.
It might be that no flags were given as an argument (rawValue is 0), why do we want to return error in that case? Shouldn't we just return an empty string?
Same for all other options below.
There was a problem hiding this comment.
This is a good point, perhaps it should be
if len(f) == 0 && rawValue != 0 {
Which would mean that it isn't an empty argument, but it isn't any valid one. WDYT?
There was a problem hiding this comment.
Another option is to check at the beginning of the function if rawValue == 0 and return an empty string.
Then you can keep this test here
There was a problem hiding this comment.
Good call, just pushed that change
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
#88