| Message ID | 1644038899-15433-2-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/3] avformat/udp: use one setsockopt for ipv4/ipv6 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On Sat, 5 Feb 2022, lance.lmwang@gmail.com wrote: > From: Limin Wang <lance.lmwang@gmail.com> > > Suggested by zhilizhao, vlc project has solved the compatibility by > the same way, so I borrowed the comments from vlc project. > > Fix #ticket9449 > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavformat/udp.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 3dc79eb..34488d6 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -164,6 +164,10 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > { > int protocol, cmd; > > + /* There is some confusion in the world whether IP_MULTICAST_TTL > + * takes a byte or an int as an argument. > + * BSD seems to indicate byte so we are going with that and use > + * int as a fallback to be safe */ The code does the opposite. Tries int and falls back to byte. Either change code or change comment. Regards, Marton > switch (addr->sa_family) { > #ifdef IP_MULTICAST_TTL > case AF_INET: > @@ -183,8 +187,15 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > } > > if (setsockopt(sockfd, protocol, cmd, &mcastTTL, sizeof(mcastTTL)) < 0) { > - ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > - return ff_neterrno(); > + /* BSD compatibility */ > + unsigned char ttl; > + > + ff_log_net_error(logctx, AV_LOG_DEBUG, "setsockopt"); > + ttl = (unsigned char)(( mcastTTL > 255 ) ? 255 : mcastTTL); > + if (setsockopt(sockfd, protocol, cmd, &ttl, sizeof(ttl)) < 0) { > + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > + return ff_neterrno(); > + } > } > > return 0; > -- > 1.8.3.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On Sat, Feb 05, 2022 at 10:59:55AM +0100, Marton Balint wrote: > > > On Sat, 5 Feb 2022, lance.lmwang@gmail.com wrote: > > > From: Limin Wang <lance.lmwang@gmail.com> > > > > Suggested by zhilizhao, vlc project has solved the compatibility by > > the same way, so I borrowed the comments from vlc project. > > > > Fix #ticket9449 > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > --- > > libavformat/udp.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > > index 3dc79eb..34488d6 100644 > > --- a/libavformat/udp.c > > +++ b/libavformat/udp.c > > @@ -164,6 +164,10 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > > { > > int protocol, cmd; > > > > + /* There is some confusion in the world whether IP_MULTICAST_TTL > > + * takes a byte or an int as an argument. > > + * BSD seems to indicate byte so we are going with that and use > > + * int as a fallback to be safe */ > > The code does the opposite. Tries int and falls back to byte. Either change > code or change comment. Yes, good catch, so vlc use wrong comments. I'll change comments like below. + * BSD seems to indicate byte so we are going with that and tries + * int and falls back to byte to be safe */ > > Regards, > Marton > > > switch (addr->sa_family) { > > #ifdef IP_MULTICAST_TTL > > case AF_INET: > > @@ -183,8 +187,15 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > > } > > > > if (setsockopt(sockfd, protocol, cmd, &mcastTTL, sizeof(mcastTTL)) < 0) { > > - ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > > - return ff_neterrno(); > > + /* BSD compatibility */ > > + unsigned char ttl; > > + > > + ff_log_net_error(logctx, AV_LOG_DEBUG, "setsockopt"); > > + ttl = (unsigned char)(( mcastTTL > 255 ) ? 255 : mcastTTL); > > + if (setsockopt(sockfd, protocol, cmd, &ttl, sizeof(ttl)) < 0) { > > + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > > + return ff_neterrno(); > > + } > > } > > > > return 0; > > -- > > 1.8.3.1 > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Since any [valid] value over 255 is impossible in the IPv4 protocol (the TTL field is only 8-bits), it should always be capped at 255 (for consistency) or return an invalid value error (the one I would suggest). Despite VLC's reversed comment, using an int seems to be the exception to the rule (i.e. only linux and windows seem to use it [as-documented]), perhaps doing the unsigned char first and using the int as the fallback would be better? It's not really just a BSD thing, unless you also count LWIP and Solaris as BSD. Unless VLC's code history shows them doing it this way at one time and swapping it (but forgetting the comment) to fix a known bug? On 2/4/2022 9:28 PM, lance.lmwang@gmail.com wrote: > From: Limin Wang <lance.lmwang@gmail.com> > > Suggested by zhilizhao, vlc project has solved the compatibility by > the same way, so I borrowed the comments from vlc project. > > Fix #ticket9449 > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavformat/udp.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 3dc79eb..34488d6 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -164,6 +164,10 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > { > int protocol, cmd; > > + /* There is some confusion in the world whether IP_MULTICAST_TTL > + * takes a byte or an int as an argument. > + * BSD seems to indicate byte so we are going with that and use > + * int as a fallback to be safe */ > switch (addr->sa_family) { > #ifdef IP_MULTICAST_TTL > case AF_INET: > @@ -183,8 +187,15 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > } > > if (setsockopt(sockfd, protocol, cmd, &mcastTTL, sizeof(mcastTTL)) < 0) { > - ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > - return ff_neterrno(); > + /* BSD compatibility */ > + unsigned char ttl; > + > + ff_log_net_error(logctx, AV_LOG_DEBUG, "setsockopt"); > + ttl = (unsigned char)(( mcastTTL > 255 ) ? 255 : mcastTTL); > + if (setsockopt(sockfd, protocol, cmd, &ttl, sizeof(ttl)) < 0) { > + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > + return ff_neterrno(); > + } > } > > return 0;
On Sat, Feb 05, 2022 at 01:26:18PM -0800, Chad Fraleigh wrote: > Since any [valid] value over 255 is impossible in the IPv4 protocol (the TTL field is only 8-bits), it should always be capped at 255 (for consistency) or return an invalid value error (the one I would suggest). > zhilizhao have submit a patch to limit the range of ttl from option. Do you want to return an invalid error here still? > Despite VLC's reversed comment, using an int seems to be the exception to the rule (i.e. only linux and windows seem to use it [as-documented]), perhaps doing the unsigned char first and using the int as the fallback would be better? It's not really just a BSD thing, unless you also count LWIP and Solaris as BSD. Unless VLC's code history shows them doing it this way at one time and swapping it (but forgetting the comment) to fix a known bug? > I have blamed vlc code and sure the code doing it this way at one time(104938796a3). For the mismatch of code and comments, I prefer to code always as code were build and used by all kinds of system which vlc is supported already. As for use BSD, I prefer to count LWIP and Solaris into BSD category which using rule of byte. If you still prefer to add them into comments, I'm OK also. > > On 2/4/2022 9:28 PM, lance.lmwang@gmail.com wrote: > > From: Limin Wang <lance.lmwang@gmail.com> > > > > Suggested by zhilizhao, vlc project has solved the compatibility by > > the same way, so I borrowed the comments from vlc project. > > > > Fix #ticket9449 > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > --- > > libavformat/udp.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > > index 3dc79eb..34488d6 100644 > > --- a/libavformat/udp.c > > +++ b/libavformat/udp.c > > @@ -164,6 +164,10 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > > { > > int protocol, cmd; > > > > + /* There is some confusion in the world whether IP_MULTICAST_TTL > > + * takes a byte or an int as an argument. > > + * BSD seems to indicate byte so we are going with that and use > > + * int as a fallback to be safe */ > > switch (addr->sa_family) { > > #ifdef IP_MULTICAST_TTL > > case AF_INET: > > @@ -183,8 +187,15 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > > } > > > > if (setsockopt(sockfd, protocol, cmd, &mcastTTL, sizeof(mcastTTL)) < 0) { > > - ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > > - return ff_neterrno(); > > + /* BSD compatibility */ > > + unsigned char ttl; > > + > > + ff_log_net_error(logctx, AV_LOG_DEBUG, "setsockopt"); > > + ttl = (unsigned char)(( mcastTTL > 255 ) ? 255 : mcastTTL); > > + if (setsockopt(sockfd, protocol, cmd, &ttl, sizeof(ttl)) < 0) { > > + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > > + return ff_neterrno(); > > + } > > } > > > > return 0; > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sun, 6 Feb 2022, lance.lmwang@gmail.com wrote: > On Sat, Feb 05, 2022 at 01:26:18PM -0800, Chad Fraleigh wrote: >> Since any [valid] value over 255 is impossible in the IPv4 protocol (the TTL field is only 8-bits), it should always be capped at 255 (for consistency) or return an invalid value error (the one I would suggest). >> > > zhilizhao have submit a patch to limit the range of ttl from option. Do you want > to return an invalid error here still? I have applied the patch, so capping is no longer needed. > > >> Despite VLC's reversed comment, using an int seems to be the exception >> to the rule (i.e. only linux and windows seem to use it >> [as-documented]), perhaps doing the unsigned char first and using the >> int as the fallback would be better? It's not really just a BSD thing, >> unless you also count LWIP and Solaris as BSD. Unless VLC's code >> history shows them doing it this way at one time and swapping it (but >> forgetting the comment) to fix a known bug? >> > > I have blamed vlc code and sure the code doing it this way at one time(104938796a3). > For the mismatch of code and comments, I prefer to code always as code were build > and used by all kinds of system which vlc is supported already. > Yeah, I agree, if we are copying VLC approach then probably it makes more sense to use the same order as they do in their code. Thanks, Marton
On 2/5/2022 6:09 PM, lance.lmwang@gmail.com wrote: > On Sat, Feb 05, 2022 at 01:26:18PM -0800, Chad Fraleigh wrote: >> Since any [valid] value over 255 is impossible in the IPv4 protocol (the TTL field is only 8-bits), it should always be capped at 255 (for consistency) or return an invalid value error (the one I would suggest). >> > > zhilizhao have submit a patch to limit the range of ttl from option. Do you want > to return an invalid error here still? If it can never be called with an invalid value, not even programmatically if someone links against the ffmpeg libs, then checking it is unneeded. But also checking it to limit the unsigned char value would be redundant, so only the value cast would be needed, i.e.: ttl = (unsigned char) mcastTTL; If however, it could be called without being first limited, then returning an error would be best to avoid silently having unexpected results. Also, checking that it isn't negative should be done in that case. Not counting pending patches, I only see udp_open() calls it, so if it's already bound in there, no extra checks are needed. Of course, these are only suggestions, since I'm a nobody. =) >> Despite VLC's reversed comment, using an int seems to be the exception to the rule (i.e. only linux and windows seem to use it [as-documented]), perhaps doing the unsigned char first and using the int as the fallback would be better? It's not really just a BSD thing, unless you also count LWIP and Solaris as BSD. Unless VLC's code history shows them doing it this way at one time and swapping it (but forgetting the comment) to fix a known bug? >> > > I have blamed vlc code and sure the code doing it this way at one time(104938796a3). > For the mismatch of code and comments, I prefer to code always as code were build > and used by all kinds of system which vlc is supported already. > > As for use BSD, I prefer to count LWIP and Solaris into BSD category which using > rule of byte. If you still prefer to add them into comments, I'm OK also. > >> >> On 2/4/2022 9:28 PM, lance.lmwang@gmail.com wrote: >>> From: Limin Wang <lance.lmwang@gmail.com> >>> >>> Suggested by zhilizhao, vlc project has solved the compatibility by >>> the same way, so I borrowed the comments from vlc project. >>> >>> Fix #ticket9449 >>> >>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com> >>> --- >>> libavformat/udp.c | 15 +++++++++++++-- >>> 1 file changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavformat/udp.c b/libavformat/udp.c >>> index 3dc79eb..34488d6 100644 >>> --- a/libavformat/udp.c >>> +++ b/libavformat/udp.c >>> @@ -164,6 +164,10 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, >>> { >>> int protocol, cmd; >>> >>> + /* There is some confusion in the world whether IP_MULTICAST_TTL >>> + * takes a byte or an int as an argument. >>> + * BSD seems to indicate byte so we are going with that and use >>> + * int as a fallback to be safe */ >>> switch (addr->sa_family) { >>> #ifdef IP_MULTICAST_TTL >>> case AF_INET: >>> @@ -183,8 +187,15 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, >>> } >>> >>> if (setsockopt(sockfd, protocol, cmd, &mcastTTL, sizeof(mcastTTL)) < 0) { >>> - ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); >>> - return ff_neterrno(); >>> + /* BSD compatibility */ >>> + unsigned char ttl; >>> + >>> + ff_log_net_error(logctx, AV_LOG_DEBUG, "setsockopt"); >>> + ttl = (unsigned char)(( mcastTTL > 255 ) ? 255 : mcastTTL); >>> + if (setsockopt(sockfd, protocol, cmd, &ttl, sizeof(ttl)) < 0) { >>> + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); >>> + return ff_neterrno(); >>> + } >>> } >>> >>> return 0; >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On Sun, Feb 06, 2022 at 02:27:09PM -0800, Chad Fraleigh wrote: > > On 2/5/2022 6:09 PM, lance.lmwang@gmail.com wrote: > > On Sat, Feb 05, 2022 at 01:26:18PM -0800, Chad Fraleigh wrote: > >> Since any [valid] value over 255 is impossible in the IPv4 protocol (the TTL field is only 8-bits), it should always be capped at 255 (for consistency) or return an invalid value error (the one I would suggest). > >> > > > > zhilizhao have submit a patch to limit the range of ttl from option. Do you want > > to return an invalid error here still? > > If it can never be called with an invalid value, not even programmatically if someone links against the ffmpeg libs, then checking it is unneeded. But also checking it to limit the unsigned char value would be redundant, so only the value cast would be needed, i.e.: > > ttl = (unsigned char) mcastTTL; Yes, checking is unneeded anymore, I'll remove it locally. > > > If however, it could be called without being first limited, then returning an error would be best to avoid silently having unexpected results. Also, checking that it isn't negative should be done in that case. Not counting pending patches, I only see udp_open() calls it, so if it's already bound in there, no extra checks are needed. Sure, the patch will be applied after the pending patches for limit anyway if nobody have other comments. > > Of course, these are only suggestions, since I'm a nobody. =) > > > >> Despite VLC's reversed comment, using an int seems to be the exception to the rule (i.e. only linux and windows seem to use it [as-documented]), perhaps doing the unsigned char first and using the int as the fallback would be better? It's not really just a BSD thing, unless you also count LWIP and Solaris as BSD. Unless VLC's code history shows them doing it this way at one time and swapping it (but forgetting the comment) to fix a known bug? > >> > > > > I have blamed vlc code and sure the code doing it this way at one time(104938796a3). > > For the mismatch of code and comments, I prefer to code always as code were build > > and used by all kinds of system which vlc is supported already. > > > > As for use BSD, I prefer to count LWIP and Solaris into BSD category which using > > rule of byte. If you still prefer to add them into comments, I'm OK also. > > > >> > >> On 2/4/2022 9:28 PM, lance.lmwang@gmail.com wrote: > >>> From: Limin Wang <lance.lmwang@gmail.com> > >>> > >>> Suggested by zhilizhao, vlc project has solved the compatibility by > >>> the same way, so I borrowed the comments from vlc project. > >>> > >>> Fix #ticket9449 > >>> > >>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > >>> --- > >>> libavformat/udp.c | 15 +++++++++++++-- > >>> 1 file changed, 13 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/libavformat/udp.c b/libavformat/udp.c > >>> index 3dc79eb..34488d6 100644 > >>> --- a/libavformat/udp.c > >>> +++ b/libavformat/udp.c > >>> @@ -164,6 +164,10 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > >>> { > >>> int protocol, cmd; > >>> > >>> + /* There is some confusion in the world whether IP_MULTICAST_TTL > >>> + * takes a byte or an int as an argument. > >>> + * BSD seems to indicate byte so we are going with that and use > >>> + * int as a fallback to be safe */ > >>> switch (addr->sa_family) { > >>> #ifdef IP_MULTICAST_TTL > >>> case AF_INET: > >>> @@ -183,8 +187,15 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > >>> } > >>> > >>> if (setsockopt(sockfd, protocol, cmd, &mcastTTL, sizeof(mcastTTL)) < 0) { > >>> - ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > >>> - return ff_neterrno(); > >>> + /* BSD compatibility */ > >>> + unsigned char ttl; > >>> + > >>> + ff_log_net_error(logctx, AV_LOG_DEBUG, "setsockopt"); > >>> + ttl = (unsigned char)(( mcastTTL > 255 ) ? 255 : mcastTTL); > >>> + if (setsockopt(sockfd, protocol, cmd, &ttl, sizeof(ttl)) < 0) { > >>> + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); > >>> + return ff_neterrno(); > >>> + } > >>> } > >>> > >>> return 0; > >> _______________________________________________ > >> ffmpeg-devel mailing list > >> ffmpeg-devel@ffmpeg.org > >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > >> To unsubscribe, visit link above, or email > >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavformat/udp.c b/libavformat/udp.c index 3dc79eb..34488d6 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -164,6 +164,10 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, { int protocol, cmd; + /* There is some confusion in the world whether IP_MULTICAST_TTL + * takes a byte or an int as an argument. + * BSD seems to indicate byte so we are going with that and use + * int as a fallback to be safe */ switch (addr->sa_family) { #ifdef IP_MULTICAST_TTL case AF_INET: @@ -183,8 +187,15 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, } if (setsockopt(sockfd, protocol, cmd, &mcastTTL, sizeof(mcastTTL)) < 0) { - ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); - return ff_neterrno(); + /* BSD compatibility */ + unsigned char ttl; + + ff_log_net_error(logctx, AV_LOG_DEBUG, "setsockopt"); + ttl = (unsigned char)(( mcastTTL > 255 ) ? 255 : mcastTTL); + if (setsockopt(sockfd, protocol, cmd, &ttl, sizeof(ttl)) < 0) { + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt"); + return ff_neterrno(); + } } return 0;