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

Ideas for the next major release #18

Closed
9 tasks done
daladim opened this issue Aug 9, 2022 · 1 comment
Closed
9 tasks done

Ideas for the next major release #18

daladim opened this issue Aug 9, 2022 · 1 comment

Comments

@daladim
Copy link
Collaborator

daladim commented Aug 9, 2022

Hello n4r1b,

Here are a few ideas that could improve the crate, but that would likely break the compatibility, so they would probably need bumping to a new major version.
(note: I have more or less planned to work on them, and I already have a few draft branches locally, I'll probably make MRs soon, but I'm currently focusing on non-breaking changes first)
Feel free to comment or discuss them, maybe there are better solutions to these issues I haven't thought about yet

Features

  • Implement event filters (that's the // TODO: Add Filters option). As a kind of side-effect in my draft branch, this slightly modified a public API, and thus should be left for a major bump

API

  • Use a bitfield for TraceFlags instead of a u32
  • (to check) have a cleaner distinction between open, start and process. One of them is probably not required.
  • I'd personally rename some structs such as PropertyInfo or Schema. This issue comes from krabsetw names, but I feel they're unfortunate because they do not really convey their actual meaning (could be respectively PropertyWithBuffer and EventAndSchema for instance)
  • ...as a consequence, since the Parser contains the Schema, which contains the EventRecord, the Parser owns the Event data, and that feels weird. If I read correctly, this even prevents an ideal caching of such structs.
  • I think we could improve the Builder pattern for the Provider (e.g. some methods should be mutually exclusive and called only once, such as by_guid and by_name)
  • Provider and Trace have setters for fields that are pub. We could probably have only one of them

Safety considerations

  • Be able to move a UserTrace (or KernelTrace). We cannot do this currently, as we feeding OpenTraceA with pointers to UserTrace::data that must not be moved during the lifetime of the trace.
    For the same reason, we could consider introducing lifetime bounds for EventTraceLogfile
  • EventRecord should be a read-only safe wrapper, instead of a simple newtype
    Because it does not make sense to modify e.g. record.ExtendedDataCount

Performance

Ideas from #25 most likely require an API change, and hence a major version bump as well

daladim added a commit to daladim/ferrisetw that referenced this issue Sep 9, 2022
This branch will include changes described in n4r1b#18
daladim added a commit that referenced this issue Sep 9, 2022
This branch will include changes described in #18
@daladim
Copy link
Collaborator Author

daladim commented Nov 7, 2022

Closing this issue.
Most of it was done already, and #63 completes it

@daladim daladim closed this as completed Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant