GNOME Bugzilla – Bug 606280
Brought down to its kness when something causes ~/.gtk-bookmarks to be very big
Last modified: 2010-01-12 12:10:38 UTC
Originally reported here: https://bugs.launchpad.net/ubuntu/+source/gnome-panel/+bug/503868 gnome panel handles big bookmarks files quite badly; I think we should impose a hard limit to keep gnome panel working even if a misbehaving application, or a misguided user fill up ~/.gtk-bookmarks.
Created attachment 150947 [details] [review] impose a hard limit on the number of entries read from ~/.gtk-bookmarks
Review of attachment 150947 [details] [review]: ::: gnome-panel/panel-menu-items.c @@ +375,3 @@ + + if (error) { + g_error_free (error); Since you don't use the content of the error there, just pass NULL to g_io_channel_new, and check if io_channel is NULL or not. @@ +387,3 @@ + gsize length; + gsize terminator_pos; + GIOStatus status; Small nitpick: length/terminator_pos/status should be aligned with contents (not *contents), so add a space ;-) @@ +396,3 @@ + break; + } else if (status != G_IO_STATUS_NORMAL) + break; Just do: if (status != G_IO_STATUS_NORMAL) break; (which means you can pass NULL as error to g_io_channel_read_line) @@ +403,3 @@ + if (terminator_pos > 0) + contents[terminator_pos] = '\0'; + Hrm, do we really need to do this? @@ +404,3 @@ + contents[terminator_pos] = '\0'; + + g_debug("contents: %s", contents); Please remove the g_debug() @@ +420,3 @@ + for (l = lines; l; l = l->next) { + char *line = (char*)l->data; Nitpick: "(char *) l->data;" (add a space after the cast) @@ +462,3 @@ } + + g_free (line); Hrm, doesn't this break the g_hash_table_lookup() call in case you have the same string again? You shouldn't free the memory now, but after the loop (with a g_slist_foreach() call).
Created attachment 151089 [details] [review] second version Thanks for the review! Here's another version.
(In reply to comment #2) > @@ +403,3 @@ > + if (terminator_pos > 0) > + contents[terminator_pos] = '\0'; > + > > Hrm, do we really need to do this? The string read with g_io_channel_read_line contains the \n, which we do not want, so yeah.
Review of attachment 151089 [details] [review]: Please commit with the three changes below, thanks! ::: gnome-panel/panel-menu-items.c @@ +371,3 @@ BOOKMARKS_FILENAME, NULL); + io_channel = g_io_channel_new_file (filename, "r", &error); s/&error/NULL/ else you're leaking the error in case of error ;-) @@ +391,3 @@ + if (status != G_IO_STATUS_NORMAL) + break; + else if (length == 0) Small nitpick: can you just remove the else here? It doesn't matter, but I find it a tiny bit more readable without it. @@ +396,3 @@ + /* Clear the line terminator, if any */ + if (terminator_pos > 0) + contents[terminator_pos] = '\0'; Thanks for the explanation for this! Maybe clarify with the comment: /* Clear the \n line terminator */
Comment on attachment 151089 [details] [review] second version OK! Pushed 5ff272ab5dbdfd3a0c259e333a9dfe006ed0e431 to master with the fixes. Thanks for the reviews!