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

value pairs bugfixes #110

Merged
merged 4 commits into from
Mar 27, 2014
Merged

value pairs bugfixes #110

merged 4 commits into from
Mar 27, 2014

Conversation

gdani
Copy link
Contributor

@gdani gdani commented Mar 14, 2014

No description provided.

@bazsi
Copy link
Collaborator

bazsi commented Mar 15, 2014

Unit tests have failed with this patch. Also can you pls give some rationale to the patch? What it fixes exactly?

@algernon
Copy link
Contributor

The rationale is that when using value-pairs() scopes, DATE was different between everything and base: the former was the $DATE macro, the latter $R_DATE. They should be the same, preferably $DATE itself, not $R_DATE.

The other change is for the rfc5424 scope, which had $SDATA, but not the $.SDATA.* stuff.

(As far as I remember - we'll look at the test failures too, they went through internally...)

@algernon
Copy link
Contributor

Ah, yes, the first commit is to fix #105, for reference.

Formerly it contained sometimes the stamp time and sometimes the
received time. From now it will always contain the stamp time.

Signed-off-by: Daniel Gados <gdani@balabit.hu>
Signed-off-by: Daniel Gados <gdani@balabit.hu>
@gdani
Copy link
Contributor Author

gdani commented Mar 17, 2014

The patches were picked from syslog-ng PE 5.1 development code where the syslog-format works slightly differently than in OSE (the sdata part is constructed differently) and the macro-set is different too, thus the newly written unit tests comparing the exact output and aiming to test the discovered new bugs failed to work in OSE.
The failing unit tests have been changed to work in OSE.

@bazsi
Copy link
Collaborator

bazsi commented Mar 18, 2014

Regarding DATE, R_DATE S_DATE pls read the documentation about the deprecated use_time_recvd setting and 11.1.3 and make sure we are creating a consistent behavior.

@gdani
Copy link
Contributor Author

gdani commented Mar 19, 2014

By the patches included in this pull request the meaning of the DATE the macro does not change, neither any of the prefixed DATE macros (ie. S_DATE, R_DATE, C_DATE) in accordance with the syslog-ng OSE 3.5 Administrator Guide section 11.1.3 and with the syslog-ng Premium Edition 5 LTS Administrator Guide section 13.1.3.

The fixes' aim was to ensure that the value-pairs expand correctly and consistently.

Formerly the value-pairs scope named rfc3164 contained the DATE macro with an alias to R_DATE resulting that syslog-ng expanded value-pairs differently using different scopes:

  • with scope everything the DATE expanded to DATE which is the same as S_DATE
  • with scope rfc3164 the DATE expanded to R_DATE because of the alias added to the definition (see lib/value-pairs.c line 96)

With one of the above mentioned patches using value-pairs this behaviour changes so that the DATE expands to the value of the DATE macro (the same as S_DATE) in all cases regardless which scope the user wishes to use.

Formerly it was expaned only to nvtable entries, but from now it
expands to macros as well without setting the scope.

Signed-off-by: Daniel Gados <gdani@balabit.hu>
Signed-off-by: Daniel Gados <gdani@balabit.hu>
@bazsi
Copy link
Collaborator

bazsi commented Mar 19, 2014

All right, then merging this is up to @algernon

@algernon
Copy link
Contributor

Took a bit of time to get back to these changes, but they all look good to me, so merge coming up! Thanks a lot for the fixes!

algernon added a commit that referenced this pull request Mar 27, 2014
@algernon algernon merged commit 123b014 into syslog-ng:master Mar 27, 2014
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

3 participants