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

Move usersim code into a separate repo #2596

Merged
merged 7 commits into from
Jun 27, 2023
Merged

Conversation

dthaler
Copy link
Collaborator

@dthaler dthaler commented Jun 14, 2023

Description

  • Moved usersim code into a separate repo so it can be used by other projects too.
  • Removed DISPATCH test code from _preprocess_load_native_module to fix bindmonitor test which was failing because it calls bpf_get_current_logon_id() which is not legal to call at DISPATCH. Per Anurag: "the code was added earlier to execute the 2 paths in native module, and it is not needed anymore, as the "DISPATCH" code has been removed now from the native module"
  • Use FindWDK from the usersim project rather than directly to avoid two copies

Addresses a portion of #2586
This step is usable on its own, but additional simplifications will come in a future PR.

Testing

Existing tests are updated.

Documentation

No impact.

@dthaler dthaler marked this pull request as draft June 14, 2023 16:13
@dthaler dthaler changed the title Move usersim code into a separate repo WIP: Move usersim code into a separate repo Jun 14, 2023
@dthaler dthaler force-pushed the usersim branch 9 times, most recently from ac0479e to 0f65744 Compare June 20, 2023 02:20
Signed-off-by: Dave Thaler <[email protected]>
Per Anurag: the code was added earlier to execute the 2 paths in
native module, and it is not needed anmore IIRC, as the "DISPATCH"
code has been removed now from the native module

Signed-off-by: Dave Thaler <[email protected]>
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #2596 (309a341) into main (d8cee6a) will decrease coverage by 2.69%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##             main    #2596      +/-   ##
==========================================
- Coverage   83.78%   81.10%   -2.69%     
==========================================
  Files         157      146      -11     
  Lines       29177    27605    -1572     
==========================================
- Hits        24447    22389    -2058     
- Misses       4730     5216     +486     
Impacted Files Coverage Δ
libs/platform/user/framework.h 100.00% <ø> (+2.43%) ⬆️
tests/netebpfext_unit/netebpf_ext_helper.h 100.00% <ø> (ø)
libs/platform/ebpf_platform.c 93.80% <93.80%> (ø)
libs/platform/user/ebpf_platform_user.cpp 83.10% <94.11%> (-1.46%) ⬇️
libs/platform/unit/platform_unit_test.cpp 98.68% <100.00%> (-1.15%) ⬇️
tests/end_to_end/end_to_end.cpp 97.59% <100.00%> (-0.07%) ⬇️
tests/end_to_end/helpers.h 89.75% <100.00%> (ø)
tests/end_to_end/netsh_test.cpp 98.17% <100.00%> (-1.05%) ⬇️
tests/end_to_end/test_helper.cpp 82.55% <100.00%> (-3.50%) ⬇️
tests/netebpfext_unit/netebpf_ext_helper.cpp 98.00% <100.00%> (+0.02%) ⬆️
... and 1 more

... and 46 files with indirect coverage changes

Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
@dthaler dthaler changed the title WIP: Move usersim code into a separate repo Move usersim code into a separate repo Jun 21, 2023
@dthaler dthaler marked this pull request as ready for review June 21, 2023 01:07
Signed-off-by: Dave Thaler <[email protected]>
@@ -76,6 +77,9 @@ _netebpf_ext_helper::_netebpf_ext_helper(
nmr_hook_client_handle = std::make_unique<nmr_client_registration_t>(&hook_client, client_context);
nmr_hook_client_handle_initialized = true;
}

_fwp_engine::get()->set_sublayer_guids(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think the _fwp_engine::test_* functions in usersim/src/fwp_um.cpp should be part of this repository.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will deal with that in the next PR in the series. But offhand, I probably disagree. Anything that allows a unit test to exercise WFP apis in a way that isn't ebpf specific would help any WFP client driver developer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this is not blocking for this PR (I have already approved the PR).

@Alan-Jowett Alan-Jowett added this pull request to the merge queue Jun 27, 2023
Merged via the queue into microsoft:main with commit ec27659 Jun 27, 2023
@dthaler dthaler deleted the usersim branch June 27, 2023 18:32
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