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 2020/03/31 04:43:01 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #664:
Dns ipv6 dual stack support(part 2)
xiaoxiang781216 opened a new pull request #664: Dns ipv6 dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #664:
Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400750168
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
i'm afraid that changing a structure which is a part of standard api this way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #664: Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400925553
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
> if h_addr_list contains both of ipv4 and ipv6 addresses, an app expecting the standard behavior (thus only look at h_length/h_addrtype) would break.
Yes, if the caller touch the non first address which has the different type from the first one. But the problem already exist there even without my modification:
https://github.com/apache/incubator-nuttx/pull/664/files#diff-b38c46bf06ff1cf66969bedcd951db60L404
https://github.com/apache/incubator-nuttx/pull/664/files#diff-b38c46bf06ff1cf66969bedcd951db60L564
My patch doesn't introduce the new problem but just extend h_length/h_addrtype to express the fact(IPv4/IPv6 combination) correctly. If this case is a concern need to fix. It's better to add a patch on top of mine to remove the different IP address from the list.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #664: Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400806446
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
> i'm afraid that changing a structure which is a part of standard api this way.
It's hard to return both IPv4 and IPv6 in one hostent, since hostent has only one h_addrtype/h_length field. The modification extend h_addrtype/h_length to array by following h_addr_list/h_addr design pattern, and provide macro to keep the backward compatibility. The most caller work fine with the new struct and macro.
To keep the compatibility, caller shouldn't directly touch h_lengths/h_addrtypes fields, they must call the new getaddrinfo/freeaddrinfo API instead, like this:
https://github.com/apache/incubator-nuttx-apps/pull/151
https://github.com/apache/incubator-nuttx-apps/pull/153
The array extension is used internally to support getaddrinfo:
https://github.com/apache/incubator-nuttx/pull/664/files#diff-cf02c526db0b2c6580f921a4083b5ce8R290
This method is simplest approach with some backword compatibility what I can think.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #664:
Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400903458
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
i don't see what's wrong with calling gethostbyname2 twice. it's certainly better than breaking gethostbyname.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 closed pull request #664: Dns the
dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #664: Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400943409
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
Ok, I will add an internal struct and funciton to return the full list.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #664: Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400909761
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
> i don't see what's wrong with calling gethostbyname2 twice. it's certainly better than breaking gethostbyname.
But gethostbyname isn't broken, many caller in apps still work as before without any modification. The beahviour is also same as before.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #664: Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400871412
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
> as gethostbyname etc should not return hostent containing multiple types anyway, there's little point to change struct hostent.
> for getaddrinfo, it's better to implement gethostbyname2 and use it.
I also consider this solution before but:
1.getaddrinfo need call gethostbyname2 twice.
2.dns cache hold the name to address(multiple entries) mapping. It's hard to manage the dns cache if we split IPv4/IPv6 query into two call.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #664:
Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400915780
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
if h_addr_list contains both of ipv4 and ipv6 addresses, an app expecting the standard behavior (thus only look at h_length/h_addrtype) would break.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #664:
Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400853636
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
as gethostbyname etc should not return hostent containing multiple types anyway, there's little point to change struct hostent.
for getaddrinfo, it's better to implement gethostbyname2 and use it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #664:
Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400940666
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
IMO,
* if it's already the case, it should be fixed.
* it isn't a good idea to rely on the bug. especially when it involves standard violation.
if you need a libc-internal non-standard gethostbyname-like interface, why don't you just introduce it with another name?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #664: Dns the dual stack support(part 2)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #664: Dns the dual stack support(part 2)
URL: https://github.com/apache/incubator-nuttx/pull/664#discussion_r400871412
##########
File path: include/netdb.h
##########
@@ -184,14 +184,17 @@ struct hostent
FAR char **h_aliases; /* A pointer to an array of pointers to the
* alternative host names, terminated by a
* null pointer. */
- int h_addrtype; /* Address type. */
- int h_length; /* The length, in bytes, of the address. */
+ FAR int *h_addrtypes; /* A pointer to an array of address type. */
+ FAR int *h_lengths; /* A pointer to an array of the length, in bytes,
+ * of the address. */
FAR char **h_addr_list; /* A pointer to an array of pointers to network
* addresses (in network byte order) for the host,
* terminated by a null pointer. */
};
#define h_addr h_addr_list[0] /* For backward compatibility */
+#define h_length h_lengths[0]
+#define h_addrtype h_addrtypes[0]
Review comment:
> as gethostbyname etc should not return hostent containing multiple types anyway, there's little point to change struct hostent.
> for getaddrinfo, it's better to implement gethostbyname2 and use it.
I also consider this solution before but:
1.getaddrinfo need call gethostbyname2 twice.
2.dns cache hold the name to address(multiple entries) mapping. It's hard to manage the dns cache if we split IPv4/IPv6 query into two call.
3.gethostbyname2 isn't POSIX too.
If we want to implement getaddrinfo on top of gethostbyname or gethostbyname2, the current method is the most clean one.
Another way is to implement gethostbyname on top of getaddrinfo, but the change is very huge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services