FreeRTOS / FreeRTOS-Plus-TCP

FreeRTOS-Plus-TCP library repository. +TCP files only. Submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
MIT License
125 stars 149 forks source link

Compile error with DNS cache disabled [BUG] #1128

Closed evpopov closed 2 months ago

evpopov commented 3 months ago

Pretty much what the title says:

To Reproduce

#define ipconfigUSE_DNS                   ipconfigENABLE
#define ipconfigUSE_DNS_CACHE             ipconfigDISABLE 

I get an error: struct <anonymous>' has no member named 'ucName'

Skptak commented 3 months ago

Hey @evpopov, thanks for submitting this bug. When I get a minute I'll hop on a linux machine and set up the tests with this config to duplicate this issue and get back to you!

evpopov commented 3 months ago

Thanks @Skptak, I believe it's a very simple fix, but I did not want to speculate and rush to conclusions.

Skptak commented 3 months ago

Hey @evpopov, I opened #1131 to fix this issue. I've messaged @tony-josi-aws about this as well.

evpopov commented 3 months ago

@Skptak First of all I'd like to thank you for looking into this and creating a PR. I really appreciate it! My first quick-and-dirty fix would have been different than what you did in #1131 and I'm happy I didn't rush to creating a PR on a topic I don't understand well.

Just to let you know, I did apply your changes from #1131 to my test machine where I have multiple DUTs that resolve each other by name and everything seems to be running fine. Don't get me wrong, I trust your changes and wasn't expecting any issues, but I though another test wouldn't hurt.

I was about to close this issue, but I'd like to ask something before I do. Maybe you could educate me a little because I'm not that familiar with FreeRTOS_DNS.c Could you tell me what pxAddrInfo->ai_canonname and pxAddrInfo->xPrivateStorage.ucName are intended to be used for? I looked around and I don't seem to find any use for them. Are they for a feature that was planned but never finished? Maybe I'm missing some configuration scenario where they get utilized? As a quick test, I completely removed those fields and all code associated with them (which is actually just the 3 lines you touched in #1131 ) and the stack seems to function just fine without them. Any ideas if we rally need those fields?

Again, Thanks for your time and insights.

Skptak commented 2 months ago

My first quick-and-dirty fix would have been different than what you did in https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/pull/1131

Hey @evpopov, not an issue that you'd have a different fix for this than I did. In the future feel free to open a PR up with your proposed changes, the team can always discuss the benefits and downsides of an approach in the PR!

Just to let you know, I did apply your changes from https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/pull/1131 to my test machine where I have multiple DUTs that resolve each other by name and everything seems to be running fine.

I'm glad to know that the fix worked for you! Thanks for testing it on your local setup as well!

As a quick test, I completely removed those fields and all code associated with them (which is actually just the 3 lines you touched in https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/pull/1131 )

I believe that pxAddrInfo->xPrivateStorage.ucName holds the string name of the DNS lookup. @tony-josi-aws or @htibosch could probably validate this. Where removing these lines should still work, just potentially cause a DNS lookup to need to occur when connecting to the address.

tony-josi-aws commented 2 months ago

Could you tell me what pxAddrInfo->ai_canonname and pxAddrInfo->xPrivateStorage.ucName are intended to be used for?

I believe during the DNS cache lookup, for example, in the FreeRTOS_ProcessDNSCache if a valid ppxAddressInfo is provided, it copies all the DNS entries (for example, if ipconfigDNS_CACHE_ADDRESSES_PER_ENTRY > 1) along with ai_canonname as a linked struct addrinfo. Currently, FreeRTOS_dnslookup or FreeRTOS_dnslookup6 doesn't seem to use it (ppxAddressInfo is set to NULL).

evpopov commented 2 months ago

Thanks for the explanation @tony-josi-aws