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

Format option and Logfmt support #74

Merged
merged 2 commits into from
Oct 8, 2022
Merged

Format option and Logfmt support #74

merged 2 commits into from
Oct 8, 2022

Conversation

Unactived
Copy link
Contributor

This PR adds an optional --format option that defaults to the current console format, and adds a logfmt logger with it.

Partially addresses #3

@Frky
Copy link
Member

Frky commented Oct 6, 2022

Thank you for the PR.
To reply to the question in #73, it is of course very fine by us if you want to PR something into masscanned, and even more if it addresses an issue (you or someone else has), so feel free.

Just two remarks before it get merged:

Other than that, I'd be happy to merge your PR.

Copy link
Member

@Frky Frky left a comment

Choose a reason for hiding this comment

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

Could you run cargo fmt on your code?

@Unactived
Copy link
Contributor Author

* not to be picky, but is there a reason for `et` instead of `eth` for `ethernet` in the key you chose (for instance, but not only)? Is it to be compliant with a tool that ingest logfmt files? Or is it a personal choice?

I personally have no problem with it being eth or whatever else. (I guess some might appreciate having only one instance of lowercase eth though).
I actually meant it as short for EtherType (acronym ET, but staying lowercase), which is the name of the field.

@Unactived
Copy link
Contributor Author

I think I'll change et to eth_type which is indeed clearer without looking at the code or thinking about the protocol's details.

@Unactived Unactived requested a review from Frky October 6, 2022 08:08
@Frky Frky merged commit 0fbdbaa into ivre:master Oct 8, 2022
@Frky
Copy link
Member

Frky commented Oct 8, 2022

Thank you for the changes (and for the PR once again).

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.

2 participants