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

Invalid clearing of additional records #34

Closed
eshvatskyi opened this issue Nov 5, 2018 · 8 comments
Closed

Invalid clearing of additional records #34

eshvatskyi opened this issue Nov 5, 2018 · 8 comments

Comments

@eshvatskyi
Copy link
Contributor

if (response.Answers.Any(a => a.Name == ServiceName))

Should be

           // Many bonjour browsers don't like DNS-SD response
            // with additional records.
            if (response.Answers.Any(a => a.Name == ServiceName))
            {
               if (AnswersContainsAdditionalRecords)
               {
                   response.Answers.AddRange(response.AdditionalRecords);
               }
                response.AdditionalRecords.Clear();
            }
@richardschneider
Copy link
Owner

I agree, but the if (AnswersContainsAddditionalRecords) should be before the DNS-SD test.

Would you like to create a PR. This way you will get credit as a contributor!

@eshvatskyi
Copy link
Contributor Author

I do, ) I have 1 optimization for DNS
and fix for mdns (using with multiple NIC)

@eshvatskyi
Copy link
Contributor Author

Please let me know how I could create a PR

@richardschneider
Copy link
Owner

@eshvatskyi
Copy link
Contributor Author

eshvatskyi commented Nov 6, 2018

Lol )) I meant I dont have permissions ) to do this, should I fork it? or how to do this better?

@richardschneider
Copy link
Owner

Just make a fork of my repo. I noticed that you have done this but your fork is not current; just delete and re-fork. Make your changes. Finally you can submit by clicking new pull request.

You do not need any permissions on my repo.

@eshvatskyi
Copy link
Contributor Author

eshvatskyi commented Nov 6, 2018

Created 2 PRs, please check. not perfect ) but tested on multy Nics works perfect on windows 10 host.
Later will test on linux alpine. Missed implementation for AnswerAllQuestions, will add in case you interested in my PR

@richardschneider
Copy link
Owner

Does #40 resolve this issue?

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

No branches or pull requests

2 participants