Remove Buf_io from the HTTP server #6064
Merged
Conversation
This file contains hidden or 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
Signed-off-by: Rob Hoes <[email protected]>
Signed-off-by: Rob Hoes <[email protected]>
The function is now passed a plain fd rather than a Buf_io.t ("bio")
value, as it does not actually use the buffered channel and just read
from the fd directly (using `Http.read_http_request_header`).
There used to be an older version if `request_from_bio` that had an
option to read requests in a different way and that did use Buf_io. This
was called the "slow path" and was removed in bc2ff45 in favour of the
current "fast path". This further clean-up opportunity was missed at
that time.
Signed-off-by: Rob Hoes <[email protected]>
The function `check_reusable_inner` used Buf_io to read a fixed-length HTTP response and then discarded the buffer. This is functionally the same as using `Unixext.really_read_string`, so do that instead. Signed-off-by: Rob Hoes <[email protected]>
At this point, the only function in the entire code base that read from a Buf_io.t is `Http_svr.read_body` (apart from a test for Buf_io). However, it only does so if the buffer is not empty and falls back to reading directly from the fd is not. And since nothing else reads from a Buf_io, the buffer is always empty... Signed-off-by: Rob Hoes <[email protected]>
The main difference between the BufIO and FdIO cases was that the former calls `assert_credentials_ok` with the `callback` in its `~fn` parameter, while the latter executed the `callback` directly after the credentials check. The function `assert_credentials_ok` either calls `fn` or raises an exception. Well, nearly... It did not actually call `fn` in the unix socket case, where checks are bypassed. This looks unintended and this patch corrects it. This only affects the following handlers in xapi, which use BufIO and require RBAC checks: post_remote_db_access, post_remote_db_access_v2, get_wlb_report, get_wlb_diagnostics, get_audit_log. I guess those were simply never used on the unix socket. The other thing that happens when using `~fn` is that the function `Rbac_audit.allowed_post_fn_ok` is called after `~fn`. This writes an "ALLOWED OK" line to the audit log. I don't see a reason not to do the same in all cases. The outcome is that now both cases of `add_handler` do the same and only the channel types are different. In the following commit the two handler types are joining into a single one, which is now easier. Signed-off-by: Rob Hoes <[email protected]>
HTTP handlers of type BufIO did not actually read from through the
buffer at all. Instead, they all assert that the buffer is empty and
then simply use the file descriptor.
All HTTP handlers now directly use file descriptors. The handler type
simply becomes:
type 'a handler = Http.Request.t -> Unix.file_descr -> 'a -> unit
Signed-off-by: Rob Hoes <[email protected]>
Signed-off-by: Rob Hoes <[email protected]>
lindig
approved these changes
Oct 18, 2024
Well, I did change bits... it's not just deleting code :) |
|
Real pirates ship |
|
Tests have passed and connecting with XenCenter also looks normal. |
psafont
reviewed
Oct 18, 2024
| @@ -713,78 +713,6 @@ let read_body ?limit req bio = | |||
| else | |||
| Buf_io.really_input_buf ~timeout:Buf_io.infinite_timeout bio length | |||
|
|
|||
| module Chunked = struct | |||
There was a problem hiding this comment.
Oof, this is used in vhd-tool. with its own copy of the code, of course.
There was a problem hiding this comment.
Ah is it a copy of http_svr... I did see the name, but thought it was something else.
There was a problem hiding this comment.
The implementation of ocaml/vhd-tool/src/chunked.ml looks different. Crucially, it doesn't use Buf_io.
psafont
approved these changes
Oct 18, 2024
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.
Cleaning up the attic... It turned out that the
Buf_iomodule, while used in the type signature of every HTTP handler, was not used for anything useful anymore.Step by step rationale in the commit messages, so best reviewed commit by commit.