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

Added test to show weird behaviour with small output buffers. #29

Closed
wants to merge 1 commit into from

Conversation

paulharris
Copy link

Backported from my CMake-based branch.

I see issues when the output buffer is too small.
librsync.h indicates I should increase the buffer size and reexecute, but that seems to be buggy.

Here is a test, the parameters control the input size, the initial output buffer size, and the output buffer growth rate (additive).

Its quite easy to allocate too-small a buffer, and as the input size goes up, it seems that the minimum required output buffer sizes also change.

@sourcefrog
Copy link
Contributor

Thanks.

Is there already a bug for this?

Are you saying this fails for some parameter values?

@paulharris
Copy link
Author

Hi,

uuh I don't think there is a bug for this,
and yes it fails for many parameters.

Smallest test case fails,
where the input is zero bytes,
initial output buffer is 1 byte,
and output buffer is grown by 1 byte at a time.

Biggest test case that fails is where the
input is 2,000,000 bytes
initial output buffer is 16000 bytes
output buffer is grown by 30 bytes at a time.

These numbers are just picked out of the air by me,
the input buffer is just filled with 'x' characters,

This patch has a script you can use to compile and run, you can see the parameters in the shell script there. I also have a patch on top of my cmake system, but for the base rsync, the procedure is:

git checkout -b issue-small-output paulharris/issue-small-output
./autogen.sh && ./configure && make && (cd tests && ./test_small_output.run)
and to repeat,
cd tests
./test_small_output.run

To run the small testcase, for example, its:
./test_small_output.test 0 1 1
big test case:
./test_small_output.test 2000000 16000 30

output for small:
$ ./test_small_output.test 0 1 1
Input size: 0
Output buffer start at 1 step growth of 1
FAIL, expected 'done'

Show success (just a bigger initial buffer):
$ ./test_small_output.test 0 12 1
Input size: 0
Output buffer start at 12 step growth of 1
Final signature size: 12

@sourcefrog
Copy link
Contributor

Thanks.

Is there already a bug for this?

Are you saying this fails for some parameter values?

@paulharris
Copy link
Author

strange, I see the same comment of yours twice.

btw I am not sure how I go about making a bug and attaching this pull request...

@sourcefrog
Copy link
Contributor

So it looks like this has the test driver in the tree, but not run by cmake (probably because this was backported), and it feels like tests that aren't hooked in will just rot. I'm glad you wrote this but it might be better to leave it attached to the bug until the bug itself is fixed, rather than merging in either a failing or not-running state.

rs_buffers_t buf;
buf.next_out = outbuf;
buf.avail_out = start;
buf.next_in = (char*)(in); // TODO why is this not const void* ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving all the data pointers to void* would be a worthwhile separate change, but probably counts as an API break.

@sourcefrog
Copy link
Contributor

I'm also going to close the PR and leave the bug open until we have a fix. Thanks again.

@sourcefrog sourcefrog closed this Jul 31, 2016
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.

None yet

2 participants