-
Notifications
You must be signed in to change notification settings - Fork 15
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
CP-47001 Introduce file descriptor testing framework #83
CP-47001 Introduce file descriptor testing framework #83
Conversation
2b4a3ac
to
8d19a23
Compare
Fixed the CI failure, had to add the new packages to the github action workflow. |
Helps catch missing dependencies in dune files. Signed-off-by: Edwin Török <[email protected]>
This will be a new library that will provide a more type-safe interface to file descriptor operations. Useful on its own, but also for testing stdext. Minimal dependencies, only Unix (and Alcotest for testing). Signed-off-by: Edwin Török <[email protected]>
This will be a test framework providing QCheck generators and properties for testing file descriptor operations. It will try to generate: * different kinds of file descriptors * actual data written/read on the other end of pipes and socket pairs * different speeds and delays on the other end to find buffering bugs * file descriptors that are >1024 to find bugs with select Signed-off-by: Edwin Török <[email protected]>
We are going to use type-level constraints a lot. Try to future proof it by using the recommended compiler flag. `ocamlc` says this about `-principal`: > When using labelled arguments and/or polymorphic methods, this flag is required to > ensure future versions of the compiler will be able to infer types correctly, even if internal algorithms change Signed-off-by: Edwin Török <[email protected]>
This is not enabled by default (but bisect-ppx is nevertheless a build-time dependency) Usage: `make coverage` Signed-off-by: Edwin Török <[email protected]>
8d19a23
to
218a5f6
Compare
I think the properties and operations module are now ready to review, next up would be some modules that do some actual quickchecking, after a bit more plumbing. |
Lightweight wrapper using polymorphic variants to track read, write, and file kind properties on file descriptors. We only track the property at the time the file descriptor was opened. This prevents bugs like accidentally swapping the read and write ends of a pipe, or attempting to run an operation on a file descriptor that would alway s fail (e.g. setting a socket timeout on a pipe) Write tests using cram-style expect tests that the operations we expect to be forbidden by this type system are actually forbidden. The error messages may be compiler version dependent, so only run them on OCaml 4.14.1 for now. Signed-off-by: Edwin Török <[email protected]>
Use the capabilities module to wrap most Unix operations needed in testing Unixext Add a testsuite that checks that whenever the type says "never" the underlying file descriptor operation would indeed raise an exception. This ensures that the type constraints we declare are actually correct. The checks use unsafe operations that bypass the type layer. Similarly check that operations that are accepted by the type system and marked as "always" in the type succeed. Signed-off-by: Edwin Török <[email protected]>
218a5f6
to
8cfb5b4
Compare
(removed the automated coveralls submission,that requires admin permission to set up, we can do that separately -- and we can use either coveralls or codecov) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is extensively documented and comes with test but it is also quite intricate and difficult to review.
module Syntax : sig | ||
val ( let@ ) : ('a -> 'b) -> 'a -> 'b | ||
(** [let@ fd = with_fd t in ... use fd] *) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't this module should be exposed, seems like an implementation detail. If users want to use let@ they can easily define it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be used by other modules that are not part of this PR yet, and it is already used by the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've kept this as is for now. Operations
isn't the ideal place for this let@
definition, but we should find some shared place for it to encourage its use (it is the preferred way of using the with_fd
and with_fd2
functions in this module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I move it to a toplevel syntax.mli
in xapi-fdcaps
?
Signed-off-by: Edwin Török <[email protected]>
ebcfa50
to
ab54dc4
Compare
I think these modules are now mostly complete, I'll squash the fixups into the appropriate commits once you had a chance to take another look at them. |
Signed-off-by: Edwin Török <[email protected]>
It can be used to wrap read or write operations andobserve the data that is transferred, and elapsed time. It also provides 2 functions that create a file of a given kind. We only test UNIX sockets, because socketpair doesn't support TCP sockets on Linux. Signed-off-by: Edwin Török <[email protected]>
Signed-off-by: Edwin Török <[email protected]>
This is continued in xapi-project/xen-api#5402 |
This will be used to test the stdext and XAPI epoll changes.
I'm breaking this up into one module / commit to make it easier to review.
This PR is for the build plumbing and detecting double close (which will be needed to write a reliable test suite).
Initially I also had some code here that would close leaked file descriptors, but I dropped it because even with all the safety checks I'm not sure it can be done safely (you could have leaked a
Unix.file_descr
and have another thread writing to it, and if you close it then the FD could be reused by something else).Next will be some wrappers that track read and write permissions using phantom types, and then a QuickCheck generator and property tester.