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

CP-47001 Introduce file descriptor testing framework #83

Closed

Conversation

edwintorok
Copy link
Contributor

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.

@edwintorok edwintorok force-pushed the private/edvint/testing branch 2 times, most recently from 2b4a3ac to 8d19a23 Compare December 14, 2023 13:16
@edwintorok
Copy link
Contributor Author

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]>
@edwintorok edwintorok force-pushed the private/edvint/testing branch from 8d19a23 to 218a5f6 Compare December 20, 2023 17:42
@edwintorok
Copy link
Contributor Author

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]>
@edwintorok edwintorok force-pushed the private/edvint/testing branch from 218a5f6 to 8cfb5b4 Compare December 20, 2023 18:22
@edwintorok
Copy link
Contributor Author

(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)

@lindig lindig self-requested a review December 21, 2023 10:49
Copy link
Contributor

@lindig lindig left a 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.

dune-project Outdated Show resolved Hide resolved
Comment on lines +51 to +54
module Syntax : sig
val ( let@ ) : ('a -> 'b) -> 'a -> 'b
(** [let@ fd = with_fd t in ... use fd] *)
end
Copy link
Member

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

Copy link
Contributor Author

@edwintorok edwintorok Dec 21, 2023

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

@edwintorok edwintorok force-pushed the private/edvint/testing branch from ebcfa50 to ab54dc4 Compare December 21, 2023 17:14
@edwintorok
Copy link
Contributor Author

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.
There are a few more modules to add before this can be used for quickcheck, but that will go into the xapi-fd-test package instead and I'll do that in a separate PR.

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]>
@edwintorok
Copy link
Contributor Author

This is continued in xapi-project/xen-api#5402

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