[Nice] patch for g_io_create_watch glib bug

Thu Mar 1 18:33:35 PST 2012

Hi Madaro,

Thanks again for the contribution!
I'm reviewing it and I found a few issues with the patch (which I've all fixed
and committed, so you don't have to follow up on them). Here are my comments if
you are interested :
1 - You're not following the coding style of the rest of the files, specifically :
  - a space between function name and args : "func ();" instead of "func();"
  - Opening { for if statements should be in the same line as the if
  - You had a few indentation misconcistencies
  - You added a few lines with trailing spaces
  - You wrote lines longer than 80 characters
2 - the GSource callback now takes a GSocket * as first argument instead of
GIOChannel
3 - there is no more need for name_len since g_socket only needs to know the
size of the storage that we use (in this case sockaddr_storage)
4 - I added some checks to make sure gaddr is non NULL before calling
g_socket_connect
5 - while you did it right for tcp-bsd, you forgot to fix udp-bsd where you used
g_socket_address_new_from_native as argument to g_socket_bind, thus leaking the
returned object
6 - A small typo, you wrote 'boolean' instead of 'gboolean', which I guess
worked for you since you were compiling it with visual studio, but 'boolean' is
not a valid C type.
7 - The test-bsd.c stopped working because it wasn't calling g_type_init() so
the g_socket_new wasn't working.

Other than that it's well done, thanks! I was actually surprised at how little
changes were needed to switch to g_socket.

Thank you,
Youness.

On 02/29/2012 09:10 AM, Madaro Livio wrote:
> 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
> 
> 
> 
> _______________________________________________
> nice mailing list
> nice at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nice

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nice/attachments/20120301/93bce623/attachment.pgp>