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

F/remove msg error etc null sentinel #941

Merged

Conversation

bkil-syslogng
Copy link
Contributor

  • msg_error("description", evt_tag_str("when", "before"), NULL);
  • msg_error("description", evt_tag_str("when", "after"));

Contains a sed moster and some vararg magic.

@MrAnno
Copy link
Collaborator

MrAnno commented Feb 26, 2016

👍

@bkil-syslogng bkil-syslogng force-pushed the f/remove-msg-error-etc-null-sentinel branch from 1f6f85a to 0955b85 Compare March 2, 2016 12:00
@bkil-syslogng
Copy link
Contributor Author

Rebased to master

@bazsi
Copy link
Collaborator

bazsi commented Mar 2, 2016

I like the change in general, even though I didn't have too much time to
check how it works, and neither did I review the individual call sites.

Bazsi

On Wed, Mar 2, 2016 at 1:59 PM, Tamas Nagy notifications@github.com wrote:

Rebased to master


Reply to this email directly or view it on GitHub
#941 (comment).

@bkil-syslogng bkil-syslogng force-pushed the f/remove-msg-error-etc-null-sentinel branch 2 times, most recently from 057e117 to aabcd6d Compare March 23, 2016 15:15
@bkil-syslogng
Copy link
Contributor Author

Rebased to master.

Fixed conflicts by dropping the last commit and reapplying the script described in the commit message.

@ihrwein
Copy link
Contributor

ihrwein commented Apr 12, 2016

👍 (Could you rebase it again? Thanks!)

@bkil-syslogng bkil-syslogng force-pushed the f/remove-msg-error-etc-null-sentinel branch 2 times, most recently from 6ca8ceb to fe48068 Compare April 12, 2016 10:11
Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>
Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>
Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>
find . -iname .git -prune -false -o -type f -exec \
 sed --regexp-extended --silent --in-place --expression "
 1h
 1! H
 $ {
  g
 s~(\<msg_(fatal|error|warning|notice|info|progress|verbose|debug|trace|warning_once)\
\s*\([^;]*),\s*NULL\s*\)\s*[;]~\1);~g
 p
}
" {} +

Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>
@bkil-syslogng bkil-syslogng force-pushed the f/remove-msg-error-etc-null-sentinel branch from fe48068 to f36548b Compare April 12, 2016 10:55
@bkil-syslogng
Copy link
Contributor Author

@ihrwein Rebased to master. No conflict or omission detected when running the transformation this time.

@bazsi
Copy link
Collaborator

bazsi commented Apr 13, 2016

I am afraid I've just merged code that will not be adjusted for this. I would merge this again and fix up the fallout. The change is trivial, makes sense it's just scattered around.

Do you have a script to run on the "http" destination?

@bazsi bazsi merged commit 3ca4f76 into syslog-ng:master Apr 13, 2016
@bkil-syslogng
Copy link
Contributor Author

@bazsi You can re-run the commit message as many times as you'd like. Do take care to remove any extra space if your tools show messages that way. Actually I wrapped a long sed line in the middle, that's the (only) one you need to consider.

I did give this a second thought few days ago, and we may consider saving this under scripts/ and eventually connect it with our Pull Request examination suite in a way that a bot would notify if you used an extra ,NULL. It's mostly a matter of whether the git diff after the transformation is empty or not. In the long run, we could probably connect a bunch of custom LLVM analysis to catch more important typos/common errors as well.

@bkil-syslogng
Copy link
Contributor Author

@bazsi I ran it on the HTTP module. You can check and merge the result and the script file in #1021.

bkil-syslogng added a commit to bkil-syslogng/syslog-ng that referenced this pull request Apr 13, 2016
msg_error("description", evt_tag_str("when", "before"), NULL);
msg_error("description", evt_tag_str("when", "after"));

This was the automated transformation done in this pull request:
syslog-ng#941 "F/remove msg error etc null sentinel"

f36548b
"dropped extra NULL sentinel at msg_*() call sites"

It was copied from the commit message verbatim, but it may be improved
in the future.

Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>
@bazsi
Copy link
Collaborator

bazsi commented Apr 15, 2016

I don't think we need to add this validation from this point on. We don't need it from now period, it may happen that some as-of-now out-of-tree code contains the extra NULL, but we won't write new ones.

@bazsi
Copy link
Collaborator

bazsi commented Apr 21, 2016

thanks a lot and sorry for breaking things. probably not a good idea to
keep PRs out-of-tree for weeks as the chance of stuff breaking when merging
them in improper order is a lot larger.

Bazsi

On Wed, Apr 13, 2016 at 4:23 PM, Tamas Nagy notifications@github.com
wrote:

@bazsi https://github.com/bazsi I ran it on the HTTP module. You can
check and merge the result and the script file in #1021
#1021.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#941 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants