[Nice] patch for g_io_create_watch glib bug

Wed Feb 29 07:43:58 PST 2012

Sorry, I forgot to check error code for receiving ICMP messages.
Here is the fix. Please join the two patches together, if possible.

Regards,
Livio

-----Original Message-----
From: nice-bounces+livio.madaro=telecomitalia.it at lists.freedesktop.org [mailto:nice-bounces+livio.madaro=telecomitalia.it at lists.freedesktop.org] On Behalf Of Madaro Livio
Sent: mercoledì 29 febbraio 2012 15.11
To: nice at lists.freedesktop.org
Subject: Re: [Nice] patch for g_io_create_watch glib bug

Hi,
I ported libnice to use GSocket. The function g_socket_create_source works fine on Windows. No problem doing multiple calls on the same socket. I also removed many #ifdef from the source code.

Attached you can find the patch for review. I tested it on Windows only.

Thanks for the idea of using GSocket.

Livio

-----Original Message-----
From: nice-bounces+livio.madaro=telecomitalia.it at lists.freedesktop.org [mailto:nice-bounces+livio.madaro=telecomitalia.it at lists.freedesktop.org] On Behalf Of Madaro Livio
Sent: lunedì 27 febbraio 2012 17.07
To: nice at lists.freedesktop.org
Subject: Re: [Nice] patch for g_io_create_watch glib bug

Hi Youness,
fixing glib would be the best solution but I think it is not easy. The problem comes from a limitation of WSAEventSelect(...) (see http://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx ). WSAEventSelect  is called by g_io_create_watch and MSDN says "Issuing a WSAEventSelect for a socket cancels any previous WSAAsyncSelect or WSAEventSelect for the same socket". 
I'm not a glib developer but I think that a small refactoring of the glib code could be required to fix the bug.

I think that using GSocket is easier than patching glib. I didn't know that GSocket had a different code from glib g_io_create_watch.

I'm trying to use g_socket_create_source to check if it fix the problem. I'll let you know...

Regards,
Livio

-----Original Message-----
From: nice-bounces+livio.madaro=telecomitalia.it at lists.freedesktop.org [mailto:nice-bounces+livio.madaro=telecomitalia.it at lists.freedesktop.org] On Behalf Of Youness Alaoui
Sent: venerdì 24 febbraio 2012 2.33
To: nice at lists.freedesktop.org
Subject: Re: [Nice] patch for g_io_create_watch glib bug

Hi,

That's an interesting bug. I didn't know about this limitation of glib.
Refactoring libnice in order to have a single watch would be impractical because
of the way it's done.
While I'm inclined to accept this patch as a temporary workaround, I've
discussed it with a colleague who said it shouldn't be merged, but instead a
patch to glib should be made. I will agree with him on this one, as I think that
adding workaround over workaround will add complexity to the code in the long
term, and since this is quite an ugly hack (I admit not having any other
solution for it), it is best to leave it out.
My suggestion to you then would be to fix it directly in glib and submit a patch
to the glib team instead.
This bug has been present and known since 2006, but no proper patch has been
submitted for it yet, so if you could fix it once and for all, many people would
benefit from that.
Here's the related bug : https://bugzilla.gnome.org/show_bug.cgi?id=338943

On another note, we are starting to get a lot of #ifdefs for windows, and it's
making the code less maintainable, my colleague, Olivier Crete, suggested using
GSocket. I didn't know that existed, but I think it would be greatly beneficial
to port libnice to using GSocket instead of normal sockets with lots of #ifdefs.
If you are up for it, it would be great if you could send a patch that does
that. There might even be a fix for this g_io_create_watch inside g_socket. I
can see that it doesn't use g_io_create_watch, but uses something else based on
pollfd and a concatenation of all requested conditions, so it's possible that it
fixes it.
See http://developer.gnome.org/gio/2.30/GSocket.html and especially
g_socket_create_source.

Let me know what you think.

Thanks,
Youness

On 02/23/2012 12:14 PM, Madaro Livio wrote:
> Hi,
> Here is the second one for review.
> 
> The glib function g_io_create_watch has a bug on Windows: multiple call to g_io_create_watch on the same socket won't work (https://bugzilla.gnome.org/show_bug.cgi?id=338943 )
> 
> Libnice calls  g_io_create_watch two times: the first one to set the callback for the socket condition G_IO_IN, the second one for the socket condition G_IO_OUT when the socket send returns WSAEWOULDBLOCK and data is queued for retransmission. The problem is that calling g_io_create_watch the second time "delete" the previous one and the callback for G_IO_IN will never be called.
> 
> The patch is a workaround that uses a polling timer to avoid the second g_io_create_watch call.
> I know polling is not a very good solution, but it is simple (I modified just few lines of code) otherwise libnice should be modified to call g_io_create_watch just one time.
> 
> Regards,
> Livio
> 
> 
> 
> 
> 
> 
> 
> 
> 
> Questo messaggio e i suoi allegati sono indirizzati esclusivamente alle persone indicate. La diffusione, copia o qualsiasi altra azione derivante dalla conoscenza di queste informazioni sono rigorosamente vietate. Qualora abbiate ricevuto questo documento per errore siete cortesemente pregati di darne immediata comunicazione al mittente e di provvedere alla sua distruzione, Grazie.
> 
> This e-mail and any attachments is confidential and may contain privileged information intended for the addressee(s) only. Dissemination, copying, printing or use by anybody else is unauthorised. If you are not the intended recipient, please delete this message and any attachments and advise the sender by return e-mail, Thanks.
> 
> 
> 
> 
> _______________________________________________
> nice mailing list
> nice at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nice

_______________________________________________
nice mailing list
nice at lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nice
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-error-check-for-receiving-ICMP.patch
Type: application/octet-stream
Size: 883 bytes
Desc: 0002-error-check-for-receiving-ICMP.patch
URL: <http://lists.freedesktop.org/archives/nice/attachments/20120229/888a1836/attachment.obj>