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

mdnsd: add handling for legacy queries #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rthakur33
Copy link
Contributor

Following are taken care of:

  • in the reply to a legacy query, send unicast udp to same port from which the packet is received.
  • copy the query from question message in reply.
  • use the same transaction id as in question.

Fixes bug FS #3

@rthakur33
Copy link
Contributor Author

I have tested this against CDRouter and test mdns_10 and mdns_12 which verify response to legacy querier pass after this change.

@Rondom
Copy link

Rondom commented Dec 10, 2024

This needs rebasing

Following are taken care of:
- in the reply to a legacy query, send unicast udp to same port
  from which the packet is received.
- copy the query from question message in reply.
- use the same transaction id as in question.
@rthakur33 rthakur33 force-pushed the handle_legacy_queries branch from bb45f8d to c57805f Compare December 11, 2024 05:55
@rthakur33
Copy link
Contributor Author

This needs rebasing

done

@Rondom
Copy link

Rondom commented Dec 11, 2024

@ynezz @blogic @nbd168 Gentle ping: Does anyone of you have the bandwith to look at this PR and other PRs for umdns?

@blogic
Copy link
Contributor

blogic commented Dec 11, 2024

@rthakur33 can you split this PR up into smaller patches to make it easier to review please

@rthakur33
Copy link
Contributor Author

@rthakur33 can you split this PR up into smaller patches to make it easier to review please

@blogic , my apologies for the way the patch looks at the moment, but to copy the query in the response message and use the transaction id of the query, the query needs to be passed down to the point where the answer is constructed. This commit basically does that. hence, I think unfortunately the commit needs to look the way it looks right now.

the only part I can take out into another PR is related to sending the unicast udp. It would not make reviewing simpler by a great deal but let me know if you prefer that. I can do that.

@ynezz
Copy link
Member

ynezz commented Dec 12, 2024

First of all thanks a lot for taking care of this, nice work!

the only part I can take out into another PR

From https://openwrt.org/submitting-patches#separate_your_changes

Separate logical changes into a single patch file (commit):

In other words, the request is about splitting of this single commit:

Following are taken care of:
- in the reply to a legacy query, send unicast udp to same port
  from which the packet is received.
- copy the query from question message in reply.
- use the same transaction id as in question.

Into possibly 3 (or more) separate commits, each being about a single topic, do not mix several topics in one huge commit, it makes review tedious.

Additionally you describe the changes in the code, you don't need to do this, its already visible from the diff, you should explain to a competent reader why you made this commit/changes.

So usually ask yourself:

  • Why I've made the changes?
  • What is currently wrong and needs to be fixed?

and put answer to this questions in the commit description.

So in this case, you can for example state, that umdns currently doesn't pass One-Shot Multicast DNS Queries CDRouter tests, because umdns currently doesn't support this legacy mode and thus sends bogus 0x0000 response transaction ID in the response and those tests fail.

You can for example quote relevant parts from the RFCs 6.7 Legacy Unicast Responses so its crystal clear, what is currently wrong and why are this changes needed, so anyone who is reviewing this can cross check the intended changes with the RFC etc. without actually looking this up. The more details, the better.

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.

4 participants