fix condition in mincore

[cedlemo]

@aantron,

#625

I started to try to understand the behavior with different cases :

when the last condition is : offset > length buffer - (Array.length states * page_size)

case where mem_len is > buffer_len

	buff_len = 6 offset = 0  pages = 1
		offset = 0
		mem_len = 1*4096
		offset > (buff_len - mem_len < 0) -> true -> Error

        buff_len = 4096 + 6 offset = 0  pages = 2
		offset = 0
		mem_len = 2*4096
		offset > (buff_len - mem_len < 0) -> true -> Error

	buff_len = 4096 + 6 offset = 4096 pages = 2
		offset = 4096
		mem_len = 2*4096
		offset > (buff_len - mem_len) < 0 -> true ->  Error

case where mem_len is < buffer_len

	buff_len = 4096 + 6 offset = 0 pages = 1
		offset = 0
		mem_len = 1*4096
		offset > buff_len - mem_len = 6  -> false -> ok

	buff_len = 4096 + 6 offset = 4096 pages = 1
		offset = 4096
		mem_len = 1*4096
		offset > (buff_len - mem_len) = 6 -> true ->  Error

and instead of we need to change the check to allow the last page queried to only partially overlap the buffer. like you said, I thought that we just have to be sure that the offset is not greater than the buffer length.
I have done the tests with the last condition offset > buffer length and it works as expected.

case where mem_len is > buffer_len

	buff_len = 6 offset = 0  pages = 1
		offset = 0
		mem_len = 1*4096
		offset > buff_len -> false -> ok

        buff_len = 4096 + 6 offset = 0  pages = 2
		offset = 0
		mem_len = 2*4096
		offset > buff_len -> false -> ok

	buff_len = 4096 + 6 offset = 4096 pages = 2
		offset = 4096
		mem_len = 2*4096
		offset > buff_len > 0 -> false -> ok

case where mem_len is < buffer_len

	buff_len = 4096 + 6 offset = 0 pages = 1
		offset = 0
		mem_len = 1*4096
		offset > buff_len -> false -> ok

	buff_len = 4096 + 6 offset = 4096 pages = 1
		offset = 4096
		mem_len = 1*4096
		offset > buff_len -> false -> ok

I send this PR first for validation, then I will send you another PR with the tests for all the IO part of Lwt_bytes.

[aantron]

I think we want to make sure that the offset is far back enough in the buffer, that for a flag array for N pages, the first N-1 pages are fully in memory, and there is at least one byte for the last page.

So, we want there to be at least (Array.length states - 1) * page_size + 1 bytes in the buffer starting at offset. So, we need something like length buffer - offset >= (Array.length states - 1) * page_size + 1. The only thing to watch out for is that Array.length states - 1 might be negative, if the array is empty. It seems like the check will still give us the right behavior, though, but we should probably leave a comment about that in the code.

[cedlemo]

Ok,
so if we want that there is at least (Array.length states - 1) * page_size + 1 bytes in the buffer starting at offset, the condition that throws an exception for bad parameter is:

length buffer - offset < (Array.length states - 1) * page_size + 1

If I go through several cases like this:

• buff_len = 6 offset = 0 n_states = 1

6 - 0 < (1 - 1) * page_size + 1 (* -> false *)

• buff_len = page_size + 6 offset = 0 n_states = 1

page_size + 6 - 0 < (1 - 1) * page_size + 1 (* -> false *)

• buff_len = page_size + 6 offset = page_size n_states = 1

page_size + 6 - page_size < (1 - 1) * page_size + 1 (* -> false *)

• buff_len = page_size + 6 offset = page_size n_states = 2

page_size + 6 - page_size < (2 - 1) * page_size + 1 (*-> true (throw exception)*)

Do you want that I do all those tests (I have already almost done them) or do you see another case that are not covered?

[aantron]

I'd say to set the offset always to page_size, then do:

1. buffer length = page_size * 2, n_states = 1 (should be ok)

2. buffer length = page_size * 2, n_states = 2 (should fail)

3. buffer length = page_size * 2 + 1, n_states = 2 (should be ok)

I think that exercises the main corner cases completely and exactly, though something may be missing.

We should also test what happens when n_states = 0.

[aantron]

I believe the condition is inverted, and we need < instead of >=.

[cedlemo]

argh you are right, I copy/pasted without checking sorry, the tests in the PR #628 are for length buffer - offset < (Array.length states - 1) * page_size + 1

[aantron]

Thanks :) It happens, I guess I started talking in inverted terms (when the bounds are ok, rather than when to throw an exception). It was misleading, sorry about that.