You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/08/12 07:38:20 UTC
[GitHub] [incubator-nuttx] zhhyu7 opened a new pull request, #6841: dns_client: directly initialize g_dns_servers and remove dns_initiali…
zhhyu7 opened a new pull request, #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841
…ze directly
g_dns_servers need init before dns_query, Otherwise sim can not add default
dns server when use usrsock mode to share host network.
Avoid similar problems in the future, so directly initialize g_dns_servers.
Signed-off-by: zhanghongyu <zh...@xiaomi.com>
## Summary
## Impact
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6841: dns_client: directly initialize g_dns_servers and remove dns_initialize directly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841#discussion_r945274292
##########
libs/libc/netdb/lib_dnsaddserver.c:
##########
@@ -45,8 +45,41 @@
#ifndef CONFIG_NETDB_RESOLVCONF
/* The DNS server addresses */
-union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS];
-uint8_t g_dns_nservers; /* Number of currently configured nameservers */
+union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS] =
+ {
+#if defined(CONFIG_NETDB_DNSSERVER_IPv4)
+ {
+ .ipv4.sin_family = AF_INET,
Review Comment:
> Yes, I understand that. I do not know what is the best solution. I mean that there is a rule C89 rule for common code, but it is violated quite heavily. I've sked this in the mailing list, but didn't found a solid answer. So C89 compatibility seems to be not a rule, but a semi-rule as it is violated when it is "convenient". I personally do not like semi-rules.
>
> Particularly here reordering of a union members may be applied to get C89 initialization.
>
Could you give more instruction here? I think the reorder can't fix the ambiguousness for union's field.
> I'm fine to keep C89 init if other people find this acceptable
The initialization is very fragile. Actually, netdb code is in the broken state just because an unrelated change: https://github.com/apache/incubator-nuttx/pull/6770.
We need fix the root problem to avoid it break again in the furture.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #6841: dns_client: directly initialize g_dns_servers and remove dns_initialize directly
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6841: dns_client: directly initialize g_dns_servers and remove dns_initialize directly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841#discussion_r945274292
##########
libs/libc/netdb/lib_dnsaddserver.c:
##########
@@ -45,8 +45,41 @@
#ifndef CONFIG_NETDB_RESOLVCONF
/* The DNS server addresses */
-union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS];
-uint8_t g_dns_nservers; /* Number of currently configured nameservers */
+union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS] =
+ {
+#if defined(CONFIG_NETDB_DNSSERVER_IPv4)
+ {
+ .ipv4.sin_family = AF_INET,
Review Comment:
> Yes, I understand that. I do not know what is the best solution. I mean that there is a rule C89 rule for common code, but it is violated quite heavily. I've sked this in the mailing list, but didn't found a solid answer. So C89 compatibility seems to be not a rule, but a semi-rule as it is violated when it is "convenient". I personally do not like semi-rules.
>
stickling to C89 is just for the very old compiler. Since netdb is an optional component, I doubt that whether network can run this old platform.
> Particularly here reordering of a union members may be applied to get C89 initialization.
>
Could you give more instruction here? I think the reorder can't fix the ambiguousness for union's field.
> I'm fine to keep C89 init if other people find this acceptable
The initialization is very fragile. Actually, netdb code is in the broken state just because an unrelated change: https://github.com/apache/incubator-nuttx/pull/6770.
We need fix the root problem to avoid it break again in the furture.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6841: dns_client: directly initialize g_dns_servers and remove dns_initialize directly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841#discussion_r945275543
##########
libs/libc/netdb/lib_dnsaddserver.c:
##########
@@ -45,8 +45,41 @@
#ifndef CONFIG_NETDB_RESOLVCONF
/* The DNS server addresses */
-union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS];
-uint8_t g_dns_nservers; /* Number of currently configured nameservers */
+union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS] =
+ {
+#if defined(CONFIG_NETDB_DNSSERVER_IPv4)
+ {
+ .ipv4.sin_family = AF_INET,
Review Comment:
Yeah... My bad. The union fields rely on `CONFIG_NET_IPvN`, but init code on `CONFIG_NETDB_DNSSERVER_IPvN`, so reordering will not help here. If those rely on the same config parameters then we could reorder members to get needed members in the first place and use C89 init. Unfortunately that is not the case here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6841: dns_client: directly initialize g_dns_servers and remove dns_initialize directly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841#discussion_r944657295
##########
libs/libc/netdb/lib_dnsaddserver.c:
##########
@@ -45,8 +45,41 @@
#ifndef CONFIG_NETDB_RESOLVCONF
/* The DNS server addresses */
-union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS];
-uint8_t g_dns_nservers; /* Number of currently configured nameservers */
+union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS] =
+ {
+#if defined(CONFIG_NETDB_DNSSERVER_IPv4)
+ {
+ .ipv4.sin_family = AF_INET,
Review Comment:
It's very hard to keep C89 since g_dns_servers's type is union.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6841: dns_client: directly initialize g_dns_servers and remove dns_initialize directly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841#discussion_r945275543
##########
libs/libc/netdb/lib_dnsaddserver.c:
##########
@@ -45,8 +45,41 @@
#ifndef CONFIG_NETDB_RESOLVCONF
/* The DNS server addresses */
-union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS];
-uint8_t g_dns_nservers; /* Number of currently configured nameservers */
+union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS] =
+ {
+#if defined(CONFIG_NETDB_DNSSERVER_IPv4)
+ {
+ .ipv4.sin_family = AF_INET,
Review Comment:
Yeah... My bad. The union fields rely on `CONFIG_NET_IPvN`, but init code on `CONFIG_NETDB_xxxx`, so reordering will not help here. If those rely on the same config parameters then we could reorder members to get needed members in the first place and use C89 init. Unfortunately that is not the case here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6841: dns_client: directly initialize g_dns_servers and remove dns_initialize directly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841#discussion_r945287428
##########
libs/libc/netdb/lib_dnsaddserver.c:
##########
@@ -45,8 +45,41 @@
#ifndef CONFIG_NETDB_RESOLVCONF
/* The DNS server addresses */
-union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS];
-uint8_t g_dns_nservers; /* Number of currently configured nameservers */
+union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS] =
+ {
+#if defined(CONFIG_NETDB_DNSSERVER_IPv4)
+ {
+ .ipv4.sin_family = AF_INET,
Review Comment:
So, I would suggest that we accept a little violation here until we find a better initialization method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6841: dns_client: directly initialize g_dns_servers and remove dns_initialize directly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841#discussion_r944644641
##########
libs/libc/netdb/lib_dnsaddserver.c:
##########
@@ -45,8 +45,41 @@
#ifndef CONFIG_NETDB_RESOLVCONF
/* The DNS server addresses */
-union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS];
-uint8_t g_dns_nservers; /* Number of currently configured nameservers */
+union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS] =
+ {
+#if defined(CONFIG_NETDB_DNSSERVER_IPv4)
+ {
+ .ipv4.sin_family = AF_INET,
Review Comment:
It would be good if we can keep C89 compatible init
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6841: dns_client: directly initialize g_dns_servers and remove dns_initialize directly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6841:
URL: https://github.com/apache/incubator-nuttx/pull/6841#discussion_r945273034
##########
libs/libc/netdb/lib_dnsaddserver.c:
##########
@@ -45,8 +45,41 @@
#ifndef CONFIG_NETDB_RESOLVCONF
/* The DNS server addresses */
-union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS];
-uint8_t g_dns_nservers; /* Number of currently configured nameservers */
+union dns_addr_u g_dns_servers[CONFIG_NETDB_DNSSERVER_NAMESERVERS] =
+ {
+#if defined(CONFIG_NETDB_DNSSERVER_IPv4)
+ {
+ .ipv4.sin_family = AF_INET,
Review Comment:
Yes, I understand that. I do not know what is the best solution. I mean that there is a rule C89 rule for common code, but it is violated quite heavily. I've sked this in the mailing list, but didn't found a solid answer. So C89 compatibility seems to be not a rule, but a semi-rule as it is violated when it is "convenient". I personally do not like semi-rules.
Particularly here reordering of a union members may be applied to get C89 initialization.
I'm fine to keep C89 init if other people find this acceptable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org