This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 431167 - some imsettings code review
some imsettings code review
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: im-chooser (Show other bugs)
9
All Linux
low Severity low
: ---
: ---
Assigned To: Akira TAGOH
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-01 00:21 EST by Matthias Clasen
Modified: 2008-06-24 23:02 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-06-24 23:02:10 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Matthias Clasen 2008-02-01 00:21:19 EST
I decided to look over some of the imsettings code. I haven't gone over all of it,
but here are some observations:


imsettings-manager.c:

imsettings_manager_real_set_property: 
 
- no need to call g_object_notify inside a set_property implementation, 
  gobject does it for you. This also affects other set_property 
  implementations throughout the code.

 - no need to protect g_free () with an if (foo != NULL), g_free is NULL-safe.


imsettings_manager_real_get_list:

 - _("No input methods is available on your system.") - the "is" is extraneous.


_start_process:

 - leaks env_list

 -  argv = g_strsplit_set(cmd, " \t", -1) should probably use g_shell_parse_argv() ?

 -  gchar *s = g_strdup_printf("%d", pid);
    write(fd, s, strlen(s));                 leaks s


_update_symlink:

 - there is at least one "goto end" between 

        p = g_path_get_dirname(f);
        n = g_path_get_basename(f);
 
   and
 
        g_free(n);
        g_free(p);
 
   causing these strings to be leaked


imsettings_manager_real_start_im:

 - I think it would be cleaner if imsettings_request_new would take a reference
on the connection,
   and imsettings_manager_real_start_im would unref the connection afterwards. 


imsettings_manager_real_stop_im

 - There is at least one "goto end" between

   oinfo = imsettings_info_new(conffile);

   and 

   if (oinfo)
       g_object_unref(oinfo);

  that will lead to leaking of oinfo
Comment 1 Akira TAGOH 2008-02-01 03:54:37 EST
Thank you for reviewing.
Comment 2 Akira TAGOH 2008-02-08 05:21:11 EST
The above things should be fixed in 0.99.1-1.fc9 except:

(In reply to comment #0)
> imsettings-manager.c:
> 
> imsettings_manager_real_set_property: 
>  
> - no need to call g_object_notify inside a set_property implementation, 
>   gobject does it for you. This also affects other set_property 
>   implementations throughout the code.

missed some cleanup for that. already fixed in svn though. so should appears in
next release.

> imsettings_manager_real_start_im:
> 
>  - I think it would be cleaner if imsettings_request_new would take a reference
> on the connection,
>    and imsettings_manager_real_start_im would unref the connection afterwards. 

Actually the request is being sent through DBusGProxy and it increments the
reference counter to keep the connection alive. so I don't think unref there
would helps for something..
Instead I had unref outside IMSettingsRequest to free it.
Comment 3 Akira TAGOH 2008-02-20 00:10:46 EST
Should be improved in 0.99.2.
Comment 4 Bug Zapper 2008-05-14 00:57:48 EDT
Changing version to '9' as part of upcoming Fedora 9 GA.
More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Note You need to log in before you can comment on or make changes to this bug.