Created attachment 217060 [details] [review]
Use the glib-2.32 multicast features. make check passed.

(In reply to comment #0)
> Glib-2.32 has introduced a bunch of multicast functions which can replace the
> in-house similar functions defined in gssdp-socket-function.c. A benefit of
> using GLib-2.32 functions is that they support ipv6.
> 
> The attached patch will use #ifdef #else to make a try to use the 2.32
> functions meanwhile keep the old functionality.

Haven't yet looked at the patches but lots of respect points for initiating this effort. I would rather want to not use #ifdefs here at all and just bump our glib dependency. People who use old glib are free to keep using old gupnp. I must point out that 2.32 is a stable release that is already in latest releases of distros so not the same as requring latest unstable release or git master..

So just to be clear, once you have removed the #ifdefs, I'll review the patch.

Review of attachment 217060 [details] [review]:

Nice work, I agree on the #ifdefs with Zeeshan, though.

::: libgssdp/gssdp-resource-browser.c
@@ -468,3 +468,3 @@
                                   version + 1,
-                                  G_REGEX_MATCH_ANCHORED,
-                                  0)) {
+                                  G_REGEX_ANCHORED,
+                                  G_REGEX_MATCH_ANCHORED)) {

This is a separate bug, can you extract this patch please? Good catch.

::: libgssdp/gssdp-socket-source.c
@@ +301,3 @@
+                /* Subscribe to multicast channel */
+#ifdef GLIB_VERSION_2_32
+                struct ifaddrs *addrs, *iap; 

I don't really like this unix-specific stuff here, and it's redundant work as we already have the interface's name in GSSDPClient. This is internal API, so you freely change the functions and pass the name instead of the address.

Just want to make it clear. As to your last paragraph, did you mean the function 'get_host_ip'? Did you suggest changing this function or write another based on this one?

No, what I mean is that the ifa->ifa_name is available in GSSDPClient->priv->iface after init_network_info has been called, which is before any of the socket sources is created.

So you can either pass it to gssdp_socket_source_new instead of gssdp_client_get_host_ip or in addition to it if you still need the IP.

Created attachment 217340 [details] [review]
Use the code in GSSDPClient to obtain the name of network interface

This patch is based on Jens suggestion.

Review of attachment 217340 [details] [review]:

I think gssdp-socket-functions.[ch] are useless now so the two files can go away.

I'll need to try the patches on Windows and OpenBSD and see if it's still working there.

::: libgssdp/gssdp-socket-source.c
@@ +32,3 @@
+#include <net/if.h>
+#include <ifaddrs.h>
+#endif

I think those includes aren't necessary anymore

@@ +43,1 @@
 static void

Same here. Do you need them?

@@ +273,3 @@
         if (self->priv->type == GSSDP_SOCKET_SOURCE_TYPE_MULTICAST) {
+                /* The 4th argument 'iface_name' can't be NULL even though Glib API doc says you 
+                 * can. 'NULL' will fail the test.

Yes. We simulate that functionality in GSSDPClient by semi-arbitrarily choosing an interface  so you're correct that here the function always has to use the interface provided by it.

The patch has trailing whitespace, btw.

(In reply to comment #8)
> Review of attachment 217340 [details] [review]:
> 
> I think gssdp-socket-functions.[ch] are useless now so the two files can go
> away.
There are still functions called by gssdp-socket-source.c

Do we still need to call gssdp_socket_mcast_interface_set() in gssdp-socket-source.c: around line 215? I removed this call temporarily and the test seemed OK.

> 
> I'll need to try the patches on Windows and OpenBSD and see if it's still
> working there.
> 
> ::: libgssdp/gssdp-socket-source.c
> @@ +32,3 @@
> +#include <net/if.h>
> +#include <ifaddrs.h>
> +#endif
> 
> I think those includes aren't necessary anymore
No. They should be removed.

> 
> @@ +43,1 @@
>  static void
> 
> Same here. Do you need them?
I have no idea.

> 
> @@ +273,3 @@
>          if (self->priv->type == GSSDP_SOCKET_SOURCE_TYPE_MULTICAST) {
> +                /* The 4th argument 'iface_name' can't be NULL even though
> Glib API doc says you 
> +                 * can. 'NULL' will fail the test.
> 
> Yes. We simulate that functionality in GSSDPClient by semi-arbitrarily choosing
> an interface  so you're correct that here the function always has to use the
> interface provided by it.

Seems to work under Windows. OpenBSD still to check.

Created attachment 221733 [details] [review]
This is the third attempt to use glib-2.32 multicast functions

Merged with the HEAD of master.


The biggest difference compared to the previous attempted patches:
I removed the files gssdp-socket-function.[ch]. It worked on my Ubuntu Linux. For the function gssdp_socket_reuse_address (), it seems that glib/gio has already taken care of the difference between Windows and Linux (see the function body of g_socket_bind () in gio/gsocket.c). Therefore I deleted it. I have no idea if it will work in OpenBSD.

I am not sure we can skip the SO_REUSEADDR. gsocket.c says "You don't want to do this on windows". Yes, it does mean something else on windows, but we need it there since the multicast address + port are definitely taken by its SSDP system service.

And I think we need gssdp_socket_mcast_interface_set as it sets the correct source interface instead of using the default mcast interface, which is necessary in multi-home scenarios. I missed that question before, sorry.

Created attachment 221758 [details] [review]
The fourth attempt to use the multicast socket in glib-2.32 

Per Jens' request, add back gssdp_socket_mcast_interface_set () and gssdp_socket_reuse_address ()

Committed with minor changes due to changed upstream code