GNOME Bugzilla – Bug 596192
[PATCH] gdesktopappinfo.c: improvement of MIME caching code
Last modified: 2017-10-05 11:56:08 UTC
Created attachment 143910 [details] glib-mime-caching-rewrite.patch MIME caching code now contains four instances of nearly equal code. Attached patch merges all four instances to a single instance allowing simpler future enhancements (e. g. desktop-specific defaults). It attempts to prevent code in style "a = g_new(...); do_something(&a); b = strdup (a); g_free (a);" It also fixes bug 595972. The new code is about 130 lines shorter and has exactly the same memory requirements as the current one.
I've committed the more minimal fix for 595972 for now. This one looks a bit harder to review...
Yes, the new code is not as straightforward as the previous. The new code uses constant descriptor MimeInfoCacheDirItemSpec of the cache operation: file names, callbacks for hash table constructor (adding to table) and destructor and NULL terminated array of expected key file group as a last argument. The dynamic counterpart of this structure is the MimeInfoCacheDirItem. It contains just a timestamp and an array of hash tables (one per defined key file group). It means, that MimeInfoCacheDirItem has not static effective size (it depends on number of items in groups spec. That is why each instance of MimeInfoCacheDirItem needs aditional space (pad) for group hash table pointers. The unified caching function moved some g_free calls to the constructor. I used valgrind desktop-app-info (from tests directory) to verify, that this approach did not introduce memory leaks.
Created attachment 144829 [details] [review] glib-mime-caching-rewrite.patch Rebased patch after fix of bug 595972. Note: The code "We are appending to a non-empty GList" is a bit tricky. Adding new GHashTable API functions (e. g. g_hash_table_update: just to update has table value without calling of destructors or g_hash_table_lookup_value_ref to get reference to the stored value) could make the code less tricky without executing extra code.
Please don't apply the patch yet. It seems that it loses some non-default applications in the list. I am debugging this problem.
Stanislav: any progress?
Stanislav, would be sad to let the patch bitrot. Do you have any news?
Not yet. Moving higher in my todo list. I have to debug the patch and fixed the problem introduced (comment 4). The main purpose of the patch is the future enhancememt: Allow desktop-specific defaults.
Created attachment 169961 [details] [review] glib-mime-caching-rewrite.patch It seems that I found a bug in my previous patch. Now it seems to work properly. I will continue in testing. Notes: - global_defaults_cache seems to be unused. - The main purpose of the patch: make possible to create arbitrary number of MIME sources to allow desktop specific defaults files in future.
Comment about the "ignore the return value of g_list_append" hack was added as GHashTable suggestion in bug 630336.
Ping. Is there any progress in this patch review? It blocks bug 630171, which can provide feature of smart and desktop environment specific MIME defaults together with https://bugs.freedesktop.org/show_bug.cgi?id=30214
FWIW, I do want to commit the feature in desktop-file-utils, so it'd be great to get the glib patches reviewed. Stanislav: it looks like the patch doesn't apply to glib master anymore :/
Created attachment 179516 [details] [review] glib-mime-caching-rewrite2.patch Patch ported to the latest glib. It is a conservative port, but the MimeInfoCacheDirItemSpec had to be changed (mimeapps.list now contains 3 groups, 2 of them contains string lists, one of them contains strings). Note: This patch itself does not implement desktop specific MIME defaults. It is done by the patch in the bug 630171. But it depends on code written in this patch.
Review of attachment 179516 [details] [review]: ::: gio/gdesktopappinfo.c @@ +2374,3 @@ + /* destructor that deletes MimeInfoCacheDirItem map item */ + void (*destructor) (gpointer value); +} MimeInfoCacheDirItemSpecGroup; I don't like this constructor/destructor business. The entire thing seems overcomplicated, with 5 different structs... @@ +2403,3 @@ + .destructor = (MimeInfoCacheDirItemDestructor) destroy_info_cache_value + } , { + .id = NULL No C99, please. @@ +2463,3 @@ + MimeInfoCacheDirItem mimeapps_list_diritem; + /* map key = MIME Type, value = desktop file name */ + MimeInfoCacheDirItemMapPad _mimeapps_list_diritem_pad[3]; These inline comments make the struct entirely unreadable. I'd prefer that in a doc comment above. And whats that pad stuff good for ? Its not like this struct is public abi, so no need to keep struct layout fixed.
I'll change comments as you want. The pad stuff: I need an array of variable size. The array has the number of elements equal to number of MimeInfoCacheDirItemSpecGroup elements in the specification. That is why I declare the size of the array as [0] and then allocate needed number of elements by these pads. If there exists somethinge better in the plain C language, I'll rewrite it.
Created attachment 179738 [details] [review] mime-caching-rewrite3.patch The patch above with comments converted to the standard comment style. There are no code changes since mime-caching-rewrite2.patch.
Created attachment 198867 [details] [review] glib-mime-caching-rewrite4.patch Rebased version of the patch that applies to the current git snapshot. It also fixes 3 issues: - Memory corruption in mime_info_cache_init_desktop_defaults() (insufficient allocation) - Obsolete code in mime_info_cache_dir_build_cache() that caused critical warnings. - Incorrect processing of "Default Applications": Now they are value lists in defaults.list and single string in mimeapps.list exactly as it was in the original code (it also fixes memory leak in mime_info_cache_dir_item.map[0] (mimeapps_list_defaults_map of the original code) destruction (previous version freed only mimeapps.list "Default Applications" value array, not strings itself). The new code introduces value_is_list boolean that makes possible to work with both arrays and strings.
Review of attachment 198867 [details] [review]: The complexity of this patch scares me, and in the intervening 6 years, the underlying gdesktopappinfo.c code has changed quite a lot, so it doesn’t apply. Sorry; I’m going to reject and close this bug. Please re-open it if you want to rebase and update the patch to apply against current git master. I would be more likely to accept it confidently if it added a load more unit tests for the MIME cache.