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

reapi c++: add satisfy endpoint #1184

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Apr 20, 2024

Problem: the c++ bindings do not have satisfy support.
Solution: add satisfy to them.

In practice I was adding this for exposure to the Go bindings, but I do not think it is necessary, because the Go bindings use the C bindings, which already have reapi_cli_match_satisfy. I saw that match_allocate seems to have support to provide the SATISFIABILITY match_op, which is provided to the traverser, so I tried to call that same function. I am opening this PR in case it is interesting or useful. If not,
please close and disregard.

If the PR is desired, here are questions I have:

  • I notice the general pattern for functions is to return a return code (0 success, -1 failure) and then take references to variables provided by the user (that are updated). I assume since we don't know what those are coming in as, we should set them to what we want for a default (for example, -1 for satisfied "no").
  • I'm not adding Go tests here because (at least I want) the main Go wrappers to be in fluxion-go. I am going to do some PRs there shortly to update the go version.
  • That said, how are the bindings tested? I couldn't find the tests in here.
  • The reapi_cli seems to support the overhead variable since it calls match_allocate under the hood, which (I think?) sets it. But the module (by way of the rpc) doesn't seem to have that, so the signatures are different. Is that an issue?
  • As best as I could tell, the RPC only returns 0 when it is a satisfy "YES"
  • It's not clear to me what the "at" variable would be given a satisfy match_op. Reading the docstring, it sounds like a "yes" is >= 0 so I used that, but I'd like to understand why. It's hard to read this code because the variable "at" is really tiny and a substring of other variables and my eyesight is bad in my elderly dinosaur years. 😆

And again, if this is not of interest, please disregard! Going to do some work on fluxion-go next.

Problem: the c++ bindings do not have satisfy support.
Solution: add satisfy to them.

In practice I was adding this for exposure to the Go
bindings, but I do not think it is necessary, because
the Go bindings use the C bindings, which already have
reapi_cli_match_satisfy. I saw that match_allocate
seems to have support to provide the SATISFIABILITY
match_op, which is provided to the traverser, so I
tried to call that same function. I am opening this
PR in case it is interesting or useful. If not,
please close and disregard.

Signed-off-by: vsoch <[email protected]>
Copy link

codecov bot commented Apr 20, 2024

Codecov Report

Merging #1184 (f08ad38) into master (1c68ec6) will decrease coverage by 0.2%.
Report is 31 commits behind head on master.
The diff coverage is 0.0%.

❗ Current head f08ad38 differs from pull request most recent head c9fb2bf. Consider uploading reports for the commit c9fb2bf to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1184     +/-   ##
========================================
- Coverage    73.9%   73.8%   -0.2%     
========================================
  Files         103     102      -1     
  Lines       14396   14396             
========================================
- Hits        10644   10627     -17     
- Misses       3752    3769     +17     
Files Coverage Δ
resource/reapi/bindings/c++/reapi_cli_impl.hpp 36.8% <0.0%> (-0.6%) ⬇️
resource/reapi/bindings/c++/reapi_module_impl.hpp 50.3% <0.0%> (-6.8%) ⬇️

... and 6 files with indirect coverage changes

@vsoch
Copy link
Member Author

vsoch commented Apr 20, 2024

I am sorry codecov bot, please forgive my sins 🙏

// A value of at >= 0 is supposed to mean match_allocate success
// (0 is allocated, >0 is timestamp of reservation). Assuming
// satisfy could be either of these cases, we check for that here
if ((return_code == 0) && (at >= 0)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think I might need to check that errno thing here instead of the response? I need to read about what it is.

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.

None yet

1 participant