Allow dynamic configuration of maximal assertion message size. #128

orenbenkiki · 2017-08-27T05:55:19Z

Resolve issue #127.

The new function is annotated as @since 0.12.0 under the assumption that would be the next released version.

brarcher · 2017-08-27T15:44:20Z

src/check_pack.c

+}
+
+static int get_max_msg_size(void) {
+    if (ck_max_msg_size <= 0) {


To be consistent with the behavior of the other environment variables, the order of precedence should be:

environment variable

check_set_max_msg_size()

default value

See CK_DEFAULT_TIMEOUT as an example of this.

brarcher · 2017-08-27T15:44:49Z

src/check_pack.c

+            ck_max_msg_size = atoi(env);
+
+        if (ck_max_msg_size <= 0)
+            ck_max_msg_size = 4096;


Could 4096 still be a variable or #define above, so it is not a magic number?

brarcher · 2017-08-27T15:52:53Z

I think it would be valuable to document this new API call in the manual, maybe somewhere in this section. Also mention that the environment variable can override the value.

There is an section which shows all the environment variables here. Also mention the new environment variable there.

Finally, I think it would be valuable to have a new test for this, which shows that if the value is set too low Check does cause an error but if it is set high enough Check continues with the test. Many unit tests in Check run using Check itself, which verifies that certain errors occur when they are expected. The tests themselves are defined up to this point, they are added to the test suite (with whatever environment variables are used) here, and the table which says what the output of the tests should be is here. Try adding a test or so to verify the behavior of the new API and environment variable.

orenbenkiki · 2017-08-27T18:59:55Z

Thanks for the review pointers. I fixed the code to give precedence to the environment variable, and updated the TexInfo documentation.

I have difficulty with creating a test though. A too-long message invokes eprintf, which writes to stderr. It seems the expected-output mechanism doesn't capture such output... is there an example of a test that captures this sort of output I could use as a guide?

brarcher · 2017-08-27T19:07:26Z

is there an example of a test that captures this sort of output I could use as a guide?

Hm, seems that the normal test setup will not work for this. I may suggest the following then. First, add a test which checks that using the API is OK, and that the value returned from the getter is as expected (e.g the env var takes precedence). For exercising the feature, the test could be put into another program and run independently. The example of this is the memory leak test test_mem_leaks.sh, which captures Valgrind output. That executes the program compiled from check_mem_leaks.c. If having another executable for the test is needed along with another script to run it, that is OK. The test would need to be added to the configure.ac and the correct CMake file, though.

brarcher · 2017-08-27T19:08:32Z

doc/check.texi

+(up to 4GB allocations were seen in the wild).
+To prevent this, a limit is placed on the assertion message size.
+This limit is 4K bytes by default.
+It can be modified by setting the @code(CK_MAX_MSG_SIZE) environment variable,


I think that you may need {} around @code.

That is:
@code{CK_MAX_MSG_SIZE}

orenbenkiki · 2017-08-27T19:34:41Z

Working on the tests; I see test/check_mem_leaks in the tests/Makefile.am, so I can add the new tests there. I don't see them listed anywhere else though, in particular in any of the CMakeLists.txt file. Is adding the new tests to tests/Makefile.am sufficient?

Edit: I see test_log_output.sh in the CMakeLists.txt file, I'll see if I can build on that. Strange that it seems that cmake doesn't run the memory leak check though...?

Edit 2: Added to both CMake and Automake tests.

brarcher · 2017-08-28T00:48:09Z

tests/check_env_max_msg_size.c

+ */
+
+/**
+ * The purpose of this test is to be used by valgrind to check for


May need to update this comment

brarcher · 2017-08-28T00:48:32Z

tests/check_set_max_msg_size.c

+ */
+
+/**
+ * The purpose of this test is to be used by valgrind to check for


Same here, the comment needs to be updated

brarcher · 2017-08-28T00:49:19Z

tests/check_set_max_msg_size.c

+     * Run the test suite. This is intended to trigger the "Message is too long" error.
+     * Actual success/failure is determined by examining the output.
+     */
+    check_set_max_msg_size(32); // This will have an effect due to no environment variable.


The two test programs could be collapsed by passing in a value over the command line, as I think that is the only difference, correct?

brarcher · 2017-08-28T00:52:07Z

tests/test_env_max_msg_size.sh

@@ -0,0 +1,22 @@
+#!/usr/bin/env sh


The two test scripts could be collapsed, and simply run the unit test executable multiple times and check the result.

brarcher · 2017-08-28T00:54:09Z

doc/check.texi

@@ -993,6 +993,17 @@ repetitive code that is hard to read.  For your convenience Check
 provides a set of functions (actually macros) for testing often used
 conditions.

+@findex check_set_max_msg_size


I think this could be moved to the end of the section, as it would not be expected to be called as frequently as the others. Or, do you disagree?

I think that placing it at the end would make it "disappear" for someone who would say "ah, a list of all kind of assertions, I'll skip to the next section and come back here if I want to locate a specific assertion function". The other option is to make it an explicit section of its own but that seems an overkill for a single simple function.

brarcher · 2017-08-28T01:57:36Z

Jenkins: ok to test
^ will start the remaining tests

brarcher · 2017-08-28T02:04:59Z

Strange that it seems that cmake doesn't run the memory leak check though...?

The valgrind test is specific to Linux. The original intention of CMake was to support Windows targets, so it did not make much sense to add it. It turns out that some users on Linux do want to use CMake as well.

Regardless, the unit tests are really to help protect the quality of the project. As long as the project still runs the tests on changes, I think being supported in one or the other build system should not be that important.

orenbenkiki · 2017-08-28T09:24:33Z

Fixed everything (except for moving the documentation paragraph).

Edit: Oops, didn't push the latest. Did now.

orenbenkiki · 2017-08-29T17:02:47Z

Ready to merge - or is there anything else?

brarcher · 2017-08-29T21:35:04Z

src/check_pack.c

 * The usual size for a message is less than 80 bytes.
 * All this is done instead of the previous approach to allocate (actually
 * continuously reallocate) one big chunk for the whole message stream.
 * Problems were seen in the wild with up to 4 GB reallocations.
 */

+void check_set_max_msg_size(int max_msg_size) {
+    if (ck_max_msg_size <= 0) {


Is it that once the value has been set it cannot be changed? The comment in the prototype says this is the intent: "if used... must be called once...". I am fine with the behavior, just curious as to the rationale.

I was probably too focused on a specific use case. Fixed.

brarcher · 2017-08-29T21:35:40Z

src/check_pack.c

 * The usual size for a message is less than 80 bytes.
 * All this is done instead of the previous approach to allocate (actually
 * continuously reallocate) one big chunk for the whole message stream.
 * Problems were seen in the wild with up to 4 GB reallocations.
 */

+void check_set_max_msg_size(int max_msg_size) {


Small thing, but can you match the style of the { in this file, namely:

void check_set_max_msg_size(int max_msg_size() { ...

brarcher · 2017-08-29T21:36:57Z

src/check_pack.c

+}
+
+static int get_max_msg_size(void) {
+    if (ck_max_msg_size <= 0)


Why is this necessary? Should not the setter verify that the value is sane?

Is there a reason that ck_max_msg_size must be signed? Is it valid for it to be negative? Could it be unsigned?

Changed type to size_t, which is probably the "right" type for a size.

orenbenkiki · 2017-08-30T17:06:14Z

Changed type of argument to size_t, allow modifying the size at any point, and fixed the style.

brarcher · 2017-08-31T02:25:16Z

src/check_pack.c

-        check_set_max_msg_size(0);
+static size_t get_max_msg_size(void)
+{
+    if (ck_max_msg_size == 0)      // If check_set_max_msg_size was not called,


Should not the getter be making the decision as to if the environment variable exists or not, and let the setter only set the value passed? This way, if the environment variable changed since when the setter was called and the getter was called the correct value would be returned? e.g.:

void check_set_max_msg_size(size_t max_msg_size) { ck_max_msg_size = max_msg_size; } void get_max_msg_size() { size_t value = 0; char *env = getenv("CK_MAX_MSG_SIZE"); if (env) value = (size_t)strtoul(env, NULL, 10); // Cast in case size_t != unsigned long. if(value == 0) value = ck_max_msg_size; if (value == 0) value = DEFAULT_MAX_MSG_SIZE; return value; }

Changing the environment variable at run time seems strange, but I guess it could happen. Also, I guess performance isn't an issue, as this is only used when an assertion fails...

…environment variable.

orenbenkiki · 2017-08-31T17:26:25Z

Fixed.

brarcher · 2017-09-01T12:35:50Z

Thanks for the changeset! I've merge it in.

If you would like, you can submit a merge request adding an entry into the AUTHORS file for yourself.

Allow dynamic configuration of maximal assertion message size.

cffe75c

brarcher reviewed Aug 27, 2017

View reviewed changes

Document the new function, give precedence to environment.

bb91bc2

brarcher reviewed Aug 27, 2017

View reviewed changes

Fix @code braces.

223d74e

Add tests for modifying maximal message size.

db50316

brarcher reviewed Aug 28, 2017

View reviewed changes

orenbenkiki added 2 commits August 28, 2017 07:52

Merge check max msg size tests, fix comments.

8280e55

Have tests clean up the log file on success.

46f62ae

Merge test scripts as well, fixed comments.

4a9d595

brarcher reviewed Aug 29, 2017

View reviewed changes

Use size_t for maximal message size, allow it to be modified.

59775ce

brarcher reviewed Aug 31, 2017

View reviewed changes

Allow modifying the maximal message size by dynamically changing the …

253678a

…environment variable.

Merge branch 'master' into master

1cb4d56

brarcher merged commit fb5408b into libcheck:master Sep 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow dynamic configuration of maximal assertion message size. #128

Allow dynamic configuration of maximal assertion message size. #128

orenbenkiki commented Aug 27, 2017

brarcher Aug 27, 2017

brarcher Aug 27, 2017

brarcher commented Aug 27, 2017

orenbenkiki commented Aug 27, 2017

brarcher commented Aug 27, 2017

brarcher Aug 27, 2017

brarcher Aug 27, 2017

orenbenkiki commented Aug 27, 2017 •

edited

Loading

brarcher Aug 28, 2017

brarcher Aug 28, 2017

brarcher Aug 28, 2017

brarcher Aug 28, 2017

brarcher Aug 28, 2017

orenbenkiki Aug 28, 2017

brarcher commented Aug 28, 2017

brarcher commented Aug 28, 2017

orenbenkiki commented Aug 28, 2017 •

edited

Loading

orenbenkiki commented Aug 29, 2017

brarcher Aug 29, 2017

orenbenkiki Aug 30, 2017

brarcher Aug 29, 2017

orenbenkiki Aug 30, 2017

brarcher Aug 29, 2017

orenbenkiki Aug 30, 2017

orenbenkiki commented Aug 30, 2017

brarcher Aug 31, 2017

orenbenkiki Aug 31, 2017

orenbenkiki commented Aug 31, 2017

brarcher commented Sep 1, 2017

Allow dynamic configuration of maximal assertion message size. #128

Allow dynamic configuration of maximal assertion message size. #128

Conversation

orenbenkiki commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brarcher commented Aug 27, 2017

orenbenkiki commented Aug 27, 2017

brarcher commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbenkiki commented Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brarcher commented Aug 28, 2017

brarcher commented Aug 28, 2017

orenbenkiki commented Aug 28, 2017 • edited Loading

orenbenkiki commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbenkiki commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbenkiki commented Aug 31, 2017

brarcher commented Sep 1, 2017

orenbenkiki commented Aug 27, 2017 •

edited

Loading

orenbenkiki commented Aug 28, 2017 •

edited

Loading