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

Limit length of metadata decode debug output #19043

Closed
wants to merge 2 commits into from

Conversation

wickerwaka
Copy link

Limit the size of the raw metadata that gets printed when debug logging is enabled.

@wickerwaka wickerwaka mentioned this pull request Nov 17, 2014
@alexcrichton
Copy link
Member

r? @pnkfelix

let mut buf = String::new();
let end = match max {
Copy link
Member

Choose a reason for hiding this comment

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

  1. You haven't documented anywhere this special handing of max == 0; (I personally would rather not treat 0 as a special case, and instead pass in max: Option<uint>, but reasonable people can differ on this sort of detail.) It might be a good idea to note the special case in a note above DATA_TRUNCATE.
  2. More important: since every call to data_log_string is passing in DATA_TRUNCATE, and data_log_string is private to this module, why make it a parameter at all? Why not just use DATA_TRUNCATE directly in data_log_string ?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I changed my mind; I am fine with making the cap a parameter to data_log_string as you did.

@pnkfelix
Copy link
Member

r=me, though when you rebase, please consider adding a comment with the DATA_TRUNCATE definition as noted in my earlier review comment.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

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.

3 participants