| Message ID | Yd5jaXBCd6GP7kBh@humpty.home.comstyle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] Fix setsockopt IP_MULTICAST_TTL on OpenBSD | expand |
| Context | Check | Description |
|---|---|---|
| andriy/commit_msg_x86 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/commit_msg_ppc | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
| andriy/commit_msg_aarch64_jetson | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/commit_msg_armv7_RPi4 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
ping. On 1/12/2022 12:13 AM, Brad Smith wrote: > Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field > type should be an unsigned char on anything but Linux. > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 180d96a988..29aa865fff 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > { > #ifdef IP_MULTICAST_TTL > if (addr->sa_family == AF_INET) { > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > +#ifdef __linux__ > + int ttl = mcastTTL; > +#else > + unsigned char ttl = mcastTTL; > +#endif > + > + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { > ff_log_net_error(NULL, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > return ff_neterrno(); > }
On Wed, Jan 12, 2022 at 12:13:13AM -0500, Brad Smith wrote: > Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field > type should be an unsigned char on anything but Linux. > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 180d96a988..29aa865fff 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > { > #ifdef IP_MULTICAST_TTL > if (addr->sa_family == AF_INET) { > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > +#ifdef __linux__ > + int ttl = mcastTTL; > +#else > + unsigned char ttl = mcastTTL; > +#endif this "ifdef __linux__" feels like the wrong thing to check, dont you agree ? thx [...]
On Sun, 23 Jan 2022, Michael Niedermayer wrote: > On Wed, Jan 12, 2022 at 12:13:13AM -0500, Brad Smith wrote: >> Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field >> type should be an unsigned char on anything but Linux. >> >> >> diff --git a/libavformat/udp.c b/libavformat/udp.c >> index 180d96a988..29aa865fff 100644 >> --- a/libavformat/udp.c >> +++ b/libavformat/udp.c >> @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, >> { >> #ifdef IP_MULTICAST_TTL >> if (addr->sa_family == AF_INET) { >> - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { >> +#ifdef __linux__ >> + int ttl = mcastTTL; >> +#else >> + unsigned char ttl = mcastTTL; >> +#endif > > this "ifdef __linux__" feels like the wrong thing to check, dont you agree ? As far as I remember linux supports both sizes. So maybe just remove the check entirely? Regards, Marton
On 1/23/2022 6:57 AM, Michael Niedermayer wrote: > On Wed, Jan 12, 2022 at 12:13:13AM -0500, Brad Smith wrote: >> Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field >> type should be an unsigned char on anything but Linux. >> >> >> diff --git a/libavformat/udp.c b/libavformat/udp.c >> index 180d96a988..29aa865fff 100644 >> --- a/libavformat/udp.c >> +++ b/libavformat/udp.c >> @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, >> { >> #ifdef IP_MULTICAST_TTL >> if (addr->sa_family == AF_INET) { >> - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { >> +#ifdef __linux__ >> + int ttl = mcastTTL; >> +#else >> + unsigned char ttl = mcastTTL; >> +#endif > this "ifdef __linux__" feels like the wrong thing to check, dont you agree ? Not sure what you mean. But as I said in one of my other posts.. "FreeBSD, NetBSD, OpenBSD, DragonFlyBSD, macOS, Solaris, AIX, IRIX, HP-UX, QNX, Minix3 and a few others define the ttl parameter to IP_MULTICAST_TTL as an unsigned char. Linux has it as an integer." I looked for various examples of IP_MULTICAST_TTL usage in whatever projects I could find and most of the examples I found used only unsigned char, with BIRD (routing daemon) being one of few that use an int for Linux and unsigned char for *BSD's. It does not have support for any other OS's.
On Sun, Jan 23, 2022 at 02:55:38PM -0500, Brad Smith wrote: > On 1/23/2022 6:57 AM, Michael Niedermayer wrote: > > > On Wed, Jan 12, 2022 at 12:13:13AM -0500, Brad Smith wrote: > > > Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field > > > type should be an unsigned char on anything but Linux. > > > > > > > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > > > index 180d96a988..29aa865fff 100644 > > > --- a/libavformat/udp.c > > > +++ b/libavformat/udp.c > > > @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > > > { > > > #ifdef IP_MULTICAST_TTL > > > if (addr->sa_family == AF_INET) { > > > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > > > +#ifdef __linux__ > > > + int ttl = mcastTTL; > > > +#else > > > + unsigned char ttl = mcastTTL; > > > +#endif > > this "ifdef __linux__" feels like the wrong thing to check, dont you agree ? > > Not sure what you mean. "__linux__" seems the wrong property to check for this is not #ifdef __linux__ osname = "linux" #else i would have expected more something like #if HAVE_INT_TTL int ttl = mcastTTL; #else or maybe even something along the lines of WHATEVER_TTL_TYPE ttl = mcastTTL; thx [...]
On Mon, Jan 24, 2022 at 01:40:47PM +0100, Michael Niedermayer wrote: > On Sun, Jan 23, 2022 at 02:55:38PM -0500, Brad Smith wrote: > > On 1/23/2022 6:57 AM, Michael Niedermayer wrote: > > > > > On Wed, Jan 12, 2022 at 12:13:13AM -0500, Brad Smith wrote: > > > > Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field > > > > type should be an unsigned char on anything but Linux. > > > > > > > > > > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > > > > index 180d96a988..29aa865fff 100644 > > > > --- a/libavformat/udp.c > > > > +++ b/libavformat/udp.c > > > > @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > > > > { > > > > #ifdef IP_MULTICAST_TTL > > > > if (addr->sa_family == AF_INET) { > > > > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > > > > +#ifdef __linux__ > > > > + int ttl = mcastTTL; > > > > +#else > > > > + unsigned char ttl = mcastTTL; > > > > +#endif > > > this "ifdef __linux__" feels like the wrong thing to check, dont you agree ? > > > > Not sure what you mean. > > "__linux__" seems the wrong property to check for > this is not > #ifdef __linux__ > osname = "linux" > #else > > i would have expected more something like > #if HAVE_INT_TTL > int ttl = mcastTTL; > #else > > or maybe even something along the lines of > > WHATEVER_TTL_TYPE ttl = mcastTTL; > > thx Ok, how about something like this? diff --git a/configure b/configure index 493493b4c5..055ff3c206 100755 --- a/configure +++ b/configure @@ -3838,6 +3838,8 @@ doc_deps_any="manpages htmlpages podpages txtpages" logfile="ffbuild/config.log" +multicast_ttl_type='unsigned char' + # installation paths prefix_default="/usr/local" bindir_default='${prefix}/bin' @@ -5653,6 +5655,7 @@ case $target_os in linux) enable section_data_rel_ro enabled_any arm aarch64 && enable_weak linux_perf + multicast_ttl_type='int' ;; irix*) target_os=irix @@ -7784,6 +7787,7 @@ cat > $TMPH <<EOF #define SLIBSUF "$SLIBSUF" #define HAVE_MMX2 HAVE_MMXEXT #define SWS_MAX_FILTER_SIZE $sws_max_filter_size +#define MULTICAST_TTL_TYPE $multicast_ttl_type EOF test -n "$assert_level" && diff --git a/libavformat/udp.c b/libavformat/udp.c index 83c042d079..179e719286 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -27,6 +27,8 @@ #define _DEFAULT_SOURCE #define _BSD_SOURCE /* Needed for using struct ip_mreq with recent glibc */ +#include "config.h" + #include "avformat.h" #include "avio_internal.h" #include "libavutil/avassert.h" @@ -164,7 +166,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, { #ifdef IP_MULTICAST_TTL if (addr->sa_family == AF_INET) { - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { + MULTICAST_TTL_TYPE ttl = mcastTTL; + + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); return ff_neterrno(); }
On Wed, Jan 12, 2022 at 12:13:13AM -0500, Brad Smith wrote: > Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field > type should be an unsigned char on anything but Linux. > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 180d96a988..29aa865fff 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > { > #ifdef IP_MULTICAST_TTL > if (addr->sa_family == AF_INET) { > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > +#ifdef __linux__ > + int ttl = mcastTTL; > +#else > + unsigned char ttl = mcastTTL; > +#endif I don't have BSD system for test, but I prefer to use socklen_t, please try with my proposal patch: --- libavformat/udp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libavformat/udp.c b/libavformat/udp.c index 83c042d079..b9baa0a803 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, { #ifdef IP_MULTICAST_TTL if (addr->sa_family == AF_INET) { - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { + socklen_t ttl = mcastTTL; + + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); return ff_neterrno(); > + > + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { > ff_log_net_error(NULL, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > return ff_neterrno(); > } > _______________________________________________ > 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 apparently linux will auto-detect (as mentioned by Marton Balint), based on the optlen parameter, just using unsigned char in all cases seems to be the cleanest. However, I would advise including a comment in the code to that effect which says to ignore the [outdated] linux documentation (so someone doesn't needlessly "correct" it in the future).
I looked at the kernel source and it does work both ways:
static int do_ip_setsockopt(struct sock *sk, int level, int optname,
sockptr_t optval, unsigned int optlen)
{
...
switch (optname) {
...
case IP_MULTICAST_TTL:
...
if (optlen >= sizeof(int)) {
if (copy_from_sockptr(&val, optval, sizeof(val)))
return -EFAULT;
} else if (optlen >= sizeof(char)) {
unsigned char ucval;
if (copy_from_sockptr(&ucval, optval, sizeof(ucval)))
return -EFAULT;
val = (int) ucval;
}
}
...
}
This check has been in the kernel since at least 2.6.12-rc2 (from Apr 2005). It should work fine, unless newer ffmpeg builds support is needed on older systems. So the only question is how old are the kernels in IoT and android devices which might use the current ffmpeg?
On 1/24/2022 11:25 PM, lance.lmwang@gmail.com wrote:
> On Wed, Jan 12, 2022 at 12:13:13AM -0500, Brad Smith wrote:
>> Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field
>> type should be an unsigned char on anything but Linux.
>>
>>
>> diff --git a/libavformat/udp.c b/libavformat/udp.c
>> index 180d96a988..29aa865fff 100644
>> --- a/libavformat/udp.c
>> +++ b/libavformat/udp.c
>> @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL,
>> {
>> #ifdef IP_MULTICAST_TTL
>> if (addr->sa_family == AF_INET) {
>> - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) {
>> +#ifdef __linux__
>> + int ttl = mcastTTL;
>> +#else
>> + unsigned char ttl = mcastTTL;
>> +#endif
>
>
> I don't have BSD system for test, but I prefer to use socklen_t, please try with my proposal patch:
>
> ---
> libavformat/udp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/udp.c b/libavformat/udp.c
> index 83c042d079..b9baa0a803 100644
> --- a/libavformat/udp.c
> +++ b/libavformat/udp.c
> @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL,
> {
> #ifdef IP_MULTICAST_TTL
> if (addr->sa_family == AF_INET) {
> - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) {
> + socklen_t ttl = mcastTTL;
> +
> + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) {
> ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)");
> return ff_neterrno();
>
>
>> +
>> + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) {
>> ff_log_net_error(NULL, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)");
>> return ff_neterrno();
>> }
>> _______________________________________________
>> 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 1/25/2022 7:28 PM, Chad Fraleigh wrote: > Since apparently linux will auto-detect (as mentioned by Marton Balint), based on the optlen parameter, just using unsigned char in all cases seems to be the cleanest. However, I would advise including a comment in the code to that effect which says to ignore the [outdated] linux documentation (so someone doesn't needlessly "correct" it in the future). > > I looked at the kernel source and it does work both ways: > > static int do_ip_setsockopt(struct sock *sk, int level, int optname, > sockptr_t optval, unsigned int optlen) > { > ... > switch (optname) { > ... > case IP_MULTICAST_TTL: > ... > if (optlen >= sizeof(int)) { > if (copy_from_sockptr(&val, optval, sizeof(val))) > return -EFAULT; > } else if (optlen >= sizeof(char)) { > unsigned char ucval; > > if (copy_from_sockptr(&ucval, optval, sizeof(ucval))) > return -EFAULT; > val = (int) ucval; > } > } > ... > } > > This check has been in the kernel since at least 2.6.12-rc2 (from Apr 2005). It should work fine, unless newer ffmpeg builds support is needed on older systems. So the only question is how old are the kernels in IoT and android devices which might use the current ffmpeg? Thanks. This would he much simpler. The oldest Android, 1.5, uses the 2.6.27 kernel. The oldest still supported Android, 4.4, uses the 3.10 kernel. I cannot imagine anyone shipping anything older than a 3.x (something like 3.18) kernel on anything IoT that is still supported and wanting to ship up to date software.
On Tue, Jan 25, 2022 at 04:28:33PM -0800, Chad Fraleigh wrote: > Since apparently linux will auto-detect (as mentioned by Marton Balint), based on the optlen parameter, just using unsigned char in all cases seems to be the cleanest. However, I would advise including a comment in the code to that effect which says to ignore the [outdated] linux documentation (so someone doesn't needlessly "correct" it in the future). > I agree with, use unsigned char is preferable for all system I think. > I looked at the kernel source and it does work both ways: > > static int do_ip_setsockopt(struct sock *sk, int level, int optname, > sockptr_t optval, unsigned int optlen) > { > ... > switch (optname) { > ... > case IP_MULTICAST_TTL: > ... > if (optlen >= sizeof(int)) { > if (copy_from_sockptr(&val, optval, sizeof(val))) > return -EFAULT; > } else if (optlen >= sizeof(char)) { > unsigned char ucval; > > if (copy_from_sockptr(&ucval, optval, sizeof(ucval))) > return -EFAULT; > val = (int) ucval; > } > } > ... > } > > This check has been in the kernel since at least 2.6.12-rc2 (from Apr 2005). It should work fine, unless newer ffmpeg builds support is needed on older systems. So the only question is how old are the kernels in IoT and android devices which might use the current ffmpeg? > > > On 1/24/2022 11:25 PM, lance.lmwang@gmail.com wrote: > > On Wed, Jan 12, 2022 at 12:13:13AM -0500, Brad Smith wrote: > >> Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field > >> type should be an unsigned char on anything but Linux. > >> > >> > >> diff --git a/libavformat/udp.c b/libavformat/udp.c > >> index 180d96a988..29aa865fff 100644 > >> --- a/libavformat/udp.c > >> +++ b/libavformat/udp.c > >> @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > >> { > >> #ifdef IP_MULTICAST_TTL > >> if (addr->sa_family == AF_INET) { > >> - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > >> +#ifdef __linux__ > >> + int ttl = mcastTTL; > >> +#else > >> + unsigned char ttl = mcastTTL; > >> +#endif > > > > > > I don't have BSD system for test, but I prefer to use socklen_t, please try with my proposal patch: > > > > --- > > libavformat/udp.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > > index 83c042d079..b9baa0a803 100644 > > --- a/libavformat/udp.c > > +++ b/libavformat/udp.c > > @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > > { > > #ifdef IP_MULTICAST_TTL > > if (addr->sa_family == AF_INET) { > > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > > + socklen_t ttl = mcastTTL; > > + > > + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { > > ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > > return ff_neterrno(); > > > > > >> + > >> + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { > >> ff_log_net_error(NULL, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > >> return ff_neterrno(); > >> } > >> _______________________________________________ > >> 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".
On Wed, Jan 12, 2022 at 12:13:14AM -0500, Brad Smith wrote: > Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field > type should be an unsigned char on anything but Linux. Based on feedback so far. Here is a much simpler approach to this issue.. diff --git a/libavformat/udp.c b/libavformat/udp.c index 83c042d079..da1b98890b 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, { #ifdef IP_MULTICAST_TTL if (addr->sa_family == AF_INET) { - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { + unsigned char ttl = mcastTTL; /* ignore the outdated Linux documentation */ + + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); return ff_neterrno(); }
On Wed, 26 Jan 2022, Brad Smith wrote: > On Wed, Jan 12, 2022 at 12:13:14AM -0500, Brad Smith wrote: >> Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field >> type should be an unsigned char on anything but Linux. > > Based on feedback so far. Here is a much simpler approach to this issue.. Win32 needs DWORD unfortunately. I missed it earlier, sorry. https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options Regards, Marton > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index 83c042d079..da1b98890b 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > { > #ifdef IP_MULTICAST_TTL > if (addr->sa_family == AF_INET) { > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > + unsigned char ttl = mcastTTL; /* ignore the outdated Linux documentation */ > + > + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { > ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > return ff_neterrno(); > } > _______________________________________________ > 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 1/26/2022 12:50 PM, Marton Balint wrote: > > > On Wed, 26 Jan 2022, Brad Smith wrote: > >> On Wed, Jan 12, 2022 at 12:13:14AM -0500, Brad Smith wrote: >>> Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field >>> type should be an unsigned char on anything but Linux. >> >> Based on feedback so far. Here is a much simpler approach to this issue.. > > Win32 needs DWORD unfortunately. I missed it earlier, sorry. > > https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options It gets worse from there. Since I can't look at the real Win32 source to see if it handles it both way, I had to go with Wine and ReactOS.. In Wine, the tests seem to use just an 'int' (which is probably always the same as 'DWORD'). But then, it proceeds to check it using a 1, 2, 3, and 4 byte optlen. In the implementation, it appears to just pass the values as-is to the underlying OS, rather than explicitly translate it to the appropriate type. For ReactOS, it blindly treats it as an unsigned char (regardless of optlen, as long as it at least 1) when setting/getting the value. But then in a debug line just below that, it treats optval as an 'int *'. Maybe some tests need to be done on Win32 to see if it works with either size (i.e. set as uchar, get as DWORD, then in reverse and make sure the input/output match in all cases). I didn't check to see what msys/mingw* does, if anything. > > Regards, > Marton > >> >> >> diff --git a/libavformat/udp.c b/libavformat/udp.c >> index 83c042d079..da1b98890b 100644 >> --- a/libavformat/udp.c >> +++ b/libavformat/udp.c >> @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, >> { >> #ifdef IP_MULTICAST_TTL >> if (addr->sa_family == AF_INET) { >> - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { >> + unsigned char ttl = mcastTTL; /* ignore the outdated Linux documentation */ >> + >> + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { >> ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); >> return ff_neterrno(); >> } >> _______________________________________________ >> 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".
On Wed, Jan 26, 2022 at 09:50:47PM +0100, Marton Balint wrote: > > > On Wed, 26 Jan 2022, Brad Smith wrote: > > > On Wed, Jan 12, 2022 at 12:13:14AM -0500, Brad Smith wrote: > > > Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field > > > type should be an unsigned char on anything but Linux. > > > > Based on feedback so far. Here is a much simpler approach to this issue.. > > Win32 needs DWORD unfortunately. I missed it earlier, sorry. > > https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options From my testing on Mac system, it support one byte only, but at least, it'll report "Invalid argument" only if the ttl > 255. Maybe we can try with one byte again if the errno is invalid like below. I tested it with ttl > 255 on Mac system and without "Invalid argument" anymore. #ifdef IP_MULTICAST_TTL + int ret = 0; if (addr->sa_family == AF_INET) { - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { + ret = setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)); + /* try with byte also for IP_MULTICAST_TTL for some system like OpenBSD */ + if (ret < 0 && errno == EINVAL) { + unsigned char ttl = mcastTTL; + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL): "); + ret = setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)); + } + if (ret < 0) { ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); return ff_neterrno(); > > Regards, > Marton > > > > > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > > index 83c042d079..da1b98890b 100644 > > --- a/libavformat/udp.c > > +++ b/libavformat/udp.c > > @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > > { > > #ifdef IP_MULTICAST_TTL > > if (addr->sa_family == AF_INET) { > > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > > + unsigned char ttl = mcastTTL; /* ignore the outdated Linux documentation */ > > + > > + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { > > ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > > return ff_neterrno(); > > } > > _______________________________________________ > > 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".
> On Jan 27, 2022, at 9:59 AM, lance.lmwang@gmail.com wrote: > > On Wed, Jan 26, 2022 at 09:50:47PM +0100, Marton Balint wrote: >> >> >> On Wed, 26 Jan 2022, Brad Smith wrote: >> >>> On Wed, Jan 12, 2022 at 12:13:14AM -0500, Brad Smith wrote: >>>> Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field >>>> type should be an unsigned char on anything but Linux. >>> >>> Based on feedback so far. Here is a much simpler approach to this issue.. >> >> Win32 needs DWORD unfortunately. I missed it earlier, sorry. >> >> https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options > > From my testing on Mac system, it support one byte only, but at least, it'll > report "Invalid argument" only if the ttl > 255. Maybe we can try with one byte > again if the errno is invalid like below. I tested it with ttl > 255 on Mac system > and without "Invalid argument" anymore. > > #ifdef IP_MULTICAST_TTL > + int ret = 0; > if (addr->sa_family == AF_INET) { > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > + ret = setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)); > + /* try with byte also for IP_MULTICAST_TTL for some system like OpenBSD */ > + if (ret < 0 && errno == EINVAL) { > + unsigned char ttl = mcastTTL; > + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL): "); > + ret = setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)); > + } > + if (ret < 0) { > ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > return ff_neterrno(); > > >> >> Regards, >> Marton >> >>> >>> >>> diff --git a/libavformat/udp.c b/libavformat/udp.c >>> index 83c042d079..da1b98890b 100644 >>> --- a/libavformat/udp.c >>> +++ b/libavformat/udp.c >>> @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, >>> { >>> #ifdef IP_MULTICAST_TTL >>> if (addr->sa_family == AF_INET) { >>> - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { >>> + unsigned char ttl = mcastTTL; /* ignore the outdated Linux documentation */ >>> + >>> + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { >>> ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); >>> return ff_neterrno(); >>> } >>> _______________________________________________ >>> 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". > > -- > Thanks, > Limin Wang > _______________________________________________ > 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 Jan 27, 2022, at 9:59 AM, lance.lmwang@gmail.com wrote: > > On Wed, Jan 26, 2022 at 09:50:47PM +0100, Marton Balint wrote: >> >> >> On Wed, 26 Jan 2022, Brad Smith wrote: >> >>> On Wed, Jan 12, 2022 at 12:13:14AM -0500, Brad Smith wrote: >>>> Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field >>>> type should be an unsigned char on anything but Linux. >>> >>> Based on feedback so far. Here is a much simpler approach to this issue.. >> >> Win32 needs DWORD unfortunately. I missed it earlier, sorry. >> >> https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options > > From my testing on Mac system, it support one byte only, but at least, it'll > report "Invalid argument" only if the ttl > 255. Maybe we can try with one byte > again if the errno is invalid like below. I tested it with ttl > 255 on Mac system > and without "Invalid argument" anymore. MacOS support int and unsigned char from my test. The upper bound of TTL is limited to 255 (limited by protocol design), I guess it’s unrelated to int or unsigned char. > > #ifdef IP_MULTICAST_TTL > + int ret = 0; > if (addr->sa_family == AF_INET) { > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > + ret = setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)); > + /* try with byte also for IP_MULTICAST_TTL for some system like OpenBSD */ > + if (ret < 0 && errno == EINVAL) { > + unsigned char ttl = mcastTTL; > + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL): "); > + ret = setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)); > + } > + if (ret < 0) { > ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > return ff_neterrno(); > > >> >> Regards, >> Marton >> >>> >>> >>> diff --git a/libavformat/udp.c b/libavformat/udp.c >>> index 83c042d079..da1b98890b 100644 >>> --- a/libavformat/udp.c >>> +++ b/libavformat/udp.c >>> @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, >>> { >>> #ifdef IP_MULTICAST_TTL >>> if (addr->sa_family == AF_INET) { >>> - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { >>> + unsigned char ttl = mcastTTL; /* ignore the outdated Linux documentation */ >>> + >>> + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { >>> ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); >>> return ff_neterrno(); >>> } >>> _______________________________________________ >>> 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". > > -- > Thanks, > Limin Wang > _______________________________________________ > 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 Thu, Jan 27, 2022 at 10:30:10AM +0800, "zhilizhao(赵志立)" wrote: > > > > On Jan 27, 2022, at 9:59 AM, lance.lmwang@gmail.com wrote: > > > > On Wed, Jan 26, 2022 at 09:50:47PM +0100, Marton Balint wrote: > >> > >> > >> On Wed, 26 Jan 2022, Brad Smith wrote: > >> > >>> On Wed, Jan 12, 2022 at 12:13:14AM -0500, Brad Smith wrote: > >>>> Fix setsockopt() usage on OpenBSD with IP_MULTICAST_TTL. The field > >>>> type should be an unsigned char on anything but Linux. > >>> > >>> Based on feedback so far. Here is a much simpler approach to this issue.. > >> > >> Win32 needs DWORD unfortunately. I missed it earlier, sorry. > >> > >> https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options > > > > From my testing on Mac system, it support one byte only, but at least, it'll > > report "Invalid argument" only if the ttl > 255. Maybe we can try with one byte > > again if the errno is invalid like below. I tested it with ttl > 255 on Mac system > > and without "Invalid argument" anymore. > > > MacOS support int and unsigned char from my test. The upper bound of TTL is limited > to 255 (limited by protocol design), I guess it’s unrelated to int or unsigned char. Yes, MacOS isn't caused by one byte or int. By #Ticket9449, it'll failed for ttl=10, so I guess my proposal which try with one byte again should be work for Openbsd system. But you can't limit to 255 only as some system support for int like Win32. > > > > > > #ifdef IP_MULTICAST_TTL > > + int ret = 0; > > if (addr->sa_family == AF_INET) { > > - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > > + ret = setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)); > > + /* try with byte also for IP_MULTICAST_TTL for some system like OpenBSD */ > > + if (ret < 0 && errno == EINVAL) { > > + unsigned char ttl = mcastTTL; > > + ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL): "); > > + ret = setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)); > > + } > > + if (ret < 0) { > > ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > > return ff_neterrno(); > > > > > >> > >> Regards, > >> Marton > >> > >>> > >>> > >>> diff --git a/libavformat/udp.c b/libavformat/udp.c > >>> index 83c042d079..da1b98890b 100644 > >>> --- a/libavformat/udp.c > >>> +++ b/libavformat/udp.c > >>> @@ -164,7 +164,9 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, > >>> { > >>> #ifdef IP_MULTICAST_TTL > >>> if (addr->sa_family == AF_INET) { > >>> - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { > >>> + unsigned char ttl = mcastTTL; /* ignore the outdated Linux documentation */ > >>> + > >>> + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { > >>> ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); > >>> return ff_neterrno(); > >>> } > >>> _______________________________________________ > >>> 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". > > > > -- > > Thanks, > > Limin Wang > > _______________________________________________ > > 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 180d96a988..29aa865fff 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -163,7 +163,13 @@ static int udp_set_multicast_ttl(int sockfd, int mcastTTL, { #ifdef IP_MULTICAST_TTL if (addr->sa_family == AF_INET) { - if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &mcastTTL, sizeof(mcastTTL)) < 0) { +#ifdef __linux__ + int ttl = mcastTTL; +#else + unsigned char ttl = mcastTTL; +#endif + + if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl)) < 0) { ff_log_net_error(NULL, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_TTL)"); return ff_neterrno(); }