F/unset value

[bazsi]

This branch implements the unset() rewrite operation and at the same time incorporates a number of cleanups on the serialization code.

I also have another 20 patches to further the cleanup and to introduce a number of performance improvements of the same, but I've figured it is easier to break them in to to facilitate review.

@juhaszviktor as the original author of the serialization code, could you pls help me in reviewing this?

Attn: @csabamajor

[bazsi]

The mentioned followup patches can be found here:

https://github.com/balabit/syslog-ng/tree/f/logmsg-serialize-performance

I am submitting a pull request once this goes in.

[juhaszviktor]

In short, LGTM 👍
I really liked reading patches and I think the result is very good, Thanks!
I've just some (sub)minor notes, but in generally it get my +1.

We have to investigate why the travis job fails, but at first look, it is independent from your modifications

[juhaszviktor]

I think these checks, validations should be in separated static inline functions, with good names and in that case we have not need for the comments.
But this is a very minor note

[bazsi]

I tend to agree with this one, however these checks are all interdependent, so I felt this one is easier to follow than to extract conditions in separate functions.

We basically check if the pointer points below or after the designated area.

[lbudai]

On tomorrow I'll will also review it.
@bazsi: will you need some help from the team to make Travis happy?

[bkil]

@bazsi
grep 'error' in copyright log:

error: mismatch lib/logmsg/tests/test_nvtable.c expected:LGPLv2.1+_SSL detected:GPLv2+_SSL
error: mismatch lib/logmsg/tests/test_tags.c expected:LGPLv2.1+_SSL detected:GPLv2+_SSL
error: mismatch lib/value-pairs/tests/test_value_pairs.c expected:LGPLv2.1+_SSL detected:GPLv2+_SSL
error: mismatch lib/value-pairs/tests/test_value_pairs_walk.c expected:LGPLv2.1+_SSL detected:GPLv2+_SSL

I opted to increase the verbosity variable recently, but if this is confusing, we can reduce it again so it only grep's errors.

grep -i ' error ' in distcheck:

../lib/logmsg/nvtable.c: In function ‘nv_table_get_entry_slow’:
../lib/logmsg/nvtable.c:215:17: error: declaration of ‘index’ shadows a global declaration [-Werror=shadow]
../lib/logmsg/nvtable.c: In function ‘nv_table_reserve_table_entry’:
../lib/logmsg/nvtable.c:259:21: error: declaration of ‘index’ shadows a global declaration [-Werror=shadow]
../lib/logmsg/nvtable.c: In function ‘nv_table_foreach_entry’:
../lib/logmsg/nvtable.c:588:17: error: declaration of ‘index’ shadows a global declaration [-Werror=shadow]

In file included from ../lib/logmsg/nvtable-serialize.c:28:0:
../lib/logmsg/nvtable-serialize-endianutils.h: In function ‘nv_table_data_swap_bytes’:
../lib/logmsg/nvtable-serialize-endianutils.h:72:17: error: declaration of ‘index’ shadows a global declaration [-Werror=shadow]
../lib/logmsg/nvtable-serialize-endianutils.h: In function ‘nv_table_struct_swap_bytes’:
../lib/logmsg/nvtable-serialize-endianutils.h:99:17: error: declaration of ‘index’ shadows a global declaration [-Werror=shadow]
../lib/logmsg/nvtable-serialize.c: In function ‘_deserialize_dynamic_entries’:
../lib/logmsg/nvtable-serialize.c:311:17: error: declaration of ‘index’ shadows a global declaration [-Werror=shadow]
../lib/logmsg/nvtable-serialize.c: In function ‘_write_struct’:
../lib/logmsg/nvtable-serialize.c:474:17: error: declaration of ‘index’ shadows a global declaration [-Werror=shadow]

grep ' error' in cmake:

Building C object modules/native/CMakeFiles/syslog-ng-native-connector.dir/native-parser.c.o
../lib/libsyslog-ng.so.3.8.alpha0�[32mBuilding C object modules/native/CMakeFiles/syslog-ng-native-connector.dir/parser.c.o
: undefined reference to `log_rewrite_unset_new'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [syslog-ng-ctl/syslog-ng-ctl] Error 1
make[1]: *** [syslog-ng-ctl/CMakeFiles/syslog-ng-ctl.dir/all] Error 2

../lib/libsyslog-ng.so.3.8.alpha0: undefined reference to `log_rewrite_unset_new'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

We should change make -j install to make -j install || make install for cmake to make errors more readible.

The make distcheck log is not clear because the return codes got mixed up, that's why it does not retry without make -j. Basically we need to either put the make V=1; inside the $S=42 branch above, or switch the return code generation branches inside exec_prop_check(). Actually, I originally intended it to behave like the latter solution, i.e., code 42 = propagation error, code not in [0,42] = build failure return code.

I can submit PRs next week if nobody else volunteers until then.

[bkil]

@bkil-syslogng We need to || cat test-suite.log @ distcheck similar to bkil-syslogng#23

[ihrwein]

The CMake related error is fixed by this commit: 15d5e2a

[bkil-syslogng]

Clarification of build log errors in Travis added here: #1116

[lbudai]

@bkil-syslogng : that patch should not be part of a MacOSX support branch
Would it be possible to send a separate PR?
(and a note to the travis.yaml: it started to have a smell... )

[bkil-syslogng]

@lbudai It was also mentioned by @bazsi last week that we should start to save Travis scripts to separate scripts. I agree if that's what you mean. I think we have too many unprocessed pull requests already, but if you insist I could create more.

[bazsi]

I have now pushed an update to this branch to fix the warnings in travis. For some reason I don't get these shadowing warnings even with -Wshadow enabled. I am not sure why though, maybe a change in gcc versions?

gcc (Ubuntu 5.2.1-22ubuntu2) 5.2.1 20151010

... hmm.. it seems that gcc 4.8 changed the meaning of -Wshadow:

The option -Wshadow no longer warns if a declaration shadows a function declaration, unless the former declares a function or pointer to function, because this is a common and valid case in real-world code.

This seems to be a lot more practical. Travis is stuck on an earlier gcc?

[bkil-syslogng]

@bazsi I noticed this earlier as well. Updated gcc variants are available even for container Travis, so I propose that we should eventually upgrade.

I originally planned to add new matrix elements, so that we compile on both earlier and newer compilers, but if you think that we can throw out older compilers to ease Travis' resource usage, I'm all in for it.

I've separated the clarification of the build log from Mac OS X support, so we can leave that one floating for some more months:
#1122

[bazsi]

for warning checks yeah. we still need to support old versions to compile
though.

Bazsi

On Fri, Jul 15, 2016 at 11:13 AM, Tamas Nagy notifications@github.com
wrote:

@bazsi https://github.com/bazsi I noticed this earlier as well. Updated
gcc variants are available even for container Travis, so I propose that we
should eventually upgrade.

I originally planned to add new matrix elements, so that we compile on
both earlier and newer compilers, but if you think that we can throw out
older compilers to ease Travis' resource usage, I'm all in for it.

I've separated the clarification of the build log from Mac OS X support,
so we can leave that one floating for some more months:
#1122 #1122

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1108 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AArldpcPABJgTQCiB7sxdc0DoPilaX1xks5qV08zgaJpZM4JEL3e
.

[bazsi]

I think this patchset is now set. I have now rebased it to master, and once it is green by travis, I am going ahead and merge it, based on the 👍 by @juhaszviktor above.

Let me know if you have objections.