Skip to content

Conversation

@ximon18
Copy link
Member

@ximon18 ximon18 commented Nov 14, 2024

See fuzz/README.md.

Uses NLnetLabs/domain#441.

@ximon18 ximon18 requested a review from a team November 14, 2024 10:41
@ximon18 ximon18 mentioned this pull request Nov 14, 2024
Copy link
Contributor

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Looks good, but I suggest that we keep this unmerged until we can make this depend on the main branch of domain.

@ximon18
Copy link
Member Author

ximon18 commented Nov 14, 2024

Looks good, but I suggest that we keep this unmerged until we can make this depend on the main branch of domain.

It now depends on the main branch of domain.

@tertsdiepraam
Copy link
Contributor

Revived! Maybe a controversial change is to make the stelline feature no longer optional, but that seemed like the easiest way to do this right now since cargo-fuzz doesn't do dev-dependencies. Another solution would be to have a stelline feature or something like that, but that makes testing less nice too.

@tertsdiepraam tertsdiepraam requested a review from a team April 15, 2025 10:11
@ximon18
Copy link
Member Author

ximon18 commented May 22, 2025

Revived! Maybe a controversial change is to make the stelline feature no longer optional, but that seemed like the easiest way to do this right now since cargo-fuzz doesn't do dev-dependencies. Another solution would be to have a stelline feature or something like that, but that makes testing less nice too.

@tertsdiepraam: I'm afraid I don't understand what is left to do here, the PR seems to work for me in its current form.

"tsig",
"unstable-client-transport",
"unstable-sign",
"unstable-stelline",
Copy link
Contributor

Choose a reason for hiding this comment

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

@ximon18 this is the "controversial" change. If you agree with this then its fine with me to merge it.

Copy link
Member Author

@ximon18 ximon18 May 22, 2025

Choose a reason for hiding this comment

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

I see.

The fuzz binary uses FakeEnv which causes this problem.

Could another way be to impl Env for FuzzEnv where FuzzEnv doesn't use Stelline (and so wouldn't be suitable for running dnst update or dnst notify unless some non-Stelline based impl of Env::dgram() and Env::stub_resolver_from_confg() would be possible)?

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should do ldns-testns. If so then we will get stelline as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you referring to #56?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. We may need to rethink the name and design for the dnst version. But it seems a useful feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting idea... :-)

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