CA-391656: OCaml C stubs: release runtime lock around all xenctrl calls #4916
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…use just errno which is It is usually set to XC_INTERNAL_ERROR, except in xenguest, but we have no bindings for that. This API might get deleted in upstream Xen too: the error code is stored in the xenctrl handle, but we made that global per process, and shared among all threads. Which means that xenctrl calls from different threads can overwrite each-others' error code. Also do not use a 'static' buffer, this will no longer be thread safe with OCaml 5.0 where the runtime lock is per domain instead of global. Signed-off-by: Edwin Török <edvin.torok@citrix.com>
This is a C call, and might benefit from a bit more parallelism if it inside the blocking section. Signed-off-by: Edwin Török <edvin.torok@citrix.com>
Most of these calls pass only integers or pointers allocated in C, so it is safe to release the OCaml runtime lock. Replace direct passing of Xfm_val/Caml_ba_data_val with storing in a temporary local variable (the result of Xfm_val or Caml_ba_data_val won't move since they are C pointers, but the OCaml value passed in as params to these macros might!) Signed-off-by: Edwin Török <edvin.torok@citrix.com>
…e blocking section where possible unshadow is not thread safe, but the other ones should be. TODO: double check all C API calls in unixpwd.c with the MT-safe portion of the manpage Signed-off-by: Edwin Török <edvin.torok@citrix.com>
We may intend to upstream the xenctrlext stubs to Xen, so it should follow the Xen CODING_STYLE. The .clang-format here is based on a version I sent upstream that tries to follow that coding style. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
|
This PR has been open for more than a year with 0 comments. |
|
I should fix the conflicts here, and run the static analyzer. I'll keep the PR open meanwhile though. |
edwintorok
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a commit in the past that added enter/leave around all stubs, however since then more stubs got added, and not all of them released the runtime.
This is opened as draft, since it is a riskier change that needs a bit more stress testing too (not just a BST), and potentially also an improved static analyzer.
This PR builds on top of the C stub bugfix PR
Review only these 3 commits here: