applications: slm: try to continue even no DNS record was sent #19701
Conversation
github-actions
bot
added
the
doc-required
|
Thank you for your contribution! Note: This comment is automatically posted and updated by the Contribs GitHub Action. |
TaeZStkyoht
force-pushed
the
fix_dns_records_expectation
branch
2 times, most recently
from
53d856f to
bfb7fc6
Compare
TaeZStkyoht
changed the title
| if (ret) { | ||
| return ret; | ||
| } | ||
| pdn_dynamic_params_get_v6(PDP_CID, NULL, NULL, &mtu); |
There was a problem hiding this comment.
I don't think that the error code should be swallowed in a case where there are no DNS-addresses provided. Instead the functions should take into account that this is a possibility and indicate which DNS-addresses were successfully fetched.
There was a problem hiding this comment.
But normally, I passed NULL to these parameters. It means this code block in slm_ppp.c already doesn't care about whether dns count 1 or 2. I think this is also related to next review point below that you wrote. With current implementation maybe I can check with error code and allow to continue whether return value is zero or -EBADMSG. Is it ok? Or let's discuss first the next review point below.
There was a problem hiding this comment.
Now I'm checking return value to allow to continue on ppp establishment if it's zero or -EBADMSG.
| @@ -643,8 +643,10 @@ int pdn_dynamic_params_get(uint8_t cid, struct in_addr *dns4_pri, | |||
| } | |||
| } | |||
| if (dns4_sec) { | |||
| if (zsock_inet_pton(AF_INET, dns4_sec_str, dns4_sec) != 1) { | |||
There was a problem hiding this comment.
Could dns4_pri (and dns4_sec) be used to indicate that DNS address was not found, if it was asked? Setting the pointers to null is likely asking for trouble, but the bytes could be set to 0, to indicate no address. Another possibility would be to return how many addresses we found (or to add out parameters to indicate what we found).
Same for IPv6.
There was a problem hiding this comment.
Which one is logical? I can do any of these.
There was a problem hiding this comment.
Ok I made it with setting to 0. Now dns4_pri, dns4_sec, ipv4_mtu, dns6_pri, dns6_sec, ipv6_mtu will be containing zeros.
There was a problem hiding this comment.
This would at least allow the caller to check if the address was fetched successfully. However, I am starting to lean on having a separate function for getting MTU and for this to return the amount of DNS addresses that were found. But let's wait for libmodem comments before proceeding with any of this.
| if (matched >= 2) { | ||
| if (zsock_inet_pton(AF_INET, dns4_sec_str, dns4_sec) != 1) { | ||
| return -EADDRNOTAVAIL; | ||
| } | ||
| } | ||
| } | ||
| if (ipv4_mtu) { |
There was a problem hiding this comment.
This requires that matched == 3, this will fail to get the mtu with the current changes. We should take into account how many addresses were actually found and get the correct mtu.
Same for IPv6.
There was a problem hiding this comment.
I didn't understand why will it fail to get mtu. I check >= because it can contain both 2 and 3 case. 2 means only DNS addresses were fetched. 3 means DNS addresses + MTU were fetched. Could you explain me more?
There was a problem hiding this comment.
My original assumption was that scanf returned all the matches, so one DNS address and MTU would still result in matched = 2. Which would then fail to set the ipv4_mtu as there were less matches than intended. This would then have failed the subsequent matched check == 3. Unfortunately my assumption was wrong
Actually scanf returns only the matches up to the first non-matched value. So with one DNS address and MTU, would resut in matched = 1. This is problematic as what we really need in this case is the MTU and either one, two or even zero DNS addresses.
I don't think that this is necessarily possible with the current implementation and it is possible that we would need to have a separate method for fetching MTU. This needs a comment from libmodem, so please do not start implementing this at this point.
TaeZStkyoht
force-pushed
the
fix_dns_records_expectation
branch
from
bfb7fc6 to
2cd13a7
Compare
We shouldn't expect 2 DNS records from ISP. Because it sends sometimes 1 and even 0. It must try to continue like in MOSH. Signed-off-by: Oguzhan Turk <[email protected]>
TaeZStkyoht
force-pushed
the
fix_dns_records_expectation
branch
from
2cd13a7 to
d1fa9c0
Compare
We shouldn't expect 2 DNS records from ISP. Because it sends sometimes 1 and even 0. We must just try to continue to establish PPP connection.
Real world scenario:
ISP sends DNS records as:
MOSH was already working fine with allowing like that I've implemented here for SLM. We were using MOSH in at_cmd_mode on our product. However we want to use SLM; because, SLM is an application while MOSH is a sample. And we want to also be able to access GPIO command to swtich sim/eSim from outside easily. SLM is capable of doing it while MOSH not.
If this change is really directly not ok for you, I can also make it configurable with KConfig. By default can be false and we can set it to true on our project. Nevertheless, MOSH works like that, why not SLM?