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/11/27 03:21:49 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #2415: igmp: bypass MULTICAST_LOOP/TTL options
anchao opened a new pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415
## Summary
igmp: bypass MULTICAST_LOOP/TTL options
## Impact
optional configurations not mandatory
## Testing
IGMP setsockopt
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#discussion_r536580737
##########
File path: net/inet/ipv4_setsockopt.c
##########
@@ -184,16 +189,41 @@ int ipv4_setsockopt(FAR struct socket *psock, int option,
}
break;
- /* The following IPv4 socket options are defined, but not implemented */
-
- case IP_MULTICAST_IF: /* Set local device for a multicast
- * socket */
case IP_MULTICAST_TTL: /* Set/read the time-to-live value of
* outgoing multicast packets */
+ {
+ if (psock->s_type != SOCK_DGRAM ||
+ value_len != sizeof(int))
Review comment:
For Linux for most of these options both signed int and unsigned char are supported and they just dereference and cast the value to in in the case of unsigned char to a signed int.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo commented on pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-739333692
IGMP is an IPv4 protocol. MLD is the IPv6 counterpart. Should MLD also support these settings?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] anchao commented on pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-739136461
Thanks for your comments,
> @anchao - See above what was the verification done to test this?
Hi, @davids5
I have verified the relevant functions of UDP IGMP locally, it works as expected. :)
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #2415: igmp: bypass MULTICAST_LOOP/TTL options
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-735216219
@xiaoxiang781216 so Xiaomi is using some ported lib that needs this, and the solution is to fake it? Why not do the right thing and implement it? Or at a minimum Kconfig it defaulted to error-ing out, with a Knob to fake the result? Things like this as landmines that can cause huge time syncs for the next poor person that steps on it.
@patacongo @antmerlino - any opinion on this?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#discussion_r536575892
##########
File path: net/inet/ipv4_setsockopt.c
##########
@@ -184,16 +189,41 @@ int ipv4_setsockopt(FAR struct socket *psock, int option,
}
break;
- /* The following IPv4 socket options are defined, but not implemented */
-
- case IP_MULTICAST_IF: /* Set local device for a multicast
- * socket */
case IP_MULTICAST_TTL: /* Set/read the time-to-live value of
* outgoing multicast packets */
+ {
+ if (psock->s_type != SOCK_DGRAM ||
+ value_len != sizeof(int))
Review comment:
bsd and as well as tldp and ltp all use uchar
```
The IP_MULTICAST_TTL option changes the time-to-live (TTL) for outgoing
multicast datagrams in order to control the scope of the multicasts:
u_char ttl; /* range: 0 to 255, default = 1 */
setsockopt(s, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl));
```
The linux man 7 ip page is unfortunate because it seems to be playing loose with the word `integer`
```
IP_MULTICAST_TTL (since Linux 1.2)
Set or read the time-to-live value of outgoing multicast pack‐
ets for this socket. It is very important for multicast pack‐
ets to set the smallest TTL possible. The default is 1 which
means that multicast packets don't leave the local network un‐
less the user program explicitly requests it. Argument is an
integer.
```
However since there own test suite uses `uchar` and every usage of it I have seen does as well I think it is clear.
https://github.com/linux-test-project/ltp/blob/master/testcases/network/multicast/mc_opts/mc_verify_opts.c
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#discussion_r536648421
##########
File path: net/inet/ipv4_setsockopt.c
##########
@@ -184,16 +189,41 @@ int ipv4_setsockopt(FAR struct socket *psock, int option,
}
break;
- /* The following IPv4 socket options are defined, but not implemented */
-
- case IP_MULTICAST_IF: /* Set local device for a multicast
- * socket */
case IP_MULTICAST_TTL: /* Set/read the time-to-live value of
* outgoing multicast packets */
+ {
+ if (psock->s_type != SOCK_DGRAM ||
+ value_len != sizeof(int))
Review comment:
Looks like you dropped the range check which is important here. If someone sends and int >255 that is invalid and should error instead of truncate.
0 is also invalid unless loopback is enabled and then it is only sent on the loopback. We do not support multicast_loop so it is always invalid.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#discussion_r536597831
##########
File path: net/inet/ipv4_setsockopt.c
##########
@@ -184,16 +189,41 @@ int ipv4_setsockopt(FAR struct socket *psock, int option,
}
break;
- /* The following IPv4 socket options are defined, but not implemented */
-
- case IP_MULTICAST_IF: /* Set local device for a multicast
- * socket */
case IP_MULTICAST_TTL: /* Set/read the time-to-live value of
* outgoing multicast packets */
+ {
+ if (psock->s_type != SOCK_DGRAM ||
+ value_len != sizeof(int))
Review comment:
I applied this change and the ttl of 20 that I requested is set (instead of the default 64). I also verified the signed int with a different value.
```c
diff --git a/net/inet/ipv4_setsockopt.c b/net/inet/ipv4_setsockopt.c
index 995b750b11..22e19f7860 100644
--- a/net/inet/ipv4_setsockopt.c
+++ b/net/inet/ipv4_setsockopt.c
@@ -193,14 +193,23 @@ int ipv4_setsockopt(FAR struct socket *psock, int option,
* outgoing multicast packets */
{
if (psock->s_type != SOCK_DGRAM ||
- value_len != sizeof(int))
+ !(value_len == sizeof(int) ||
+ value_len == sizeof(uint8_t)))
{
ret = -EINVAL;
}
else
{
FAR struct udp_conn_s *conn;
- int ttl = *(FAR int *)value;
+ int ttl;
+ if (value_len == sizeof(int))
+ {
+ ttl = *(FAR int *)value;
+ }
+ else
+ {
+ ttl = (int)*(FAR uint8_t *)value;
+ }
if (ttl <= 0 || ttl > 255)
{
```
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #2415: igmp: bypass MULTICAST_LOOP/TTL options
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-734863712
Is this a Hack? This code change, changes the functionality from returning not supported to falsely returning that the functionality is supported but does nothing.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#discussion_r536635561
##########
File path: net/inet/ipv4_setsockopt.c
##########
@@ -184,16 +189,41 @@ int ipv4_setsockopt(FAR struct socket *psock, int option,
}
break;
- /* The following IPv4 socket options are defined, but not implemented */
-
- case IP_MULTICAST_IF: /* Set local device for a multicast
- * socket */
case IP_MULTICAST_TTL: /* Set/read the time-to-live value of
* outgoing multicast packets */
+ {
+ if (psock->s_type != SOCK_DGRAM ||
+ value_len != sizeof(int))
Review comment:
Hi @btashton
You are right. I just checked the linux source code and found that they have added processing of different data types.
https://github.com/torvalds/linux/blob/b3298500b23f0b53a8d81e0d5ad98a29db71f4f0/net/ipv4/ip_sockglue.c#L923-L932
I made a new change according to this part, please review it again, thank you!
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2415: igmp: bypass MULTICAST_LOOP/TTL options
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-734919971
> Is this a Hack? This code change, changes the functionality from returning not supported to falsely returning that the functionality is supported but does nothing.
@davids5 these options control the minor behaviour of group broadcast(e.g. time to live value). Where support these options doesn't generate the huge difference, it isn't too bad to return OK here to let the program run successfully. The similar approach use for open too:
```
#define O_SYNC (1 << 7) /* Synchronize output on write */
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
```
We define these flags in fcntl.h, which mean user can open file with these flags, but none of NutX filesystem support any of them.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton merged pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
btashton merged pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#discussion_r536557076
##########
File path: net/inet/ipv4_setsockopt.c
##########
@@ -184,16 +189,41 @@ int ipv4_setsockopt(FAR struct socket *psock, int option,
}
break;
- /* The following IPv4 socket options are defined, but not implemented */
-
- case IP_MULTICAST_IF: /* Set local device for a multicast
- * socket */
case IP_MULTICAST_TTL: /* Set/read the time-to-live value of
* outgoing multicast packets */
+ {
+ if (psock->s_type != SOCK_DGRAM ||
+ value_len != sizeof(int))
Review comment:
This is not correct. I should be able to use `unsigned char ttl;` as the input to set the ttl.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-738171291
@patacongo - Would you please see it this looks correct to you?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] anchao commented on pull request #2415: igmp: bypass MULTICAST_LOOP/TTL options
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-735241753
> @xiaoxiang781216 so Xiaomi is using some ported lib that needs this, and the solution is to fake it? Why not do the right thing and implement it? Or at a minimum Kconfig it defaulted to error-ing out, with a Knob to fake the result? Things like this as landmines that can cause huge time syncs for the next poor person that steps on it.
>
> @patacongo @antmerlino - any opinion on this?
Hi @davids5 ,
Could you please remove the nouns like xiaomi? It makes me feel uncomfortable, this is just a technical discussion, do not rise to benefit, back to the PR, if the implementation of these options completely block the results of application, I will definitely consider the priority implementation before raise this PR. But in fact, TTL and LOOP are not mandatory parts in use of IGMP multicast case. Of course, I totally agree with your insistence, I will consider implement this feature, please keep this PR, thanks.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo commented on pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-738817826
>
>
> @patacongo - Would you please see it this looks correct to you?
I don't see anything incorrect but I cannot verify that it is correct by looking at 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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2415: igmp: bypass MULTICAST_LOOP/TTL options
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-734919971
> Is this a Hack? This code change, changes the functionality from returning not supported to falsely returning that the functionality is supported but does nothing.
@davids5 these options control the minor behaviour of group broadcast(e.g. time to live value). Where support these options doesn't generate the huge difference, it isn't too bad to return OK here to let the program run successfully. The similar approach use for open too:
```
#define O_SYNC (1 << 7) /* Synchronize output on write */
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
```
We define these flags in fcntl.h, but none of NutX filesystem support any of these flags, but nobody return fail when user open file with these flags.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] acassis commented on pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-737975012
> > @xiaoxiang781216 so Xiaomi is using some ported lib that needs this, and the solution is to fake it? Why not do the right thing and implement it? Or at a minimum Kconfig it defaulted to error-ing out, with a Knob to fake the result? Things like this as landmines that can cause huge time syncs for the next poor person that steps on it.
> > @patacongo @antmerlino - any opinion on this?
>
> Hi @davids5 ,
>
> Could you please remove the nouns like xiaomi? It makes me feel uncomfortable, this is just a technical discussion, do not rise to benefit, back to the PR, if the implementation of these options completely block the results of application, I will definitely consider the priority implementation before raise this PR. But in fact, TTL and LOOP are not mandatory parts in use of IGMP multicast case. Of course, I totally agree with your insistence, I will consider implement this feature, please keep this PR, thanks.
I agree, don't use a person's commit to generalize things about the company "modus operandi". But yes, @davids5 is right to enforce a complete solution than a quick hack to make things working.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-738914131
@anchao - See above what was the verification done to test this?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #2415: net/igmp: add MULTICAST_TTL support
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2415:
URL: https://github.com/apache/incubator-nuttx/pull/2415#issuecomment-739247139
@btashton - Thank you for for looking into this. Please merge it when you are satisfied, as my review will not add any value.
----------------------------------------------------------------
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