GNOME Bugzilla – Bug 616877
Several issues with g_socket_receive_message
Last modified: 2010-04-26 23:25:22 UTC
Created attachment 159626 [details] [review] Patch Will attach a patch to fix this.
Created attachment 159628 [details] [review] Better patch Actually the code becomes much easier to read if we use GPtrArray. Note that there is no allocation overhead etc. unless there are actual control messages to be handled.
Created attachment 159629 [details] [review] Better patch v2 Previous patched missed the doc fixes.
Comment on attachment 159629 [details] [review] Better patch v2 >- my_messages[index++] = message; >+ if (my_messages == NULL) can you fix your patch to use the same tabs/spaces convention as the surrounding code? (ie, stupid broken emacs default switch to tabs when the indentation reaches 8 spaces) >+ g_ptr_array_foreach (my_messages, (GFunc) g_object_unref, NULL); >+ g_ptr_array_free (my_messages, TRUE); Can't we just unref the messages immediately after deserializing them, and not even bother creating the array? Pre-existing bug noticed while reviewing: the G_OS_WIN32 version needs: if (messages != NULL) *messages = NULL; if (n_messages != NULL) *n_messages = 0; at the end right before "return bytes_received;"
Created attachment 159641 [details] [review] Patch v3 Thanks for the review, here's an improved version. I've also made the Win32 version return an empty array in messages just like the unix version.
Created attachment 159642 [details] [review] Patch v4 Previous patch had some issues. This one is better.
Comment on attachment 159642 [details] [review] Patch v4 >+ if (my_messages == NULL) >+ { >+ *messages = g_new0 (GSocketControlMessage *, 1); >+ } is that really the right thing? you do a vaguely-useless malloc, and force the caller to do a vaguely-useless free. wouldn't it be nicer to just set *messages to NULL? whichever way you go, you should probably update the doc comment to be explicit about it. >- * we have to do it this way if the user ignores the >- * messages so that we will close any received fds. might be nice to keep that info somewhere, so someone doesn't go and optimize out the cmsg parsing later... otherwise looks ok to commit i think
Created attachment 159653 [details] [review] Patch that was committed
(In reply to comment #6) > (From update of attachment 159642 [details] [review]) > >+ if (my_messages == NULL) > >+ { > >+ *messages = g_new0 (GSocketControlMessage *, 1); > >+ } > > is that really the right thing? you do a vaguely-useless malloc, and force the > caller to do a vaguely-useless free. wouldn't it be nicer to just set *messages > to NULL? > > whichever way you go, you should probably update the doc comment to be explicit > about it. I initially thought it was part of the ABI to always return something in *messages but thinking about it this never worked anyway since we used to segfault when no messages were received. I've clarified the docs about it. > >- * we have to do it this way if the user ignores the > >- * messages so that we will close any received fds. > > might be nice to keep that info somewhere, so someone doesn't go and optimize > out the cmsg parsing later... Fixed. > otherwise looks ok to commit i think Committed, thanks for the review: http://git.gnome.org/browse/glib/commit/?id=3ceddd74bb4304e4e9b2e8955b80212108703632 Cheers, David