keepsimple1 / mdns-sd

Rust library for mDNS based Service Discovery
Apache License 2.0
88 stars 38 forks source link

query TYPE_A and TYPE_AAAA via Command::Resolve #185

Closed hrzlgnm closed 3 months ago

hrzlgnm commented 3 months ago

This fixes issues with resolving records published by python-zeroconf or systemd-resolved without an actual A or AAAA address assigned.

Closes #182

Manually tested on linux and windows successfully

hrzlgnm commented 3 months ago

Thanks for coming up with the fix! Good work! I have two comments:

1. Should we use the existing `Command::Resolve` to send this query? The reason is that it is possible that the responses came in more than 1 `DnsIncoming` datagram. This will give it a chance to process completely.  It will also handle retry.

I opened PR #186 to show what I meant.

2. As an optimization, we can refactor `send_query` to accept more than 1 type so that we can ask more than 1 questions in the packet. So we can reduce the network traffic. (We  / I can do this later, just a comment here)

Regarding 1. yes that makes more sense to me as we wouldn't retry endlessly if we never receive a reply.

Regarding 2: yes makes sense to send multiple queries in one packet to reduce network traffic.

keepsimple1 commented 3 months ago

Regarding 1. yes that makes more sense to me as we wouldn't retry endlessly if we never receive a reply.

Regarding 2: yes makes sense to send multiple queries in one packet to reduce network traffic.

I think at least two options we can do:

a) Merge your current changes in this PR and then I will follow up with changes using the other PR. And will need your help to verify your use case still works.

b) You can update this PR with suggestion changes (particularly 1) and we merge it.

I'm good with either approaches. Let me know what you think?

hrzlgnm commented 3 months ago

I'd rather do b to avoid the unclean intermediate state

hrzlgnm commented 3 months ago

I'll update my pr after work

hrzlgnm commented 3 months ago

Update the PR and retested my 2 cases successfully on windows and linux.

hrzlgnm commented 3 months ago

And thank you very much for the explanatory PR, it was very educational :+1: