Skip to content

cleanup: refactor submit event functions#476

Open
Molter73 wants to merge 4 commits intojv-ROX-33217-instrument-inode-tracking-on-directory-being-created-path_mkdirfrom
mauro/refactor-submit-event
Open

cleanup: refactor submit event functions#476
Molter73 wants to merge 4 commits intojv-ROX-33217-instrument-inode-tracking-on-directory-being-created-path_mkdirfrom
mauro/refactor-submit-event

Conversation

@Molter73
Copy link
Copy Markdown
Contributor

@Molter73 Molter73 commented Apr 6, 2026

Description

A new event_args_t type is added to group together common arguments used by all submit event functions. This reduces the number of arguments that need to be passed into these functions and make it harder to make mistakes in the ordering of the actual arguments.

While implementing this cleanup, a bug in the order of inodes being passed to submit_rename_event was fixed.

Finally, inode handling has also been simplified by removing the possibility of a NULL inode pointer from being used.

Checklist

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Automated testing

  • Added unit tests
  • Added integration tests
  • Added regression tests

If any of these don't apply, please comment below.

Testing Performed

This is simply a refactoring PR, CI should be enough to validate it works correctly.

@Molter73 Molter73 requested a review from a team as a code owner April 6, 2026 13:18
@Molter73 Molter73 marked this pull request as draft April 6, 2026 13:24
@Molter73 Molter73 marked this pull request as ready for review April 9, 2026 09:23
@Molter73
Copy link
Copy Markdown
Contributor Author

Molter73 commented Apr 9, 2026

This PR will cause conflicts with #465 which I'm hoping we can merge tomorrow, so I'll hold off on merging for now.

@JoukoVirtanen
Copy link
Copy Markdown
Contributor

JoukoVirtanen commented Apr 9, 2026

While looking at the PR, I thought about how we have

args->event = bpf_ringbuf_reserve(&rb, sizeof(struct event_t), 0);
  if (args->event == NULL) {
    args->metrics->ringbuffer_full++;
    return;
  }

Many times. Perhaps it would be better if this was its own function.

Then we could have something like

__always_inline static bool prepare_event(struct event_args_t* args,
                                          file_activity_type_t type) {
  args->event = bpf_ringbuf_reserve(&rb, sizeof(struct event_t), 0);
  if (args->event == NULL) {
    args->metrics->ringbuffer_full++;
    return false;
  }
  args->event->type = type;
  return true;
}

__always_inline static void submit_open_event(struct event_args_t* args,
                                              file_activity_type_t event_type) {
  if (!prepare_event(args, event_type)) {
    return;
  }
  __submit_event(args);
}

I think this is outside of the scope of the PR, but something I wanted to mention.

@JoukoVirtanen
Copy link
Copy Markdown
Contributor

This looks good. I left a couple of comments, but I don't want to block the PR.

@Molter73
Copy link
Copy Markdown
Contributor Author

While looking at the PR, I thought about how we have

args->event = bpf_ringbuf_reserve(&rb, sizeof(struct event_t), 0);
  if (args->event == NULL) {
    args->metrics->ringbuffer_full++;
    return;
  }

Many times. Perhaps it would be better if this was its own function.

Then we could have something like

__always_inline static bool prepare_event(struct event_args_t* args,
                                          file_activity_type_t type) {
  args->event = bpf_ringbuf_reserve(&rb, sizeof(struct event_t), 0);
  if (args->event == NULL) {
    args->metrics->ringbuffer_full++;
    return false;
  }
  args->event->type = type;
  return true;
}

__always_inline static void submit_open_event(struct event_args_t* args,
                                              file_activity_type_t event_type) {
  if (!prepare_event(args, event_type)) {
    return;
  }
  __submit_event(args);
}

I think this is outside of the scope of the PR, but something I wanted to mention.

This is a good call, I think it is small enough that I can fit it into this PR.

A new submit_event_args_t type is added to group together common
arguments used by all submit event functions. This reduces the number of
arguments that need to be passed into these functions and make it harder
to make mistakes in the ordering of the actual arguments.
@Molter73 Molter73 force-pushed the mauro/refactor-submit-event branch from a861f3d to 96c8f45 Compare April 10, 2026 10:49
@Molter73 Molter73 changed the base branch from main to jv-ROX-33198-instrument-inode-tracking-on-file_open-lsm-hook-use-inode_to_key April 10, 2026 10:50
@Molter73 Molter73 requested review from a team and rhacs-bot as code owners April 10, 2026 10:50
@Molter73 Molter73 changed the base branch from jv-ROX-33198-instrument-inode-tracking-on-file_open-lsm-hook-use-inode_to_key to jv-ROX-33217-instrument-inode-tracking-on-directory-being-created-path_mkdir April 10, 2026 10:50
@Molter73 Molter73 requested review from JoukoVirtanen and removed request for a team and rhacs-bot April 10, 2026 10:50
@Molter73
Copy link
Copy Markdown
Contributor Author

I'm getting some verifier errors in #487 due to the use_bpf_d_path argument being moved into the struct, seems the verifier no longer realizes it can remove a branch in the d_path implementation, so I'm gonna go back to have it as a separate argument.

CI run with RHCOS verifier error: https://github.com/stackrox/fact/actions/runs/24244378585/job/70786710312?pr=487

Copy link
Copy Markdown
Contributor

@JoukoVirtanen JoukoVirtanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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

Successfully merging this pull request may close these issues.

3 participants