GNOME Bugzilla – Bug 740791
gio: cannot specify the source when joining a multicast group (IGMPv3 SSM Source Specific Multicast RFC 4604)
Last modified: 2018-04-24 19:01:48 UTC
Even if "g_socket_join_multicast_group" has a "gboolean source-specific" parameter, there is no way to setup the source address. It should use the structure "ip_mreq_source" defined here: http://lxr.free-electrons.com/source/include/uapi/linux/in.h#L168 Instead of "ip_mreq" when IP_ADD_SOURCE_MEMBERSHIP is passed to "setsockopt", see http://man7.org/linux/man-pages/man7/ip.7.html Indeed "ip_mreq_source" has a field "imr_sourceaddr" which is the "source-specific" (see RFC 4604)
Created attachment 291705 [details] [review] gio: really add support for source-specific multicast IGMPv3 SSM It breaks current api as I changed a parameter from gboolean to GInetAddress* . Should we use a new function or wait for glib 3.0 ?
(In reply to comment #1) > It breaks current api as I changed a parameter from gboolean to GInetAddress* . > Should we use a new function or wait for glib 3.0 ? There are no plans for glib 3.0, so you'd be waiting a long time. Make a new function.
Created attachment 291966 [details] [review] gio: add g_socket_join_multicast_group_ssm (IGMPv3 SSM)
ping ?
Review of attachment 291966 [details] [review]: ::: gio/gsocket.c @@ +2140,3 @@ GError **error) { + return g_socket_multicast_group_operation (socket, group, NULL, iface, TRUE, error); Here and in the leave function you drop the source_specific boolean now, which would change behaviour. Or did the old code not work at all anyway?
(In reply to Sebastian Dröge (slomo) from comment #5) > Review of attachment 291966 [details] [review] [review]: > > ::: gio/gsocket.c > @@ +2140,3 @@ > GError **error) > { > + return g_socket_multicast_group_operation (socket, group, NULL, iface, > TRUE, error); > > Here and in the leave function you drop the source_specific boolean now, > which would change behaviour. Or did the old code not work at all anyway? Because "source_specific" is a boolean in this function, see https://github.com/GNOME/glib/blob/master/gio/gsocket.c#L2319 (It was the point of going from #1 to #3, i.e. adding a new function instead of breaking API). "source_specific" has a different type in the new "g_socket_join_multicast_group_ssm" function.
Yes I saw that, but now if source_specific==TRUE we would do the same as with source_specific==FALSE. That is, use IP_ADD_MEMBERSHIP instead ofIP_ADD_SOURCE_MEMBERSHIP
(In reply to Sebastian Dröge (slomo) from comment #7) > Yes I saw that, but now if source_specific==TRUE we would do the same as > with source_specific==FALSE. That is, use IP_ADD_MEMBERSHIP instead > ofIP_ADD_SOURCE_MEMBERSHIP Yup, if someone call it with source_specific is true then maybe I should add a g_warning with a message saying "source_specific is ineffective, please use g_socket_join_multicast_group_ssm instead", pointing to the other function. Other solution would be to set a default "source_specific" if source_specific is true. The same default as before this patch, I do not know what it was since "ip_mreq_source mreq4source;" was not used/set. (maybe equal as if the struct was memset(0)) (same remarks for the "leave" functions) > Or did the old code not work at all anyway? Yes it was not working because of missing code I added around "ip_mreq_source mreq4source;"
Makes sense then
GLib people, what should happen here? :)
Gentle ping ?
(In reply to Julien Isorce from comment #11) > Gentle ping ? Please don’t assign bugs to people unannounced. A comment would suffice (the GLib maintainers subscribe to all GLib bugs automatically). (In reply to Sebastian Dröge (slomo) from comment #10) > GLib people, what should happen here? :) Unless the old source_specific==TRUE behaviour for g_socket_{join,leave}_multicast_group() was totally broken in every situation, please leave the behaviour of those functions unchanged for when they’re called with source_specific==TRUE. Don’t add a warning pointing to the new API, but do amend the documentation for them to point to it. I’m happy with adding new API for this, but I am not a multicast expert and haven’t read up on this. Ultimately, it’s Dan’s call about what to push here.
Created attachment 360768 [details] [review] gio: add g_socket_join_multicast_group_ssm (IGMPv3 SSM) >> please leave the behaviour of those functions unchanged Done
Review of attachment 360768 [details] [review]: The new APIs need to be added to docs/reference/gio/gio-sections.txt. It would also be nice to have some unit tests for them. The new APIs also need to be mentioned in the documentation for the old APIs. ::: gio/gsocket.c @@ +2385,3 @@ + g_return_val_if_fail (G_IS_SOCKET (socket), FALSE); + g_return_val_if_fail (socket->priv->type == G_SOCKET_TYPE_DATAGRAM, FALSE); + g_return_val_if_fail (G_IS_INET_ADDRESS (group), FALSE); Missing some other preconditions: g_return_val_if_fail (source_specific == NULL || G_IS_INET_ADDRESS (source_specific), FALSE); g_return_val_if_fail (iface == NULL || *iface != '\0', FALSE); g_return_val_if_fail (error == NULL || *error == NULL, FALSE); @@ +2387,3 @@ + g_return_val_if_fail (G_IS_INET_ADDRESS (group), FALSE); + + if (!source_specific) { Braces should be on their own lines. For example: if (!source_specific) { /* blah */ } @@ +2395,3 @@ + return FALSE; + + if (g_inet_address_get_family (group) == G_SOCKET_FAMILY_IPV4) Instead of an `if` statement, please use a `switch` on the return value from g_inet_address_get_family(), so that -Wswitch-enum can warn us in future if more socket families are added to the enum but not to here. @@ +2408,3 @@ + _("Error joining multicast group: %s") : + _("Error leaving multicast group: %s"), + _("ip_mreq_source expects an ipv4 source specific ip")); s/ipv4 source specific ip/IPv4 source-specific IP address/ @@ +2416,3 @@ + + memset (&mc_req_src, 0, sizeof (mc_req_src)); + memcpy (&mc_req_src.imr_multiaddr, native_addr, sizeof (struct in_addr)); Should you use g_inet_address_get_native_size() here rather than sizeof(struct in_addr)? @@ +2420,3 @@ +#if defined(G_OS_WIN32) + if (iface) + mc_req_src.imr_interface.s_addr = g_htonl (if_nametoindex (iface)); You need to check for an error return from if_nametoindex(). @@ +2428,3 @@ + + memcpy (&mc_req_src.imr_sourceaddr, native_addr_src, + sizeof (struct in_addr)); Possibly should use g_inet_address_get_native_size() here too. @@ +2439,3 @@ + _("Error joining multicast group: %s") : + _("Error leaving multicast group: %s"), + _("No support for ipv4 source-specific multicast")); s/ipv4/IPv4/ (similarly for IPv6 below). @@ +2449,3 @@ + _("Error joining multicast group: %s") : + _("Error leaving multicast group: %s"), + _("No support for ipv6 source-specific multicast")); What would it take for IPv6 to be supported? @@ +2474,3 @@ + * @socket: a #GSocket. + * @group: a #GInetAddress specifying the group address to join. + * @iface: (allow-none): Name of the interface to use, or %NULL (nullable) and (optional) are the replacements for (allow-none), which is deprecated. (nullable) should be used here (and elsewhere in the new documentation comments). See https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations. @@ +2476,3 @@ + * @iface: (allow-none): Name of the interface to use, or %NULL + * @source_specific: (allow-none): a #GInetAddress specifying the + * source-specific multicast address or %NULL to ignore. @source_specific and @iface are listed in the wrong order. @@ +2487,3 @@ + * to bind to based on @group. + * + * If @source_specific is not NULL, use source-specific multicast as s/NULL/%NULL/ s/use source-specific/source-specific/ (since there’s a ‘used’ later in the sentence). @@ +2511,3 @@ + * @group: a #GInetAddress specifying the group address to leave. + * @iface: (allow-none): Interface used + * @source_specific: (allow-none): a #GInetAddress specifying the (nullable), as above. @@ +2512,3 @@ + * @iface: (allow-none): Interface used + * @source_specific: (allow-none): a #GInetAddress specifying the + * source-specific multicast address or %NULL to ignore. @iface and @source_specific are listed in the wrong order.
Created attachment 360777 [details] [review] gio: add g_socket_join_multicast_group_ssm (IGMPv3 SSM) Thx for the detailed review. Addressed all remarks. For IPv6 I tried using group_source_req but I got errors about the protocol not being supported. So I added an explicit "not implemented" instead.
Created attachment 360778 [details] [review] gio: add g_socket_join_multicast_group_ssm (IGMPv3 SSM) Upload the right file.
Created attachment 360779 [details] [review] gio: add g_socket_join_multicast_group_ssm (IGMPv3 SSM) Upload the right file.
Review of attachment 360779 [details] [review]: The new APIs *still* need to be mentioned in the documentation for the old APIs. ::: gio/gsocket.c @@ +2469,3 @@ + _("Error joining multicast group: %s") : + _("Error leaving multicast group: %s"), + _("Support for IPv6 source-specific multicast not implemented")); What would it take for IPv6 support to be implemented? @@ +2482,3 @@ + break; + + default: Please also add cases for the other existing members of the GSocketType enum. They should return runtime errors (maybe G_IO_ERROR_NOT_SUPPORTED?) rather than assertion failures. ::: gio/gsocket.h @@ +169,3 @@ + GInetAddress *source_specific, + const gchar *iface, + GError **error); Nitpick: The argument blocks for the two functions aren’t aligned wrt each other.
Created attachment 361329 [details] [review] gio: add g_socket_join_multicast_group_ssm (IGMPv3 SSM) Back from holiday. I addressed all remarks.
(In reply to Philip Withnall from comment #18) > What would it take for IPv6 support to be implemented? I added a remark in #15 and a comment in previous patch (just below case G_SOCKET_FAMILY_IPV6).
Review of attachment 361329 [details] [review]: Thanks for the update. A few more comments. ::: gio/gsocket.c @@ +2273,3 @@ * with a %G_IO_ERROR_NOT_SUPPORTED error. + * If @source_specific is a #GInetAddress use + * g_socket_join_multicast_group_ssm() instead. That doesn’t really make sense, since in this context, @source_specific is a gboolean and can never be a #GInetAddress. How about: > To bind to a given source-specific multicast address, use g_socket_join_multicast_group_ssm() instead. @@ +2345,3 @@ + switch (g_inet_address_get_family (group)) + { + case G_SOCKET_FAMILY_UNIX: Please also list G_SOCKET_FAMILY_INVALID explicitly above here so we don’t get -Wswitch-enum warnings, and so that an error is correctly returned for it: case G_SOCKET_FAMILY_INVALID: case G_SOCKET_FAMILY_UNIX: @@ +2351,3 @@ + _("Error joining multicast group: %s") : + _("Error leaving multicast group: %s"), + _("No support for unix socket")); Change this message to “Unsupported socket family”. @@ +2392,3 @@ + mc_req_src.imr_interface.s_addr = g_htonl (INADDR_ANY); +#else + mc_req_src.imr_interface.s_addr = g_htonl (INADDR_ANY); Why not use if_nametoindex() on non-Windows platforms? This deserves a comment. @@ +2420,3 @@ + { +#ifdef MCAST_JOIN_SOURCE_GROUP + /* TODO: use struct group_source_req. */ From this code I found, it looks like this should be possible for IPv6: http://courses.cs.vt.edu/cs4254/spring06/unpv13e/lib/mcast_join.c Windows certainly claims to support it: https://msdn.microsoft.com/en-us/library/windows/desktop/bb427441(v=vs.85).aspx
Created attachment 361538 [details] [review] gio: add g_socket_join_multicast_group_ssm (IGMPv3 SSM) Addressed all remarks. Works for IPv6 too, in fact my mistake in my previous attempt was to use filter "igmp" in wireshark whereas filter has to be icmpv6 for ipv6. Regarding if_nametoindex win32/linux, see the new version. (0.0.0.index) only works on win32 so on Linux I had to use an ioctl(SIOCGIFADDR) to retrieve the IPv4 address of the network iface name. Missing meson build case but I want to know first if you are ok to use that ioctl or not. See the patch. Thx!
Review of attachment 361538 [details] [review]: ::: configure.ac @@ +966,3 @@ + ], [ + AC_MSG_RESULT(no) +]) Yup, this needs the equivalent changes in meson.build. ::: gio/gsocket.c @@ +2276,3 @@ * + * To bind to a given source-specific multicast address, use + * g_socket_join_multicast_group_ssm() instead. The documentation for g_socket_leave_multicast_group() needs a similar addition. @@ +2411,3 @@ + } + + memcpy(ifr.ifr_name, iface, if_name_len); Nitpick: Coding style requires a space before the `(` in a function call. @@ +2464,3 @@ + if (iface) + { + iface_index = if_nametoindex (iface); Indentation problem: This whole block should be indented by a further 2 spaces. @@ +2471,3 @@ + g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errsv), + _("Interface not found: %s"), g_strerror (errsv)); + return FALSE; saddr_group and saddr_source_specific are leaked on this error path (and on others, below). @@ +2474,3 @@ + } + } +#endif Since there are nested #ifdef/#endif statements here, it would be useful to add comments which make it easier to work out which one relates to which. i.e.: #ifdef SOMETHING … #ifdef OTHER_THING … #endif /* OTHER_THING */ … #endif /* SOMETHING */ @@ +2478,3 @@ + + result = g_socket_address_to_native (saddr_group, &mc_req_src.gsr_group, + sizeof mc_req_src.gsr_group, error); sizeof (mc_req_src.gsr_group) to conform with the coding style. Similarly elsewhere in the patch. @@ +2492,3 @@ + optname = + join_group ? MCAST_JOIN_SOURCE_GROUP : MCAST_LEAVE_SOURCE_GROUP; + result &= setsockopt (socket->priv->fd, IPPROTO_IPV6, optname, Don’t reuse `result` for gboolean and int return value handling; it makes the type handling confusing. Have two separate variables. @@ +2493,3 @@ + join_group ? MCAST_JOIN_SOURCE_GROUP : MCAST_LEAVE_SOURCE_GROUP; + result &= setsockopt (socket->priv->fd, IPPROTO_IPV6, optname, + &mc_req_src, sizeof (mc_req_src)); Indentation problem: When wrapping function parameters, they should be indented up to the level of the function parameters on the previous line. For example: some_function (arg1, arg2, arg3, arg4); (Applies throughout the patch.)
Created attachment 361655 [details] [review] gio: add g_socket_join_multicast_group_ssm (IGMPv3 SSM) Addressed all remarks (meson build, doc, indent).
Review of attachment 361655 [details] [review]: OK, let’s go with this. Thanks for the patches.
Attachment 361655 [details] pushed as ea725a6 - gio: add g_socket_join_multicast_group_ssm (IGMPv3 SSM)
Hi Philip, in the patch it is GLIB_AVAILABLE_IN_2_56, I wonder if it should have been 2_55 instead ?
(In reply to Julien Isorce from comment #27) > Hi Philip, in the patch it is GLIB_AVAILABLE_IN_2_56, I wonder if it should > have been 2_55 instead ? No, 2_56 is correct. Those macros operate on a per-stable-release basis. 2.55 is an unstable release leading to the 2.56 stable release series.
I think this patch breaks cross build for Android: The other place where s_addr is used it goes in the #ifdef HAVE_IP_MREQN case instead (I have no idea what it is), that's why it wasn't failing in glib 2.54. Maybe g_socket_multicast_group_operation_ssm() needs the same #ifdef ?
Oops, forgot to include the error message: gsocket.c: In function 'g_socket_multicast_group_operation_ssm': gsocket.c:2400:33: error: request for member 's_addr' in something not a structure or union mc_req_src.imr_interface.s_addr = g_htonl (INADDR_ANY);
Do you have a patch? You obviously seem able to test this.
I have to admit I have no idea what this code does, and what it should do. I hope Julien Isorce will tell what it should do.
(In reply to Xavier Claessens from comment #29) > Maybe g_socket_multicast_group_operation_ssm() needs the same #ifdef ? Please try to add an early exit #ifdef HAVE_IP_MREQN with error message similar as line 2224 cause there is no way to set the sourceaddr with struct ip_mreqn. Thx!
Created attachment 371159 [details] [review] Meson: Use cc.has_type() instead of our own snippet
Created attachment 371160 [details] [review] gsocket: Fix build error on Android imr_interface.s_addr is not defined in the HAVE_IP_MREQN case.
Disclaimer: I have no idea what I'm doing here, but it builds.
Review of attachment 371159 [details] [review]: Looks reasonable
(In reply to Philip Withnall from comment #37) > Review of attachment 371159 [details] [review] [review]: > > Looks reasonable Waiting for Julien to +1 that patch since we are not sure what we are doing here, or are you ok with that patch?
Review of attachment 371160 [details] [review]: Looks good to me, if not def then it falls back to the existing error case so no need the early exit I mentioned, nice!
Attachment 371160 [details] pushed as 994dd17 - gsocket: Fix build error on Android
Oh, wait, HAVE_IP_MREQN is defined on linux too. So I think I won't work anymore on linux.
Sorry, that was clearly the wrong fix: The ip_mreqn structure is available only since Linux 2.2. For compatibility, the old ip_mreq structure.
Ok, I think I understand better the reason of the build error on Android: sysroot/usr/include/linux/in.h has this: struct ip_mreq_source { __be32 imr_multiaddr; __be32 imr_interface; __be32 imr_sourceaddr; }; so imr_interface is not a struct/union with s_addr member. wtf?
Sorry for the noise, but I finally found the root cause; it's a bug in Android NDK! https://issuetracker.google.com/issues/36987220 Last comment says: "this should be fixed in NDK r17.". r17 is still beta release....
I reverted the commit in glib master.
OK, you could add an `#if !defined(__BIONIC__)` around the IGMPv3 SSM introduced in this bug with a FIXME comment pointing to this bug. If there’s a way to check in the preprocessor what version of the NDK is being used, you could make the check more specific to < r17.
Created attachment 371280 [details] [review] struct ip_mreq_source definition is broken on Android NDK <= r16 This fix the build on Android r16 and older, see: https://issuetracker.google.com/issues/36987220
Created attachment 371281 [details] [review] struct ip_mreq_source definition is broken on Android NDK <= r16 This fix the build on Android r16 and older, see: https://issuetracker.google.com/issues/36987220
btw, we really should have android/mingw CI for glib. Is it possible to setup with gitlab ?
Review of attachment 371281 [details] [review]: OK. Please push to master and glib-2-56. Thanks.
# Bug 740791 - gio: cannot specify the source when joining a multicast group (IGMPv3 SSM Source Specific Multicast RFC 4604) - REOPENED Attachment 371281 [details] pushed as 7efd76d - struct ip_mreq_source definition is broken on Android NDK <= r16