From: Evan S. <ev...@ad...> - 2006-09-26 11:22:29
|
I was working on fixing Yahoo profile/icon lookups to be cancellable, to prevent crashes after a disconnection like: Thread 0 Crashed: 0 Libgaim 0x0798992d yahoo_buddy_icon_upload_connected 97 (yahoo_picture.c:455) 1 Libgaim 0x07904731 gaim_proxy_connect_data_connected 32 (proxy.c:383) 2 Libgaim 0x079047d5 socket_ready_cb 118 (proxy.c:425) 3 com.apple.CoreFoundation 0x908393f3 __CFSocketDoCallback 551 Since there can be any number of such requests going (I believe), my plan was to have a GSList of GaimProxyConnectData structs associated with the prpl's connection-specific data (yahoo_data struct). However, I can't see how to remove the right GaimProxyConnectData from the list when the connect is successful (immediately after which it will be free()'d by proxy.c). Should the connect callback receive the GaimProxyConnectData back as a parameter to use if necessary, or am I missing something? It looks like code elsewhere has a 1:1 relationship between some object (e.g. a directconn in the msn prpl) and its GaimProxyConnectData. -Evan |
From: Eduardo <ep...@us...> - 2006-09-26 14:54:36
|
On 2006-09-26 11:22:18 UTC, Evan Schoenberg wrote: > I was working on fixing Yahoo profile/icon lookups to be cancellable, > to prevent crashes after a disconnection like: > Thread 0 Crashed: > 0 Libgaim 0x0798992d > yahoo_buddy_icon_upload_connected 97 (yahoo_picture.c:455) > 1 Libgaim 0x07904731 > gaim_proxy_connect_data_connected 32 (proxy.c:383) > 2 Libgaim 0x079047d5 socket_ready_cb 118 > (proxy.c:425) > 3 com.apple.CoreFoundation 0x908393f3 __CFSocketDoCallback > 551 > > Since there can be any number of such requests going (I believe), my > plan was to have a GSList of GaimProxyConnectData structs associated > with the prpl's connection-specific data (yahoo_data struct). > However, I can't see how to remove the right GaimProxyConnectData > from the list when the connect is successful (immediately after which > it will be free()'d by proxy.c). Should the connect callback receive > the GaimProxyConnectData back as a parameter to use if necessary, or > am I missing something? It looks like code elsewhere has a 1:1 > relationship between some object (e.g. a directconn in the msn prpl) > and its GaimProxyConnectData. I think using a GSList of GaimProxyConnectData is a bad idea. I do not see why you need to keep a list of GaimProxyConnectData. Could you describe the problem (crash)? I was looking the other day to this web page: http://www.artima.com/weblogs/viewpost.jsp?thread=168511 and saw this inside yahoo_buddy_icon_upload_connected(): if (!d) return; That seems like bad code. |
From: John B. <rek...@re...> - 2006-09-26 15:08:31
|
> I think using a GSList of GaimProxyConnectData is a bad idea. > I do not see why you need to keep a list of GaimProxyConnectData. > > Could you describe the problem (crash)? > > I was looking the other day to this web page: > http://www.artima.com/weblogs/viewpost.jsp?thread=168511 > and saw this inside yahoo_buddy_icon_upload_connected(): > if (!d) > return; > That seems like bad code. This is hardly bad code. It is a simple check to return from the function if d is null and is needed because it is possible NOT to have a buddy icon to send to the server. It makes far more sense to have the null check in this function than to check for null everywhere the function is called, which would be the alternative. John |
From: Eduardo <ep...@us...> - 2006-09-26 15:25:18
|
On 2006-09-26 15:09:12 UTC, John Bailey wrote: > > I think using a GSList of GaimProxyConnectData is a bad idea. > > I do not see why you need to keep a list of GaimProxyConnectData. > > > > Could you describe the problem (crash)? > > > > I was looking the other day to this web page: > > http://www.artima.com/weblogs/viewpost.jsp?thread=168511 > > and saw this inside yahoo_buddy_icon_upload_connected(): > > if (!d) > > return; > > That seems like bad code. > > This is hardly bad code. It is a simple check to return from the function if d > is null and is needed because it is possible NOT to have a buddy icon to send to > the server. It makes far more sense to have the null check in this function > than to check for null everywhere the function is called, which would be the > alternative. I do not know if you read the web page and the code. But why would you call that function if you do not want to upload an icon? That NULL check does not seem to be needed in any code path anyway. |
From: John B. <rek...@re...> - 2006-09-26 15:56:01
|
> I do not know if you read the web page and the code. > But why would you call that function if you do not want to upload an icon? > > That NULL check does not seem to be needed in any code path anyway. I did read the article and the code before I replied, although this fact is unimportant to the discussion at hand. The mistake in the article is that the author is making blanket assumptions. "Bad" and "good" code are highly subjective when it comes to working code. In my experience, it is far more useful to approach the code asking yourself if it is necessary or right for the situation. Granted, this too is a subjective decision, particularly if you're not the one who wrote the code or don't have any understanding of the underlying protocol this code deals with. Knowing how to send a message, how to start Gaim and use it, or knowing what server to connect to does not count as understanding the protocol. While it currently *appears* that the NULL check may not be needed, that is no guarantee that it hasn't been needed in the past and won't be needed in the future. John |
From: Joshua B. <jbl...@ma...> - 2006-09-26 15:28:18
|
John Bailey wrote: > > I was looking the other day to this web page: > > http://www.artima.com/weblogs/viewpost.jsp?thread=3D168511 > > and saw this inside yahoo_buddy_icon_upload_connected(): > > if (!d) > > return; > > That seems like bad code. >=20 > This is hardly bad code. It is a simple check to return from the functio= n if d=20 > is null and is needed because it is possible NOT to have a buddy icon to = send to=20 > the server. It makes far more sense to have the null check in this funct= ion=20 > than to check for null everywhere the function is called, which would be = the=20 > alternative. Oh come now - are you saying that random weblogs could possibly be wrong when they spew forth Rules for Coding? Surely you aren't implying that individual implementations might have reason to do something that Michael Feathers would blanketly label as bad... Kids these days. --jtb |
From: <si...@km...> - 2006-09-26 15:33:29
|
Joshua Blanton wrote: > John Bailey wrote: >>> I was looking the other day to this web page: >>> http://www.artima.com/weblogs/viewpost.jsp?thread=3D168511 >>> and saw this inside yahoo_buddy_icon_upload_connected(): >>> if (!d) >>> return; >>> That seems like bad code. >> This is hardly bad code. It is a simple check to return from the func= tion if d=20 >> is null and is needed because it is possible NOT to have a buddy icon = to send to=20 >> the server. It makes far more sense to have the null check in this fu= nction=20 >> than to check for null everywhere the function is called, which would = be the=20 is called only 2 times in whole gaim... >> alternative. >=20 > Oh come now - are you saying that random weblogs could possibly be > wrong when they spew forth Rules for Coding? Surely you aren't > implying that individual implementations might have reason to do > something that Michael Feathers would blanketly label as bad... this is maybe, hardly say wath is goog and what is wrong. On bigger projects in work I learned that these small "evils" can be very dangerous later if you ignore them. but generally I have to agree that it's a bit kiddy.. --=20 Peter =C5=A0i=C5=A1ka e-mail/(MSN): sisken (at) kmit (dot) sk e-mail/(Jabber): sisiacik (at) gmail (dot) com tel: 0904/942744 ICQ#: 204077255 |
From: Evan S. <ev...@ad...> - 2006-09-26 15:14:16
|
On Sep 26, 2006, at 10:53 AM, Eduardo P=E9rez wrote: > I think using a GSList of GaimProxyConnectData is a bad idea. > I do not see why you need to keep a list of GaimProxyConnectData. > > Could you describe the problem (crash)? > 1) yahoo_buddy_icon_upload() is called. This calls gaim_proxy_connect=20 () to begin a connection process; yahoo_buddy_icon_upload_connected() =20= is the callback to be triggered when the connection is complete 2) The account is disconnected. 3) gaim_proxy_connect() finishes the connection. It calls =20 yahoo_buddy_icon_upload_connected(), passing it back the =20 yahoo_buddy_icon_upload_data struct associated with the original =20 gaim_proxy_connect() call. This struct has been free()'d because the =20 account was disconnected. (Additionally, if it hadn't been, it would =20 have a pointer to a GaimConnection which has now been freed). 4) Crash This problem exists within Gaim anywhere that the connect attempt is =20 not guaranteed to return immediately and is not cancelled when the =20 account disconnects. The GaimProxyConnectData struct is necessary to cancel the connection =20= attempt via the proxy.c API. Therefore, when the Yahoo account disconnects, it needs to cancel all =20= attempts it initiated. To do so it needs to keep track of them; =20 hence, the GSList. Better, though, looking it in that way, would be to have proxy.c keep =20= track of handles for accounts, as the request and notify APIs do, and =20= automatically cancel all pending requests when the account =20 disconnects. Mark, what do you think about that? > I was looking the other day to this web page: > http://www.artima.com/weblogs/viewpost.jsp?thread=3D168511 > and saw this inside yahoo_buddy_icon_upload_connected(): > if (!d) > return; > That seems like bad code. Indeed. -Evan= |
From: Chris S. <sou...@no...> - 2006-09-26 15:21:28
|
On Tue, 2006-09-26 at 14:53 +0000, Eduardo P=C3=A9rez wrote: > I was looking the other day to this web page: > http://www.artima.com/weblogs/viewpost.jsp?thread=3D168511 > and saw this inside yahoo_buddy_icon_upload_connected(): > if (!d) > return; > That seems like bad code. Have you ever written code? I mean if you know there should always be a valid value for d then an ASSERT would be more appropriate, otherwise a simple NULL check is vastly preferable to crashing the application. I wasted valuable seconds reading this email before I remembered I can just put your email address into my spam filter. regards Chris |
From: Mark D. <ma...@ki...> - 2006-09-26 16:43:25
|
On Tue, 26 Sep 2006 07:22:18 -0400, Evan Schoenberg wrote > I was working on fixing Yahoo profile/icon lookups to be cancellable, > to prevent crashes after a disconnection like: Thread 0 Crashed: 0 > Libgaim 0x0798992d > yahoo_buddy_icon_upload_connected 97 (yahoo_picture.c:455) 1 > Libgaim 0x07904731 > gaim_proxy_connect_data_connected 32 (proxy.c:383) 2 Libgaim > 0x079047d5 socket_ready_cb 118 > (proxy.c:425) 3 com.apple.CoreFoundation 0x908393f3 > __CFSocketDoCallback 551 > > Since there can be any number of such requests going (I believe), my > plan was to have a GSList of GaimProxyConnectData structs > associated with the prpl's connection-specific data (yahoo_data > struct). However, I can't see how to remove the right > GaimProxyConnectData from the list when the connect is successful > (immediately after which it will be free()'d by proxy.c). Should > the connect callback receive the GaimProxyConnectData back as a > parameter to use if necessary, or am I missing something? It looks > like code elsewhere has a 1:1 relationship between some object > (e.g. a directconn in the msn prpl) and its GaimProxyConnectData. > > -Evan Hmm, is this the code for uploading your own icon? It seems like you would only want to have one of those at a time? If someone sets their icon twice in quick succession, you could just cancel the first one. If I'm wrong, then yes a GSList of GaimProxyConnectData structs is a good idea. Or maybe even a hash table, but that seems like overkill. In any case, having the proxy code pass back GaimProxyConnectData as a parameter is probably a good idea. When I did the cancelable stuff I thought about being able to pass in a handle/reference thingy like the notify and request APIs, and then you could call something like gaim_proxy_request_close_with_handle(gc), but that seemed a bit over engineered to me. I figured I could always do it this way and if it started to get messy then I could change it. I guess I would suggest first changing Yahoo! to keep track of the GaimProxyConnectData(s), and if the code is really ugly or people want to use the handle paradigm then we can change it. -Mark |
From: Evan S. <ev...@ad...> - 2006-09-26 21:07:47
|
On Sep 26, 2006, at 12:47 PM, Mark Doliner wrote: > Hmm, is this the code for uploading your own icon? It seems like > you would > only want to have one of those at a time? If someone sets their > icon twice in > quick succession, you could just cancel the first one. > > If I'm wrong, then yes a GSList of GaimProxyConnectData structs is > a good > idea. Or maybe even a hash table, but that seems like overkill. Looking at the code a second time, I see that I was mixing up the yahoo_send_picture_*() and yahoo_buddy_icon_upload_*() functions. The former can have several in progress at once, appear to send directly via the yahoo connection, and don't do a gaim_proxy_connect (). The latter do a gaim_proxy_connect() and are for uploading one's own buddy icon to the server -- so you're right, you would only want one at a time. > In any case, having the proxy code pass back GaimProxyConnectData as a > parameter is probably a good idea. Agreed. > > When I did the cancelable stuff I thought about being able to pass > in a > handle/reference thingy like the notify and request APIs, and then > you could > call something like gaim_proxy_request_close_with_handle(gc), but > that seemed > a bit over engineered to me. I figured I could always do it this > way and if > it started to get messy then I could change it. I guess I would > suggest first > changing Yahoo! to keep track of the GaimProxyConnectData(s), and > if the code > is really ugly or people want to use the handle paradigm then we > can change it. Without having to add and remove from a list, the fix should be quite clean here, as it has been elsewhere previously. It wouldn't be bad for all this to be handled by the core automatically rather than for each prpl to have to deal with it... but I wouldn't remove the current implementation because it's very nice to be able to cancel selectively at will -- as we see in this case, where a second buddy icon set should cancel a first if it is in-progress. The current implementation is Good and seems to be sufficient. :) -Evan |
From: Richard L. <rl...@wi...> - 2006-09-26 17:40:58
|
On Tue, 2006-09-26 at 14:53 +0000, Eduardo P=C3=A9rez wrote: > I was looking the other day to this web page: > http://www.artima.com/weblogs/viewpost.jsp?thread=3D168511 > and saw this inside yahoo_buddy_icon_upload_connected(): > if (!d) > return; > That seems like bad code. I've changed this to a g_return_if_fail(), and added a similar check to yahoo_buddy_icon_upload(). Here's my commit message, which I think sums everything up: After the comments on gaim-devel about 'bad' code, I looked into the function referenced. It appears that yahoo_buddy_icon_upload_connected() is only called as a callback. It's registered from yahoo_buddy_icon_upload(). yahoo_buddy_icon_upload() is only called twice. In neither case can a NULL make it down to yahoo_buddy_icon_upload_connected(). As this is an exceptional case rather than a normal one, a g_return_if_fail() is more appropriate. That adds logging, makes the intent clearer, and will help the Coverity software detect violations of this assumption. Richard |
From: Tim R. <tim...@gm...> - 2006-09-27 02:28:47
|
On 9/26/06, Richard Laager <rl...@wi...> wrote: > > I've changed this to a g_return_if_fail(), and added a similar check to > yahoo_buddy_icon_upload(). Here's my commit message, which I think sums > everything up: > > After the comments on gaim-devel about 'bad' code, I looked into the > function referenced. It appears that yahoo_buddy_icon_upload_connected() > is only called as a callback. It's registered from > yahoo_buddy_icon_upload(). yahoo_buddy_icon_upload() is only called > twice. In neither case can a NULL make it down to > yahoo_buddy_icon_upload_connected(). As this is an exceptional case > rather than a normal one, a g_return_if_fail() is more appropriate. That > adds logging, makes the intent clearer, and will help the Coverity > software detect violations of this assumption. I don't remember why I have that check there. Perhaps I did call it with NULL or some point, or was just being overly paranoid. Or maybe I wasn't thinking clearly, and erroneously thought a not null check would save me from invalid memory. Looking at it now, it probably does make more sense as a g_return_if_fail(). --Tim |