retry if lseek(2) doesn't support SEEK_DATA or SEEK_HOLE

lindig commented 8 years ago

This PR tries to improve https://github.com/djs55/ocaml-vhd/pull/33. Improvement are:

We keep using #ifdef to make sure to only use SEEK_DATA, SEEK_HOLE when it is defined on a platform.
We retry when lseek(2) fails only when it is because the file system doesn't support SEEK_DATA or SEEK_HOLE. The previous patch retried irrespective of the reason for failure.

I'd like to mention remarks from an internal discussion on the design in general:

A wrapper around lseek like this should communicate problems rather than trying a fallback. This would mean that more details need to be exposed by the wrapper.
If a file descriptor is known not to support SEEK_DATA and the like, it would be better not to try it repeatedly but to associate this information with the file descriptor. This, again, would mean a different interface that exposes these details at the OCaml level.
For the given use case the PR is probably still the right thing as it increases abstraction at a small performance cost (of dynamically trying a system call that could be known to fail).

Signed-off-by: Christian Lindig christian.lindig@citrix.com

lesserevil commented 8 years ago

Yes, this is much better. Thank you!

lindig commented 8 years ago

My patch contains a bug: c_ret = lseek(c_fd, c_ofs, SEEK_END) must use 0 rather than c_ofs. I'm preparing a new one after having verified this.

lindig commented 8 years ago

I'm closing this in favour of a corrected pull request: https://github.com/djs55/ocaml-vhd/pull/35

mirage / ocaml-vhd