blist backend patch

Developers
2010-11-27
2013-03-28
  • Jochen De Smet

    Jochen De Smet - 2010-11-27

    Stefan, could you please review the patch at the link below:

    http://pastebin.com/QZqni2gX

    It removes most if not all of the buddy-list related purple calls from sipe.c and replaces them with sipe_backend_* calls; purple implementation is in the patch in src/purple/purple-blist.c, the miranda equivalent is already committed in src/miranda/miranda-blist.c

     
  • Stefan Becker

    Stefan Becker - 2010-11-28

    * Naming of backend API is inconsistent. If they are all buddy functions then they all must start with sipe_backend_buddy_ (sipe_backend_find_buddy, sipe_backend_find_buddies, sipe_backend_request_add, sipe_backend_request_authorization, sipe_backend_group_add)
    * Same for core API (sipe_got_user_status)
    * Same for
             SIPE_INFO -> SIPE_BUDDY_INFO_
             sipe_info_fields -> sipe_buddy_info_fields
             sipe_blist_buddy -> sipe_backend_buddy
    * Please always use "gchar" not "char"
    * "gpointer" instead if "void *"?
    * Did you check your code with Pidgin?
    * Did you verify your changes with a full OBS build? (these are major changes after all)
    * Did you check for memory leaks, e.g. with valgrind?
    * Editorial: your TAB space size seems to be wrong. It should be 8.

    I skipped only cursory over the code. It looks like it does what it is supposed to do.

     
  • Stefan Becker

    Stefan Becker - 2010-11-28

    The update for src/core/Makefile.mingw is missing.

     
  • Jochen De Smet

    Jochen De Smet - 2010-11-28

    * Naming of backend API is inconsistent. If they are all buddy functions then they all must start with sipe_backend_buddy_ (sipe_backend_find_buddy, sipe_backend_find_buddies, sipe_backend_request_add, sipe_backend_request_authorization, sipe_backend_group_add)
    * Same for core API (sipe_got_user_status)
    * Same for SIPE_INFO -> SIPE_BUDDY_INFO_ sipe_info_fields -> sipe_buddy_info_fields sipe_blist_buddy -> sipe_backend_buddy
    * Please always use "gchar" not "char"
    * "gpointer" instead if "void *"?

    Fixed. Changing sipe_backend_group_add to sipe_backend_buddy_group_add feels strange though, since it's adding a group without any buddies at that point.

    I also changed purple-blist.c to purple-buddy.c to be in line with the rest of the names.

    * Did you check your code with Pidgin?

    I checked with finch; I don't normally have the GUI up on my linux box.

    * Did you verify your changes with a full OBS build? (these are major changes after all)

    Trying that now. Are there any updated instructions? I found the official suse docs and some dead links in an earlier thread.
    I managed to get a local OBS checkout; it only shows the tgz though. Do I have to unpack, change, repack, commit and build, or is there some way to just work with the unpacked files ?

    * Did you check for memory leaks, e.g. with valgrind?

    No I didn't. I see the valgrind command line on the dev page, so I'll give it a shot. Is there any specific set of actions I should try, or just use my best judgement in making sure all new code ges called?

    * Editorial: your TAB space size seems to be wrong. It should be 8.

    Also fixed I think. I do most of my editing in vi, which has it set right, but use VS to compile and fix quick errors; I'm not 100% sure I got the settings right there.

     
  • Stefan Becker

    Stefan Becker - 2010-11-28

    I use the OBS files from contrib/ plus the .tar.gz generated by "make distcheck". "osc commit" uploads the file changes and triggers a build.

     
  • Jochen De Smet

    Jochen De Smet - 2010-11-28

    Ok i got passes on CentOS_5, Fedora 12 and 13, Mandriva 2009.1, 2010, Moblin Factory SLE-11 and openSuse11.2, RHEL 5, SLE 11, SLE 11 SP1, openSuse, 11.1 and 11.2 and 11.3 and factory, xUbuntu 8.04 and 9.04 and 9.10 and 10.04.

    I got "unresolvable" on Mandrive 2010.1

    Any others I should have added ?

     
  • Jochen De Smet

    Jochen De Smet - 2010-11-28

    As for valgrind, I see lots of leaks, but only 5 that mention sipe.

    ==8260== 11 bytes in 1 blocks are definitely lost in loss record 1,167 of 3,247   
    ==8260==    at 0x400677E: malloc (vg_replace_malloc.c:195)                        
    ==8260==    by 0x89333F: __vasprintf_chk (in /lib/libc-2.12.90.so)                
    ==8260==    by 0xA2EDF3: g_vasprintf (in /lib/libglib-2.0.so.0.2600.0)            
    ==8260==    by 0xA08112: g_strdup_vprintf (in /lib/libglib-2.0.so.0.2600.0)       
    ==8260==    by 0xA08144: g_strdup_printf (in /lib/libglib-2.0.so.0.2600.0)        
    ==8260==    by 0x4261C73: gentag (sipe-utils.c:69)                                
    ==8260==    by 0x4257539: process_incoming_invite_conf (sipe-conf.c:692)          
    ==8260==    by 0x425EC0E: process_incoming_invite (sipe-incoming.c:303)           
    ==8260==    by 0x425163E: process_input_message (sip-transport.c:1401)            
    ==8260==    by 0x4251947: sip_transport_input (sip-transport.c:1593)              
    ==8260==    by 0x4275764: transport_common_input (purple-transport.c:124)         
    ==8260==    by 0x19D829: ??? (in /usr/lib/libpurple.so.0.7.5)
    

    which I think is a missing g_free(newTag) in process_incoming_invite_conf; unrelated to my change but I assume it should be fixed.

    The other four appear to be related to not properly freeing the buddy menus at shutdown. Unrelated as well I think.

     
  • Stefan Becker

    Stefan Becker - 2010-11-29

    Interesting, the gentag() thing should have been found by Coverity. Fix committed.

    About buddy menus: are you talking about sipe_private->blist_menu_containers? Then your code must not be calling sipe_connection_cleanup(). Otherwise: Pidgin/libpurple/GTK seem to have some window/menu memory leaks not caused by pidgin-sipe.

     
  • Jochen De Smet

    Jochen De Smet - 2010-11-29

    Here's the sections as printed by valgrind, and roughly the line they correspond to; some of those don't seem right though, pointing to empty lines or comments.

    ==8260== 187 (12 direct, 175 indirect) bytes in 1 blocks are definitely lost in loss record 3,037 of 3,247
    ==8260==    at 0x400677E: malloc (vg_replace_malloc.c:195)
    ==8260==    by 0x9EDFA4: g_malloc (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0xA06050: g_slice_alloc (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0x9E2756: g_list_prepend (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0x42684AA: sipe_get_access_levels_menu (sipe.c:6739)
                            member = g_new0(struct sipe_container_member, 1);
    ==8260==    by 0x427281A: sipe_buddy_menu (sipe.c:6788)
            access_domains = sipe_get_access_domains(sipe_private);
    ==8260==    by 0x42479FE: sipe_purple_blist_node_menu (purple-plugin.c:228)
                    return sipe_buddy_menu((PurpleBuddy *) node);
    ==8260==    by 0x805F32C: ??? (in /usr/bin/finch)
    ==8260==    by 0x805F91A: ??? (in /usr/bin/finch)
    ==8260==    by 0xBFBF59: gnt_closure_marshal_BOOLEAN__VOID (in /usr/lib/libgnt.so.0.0.0)
    ==8260==    by 0xB25BE2: g_closure_invoke (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260==    by 0xB380EF: ??? (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260== 315 (12 direct, 303 indirect) bytes in 1 blocks are definitely lost in loss record 3,084 of 3,247
    ==8260==    at 0x400677E: malloc (vg_replace_malloc.c:195)
    ==8260==    by 0x9EDFA4: g_malloc (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0xA06050: g_slice_alloc (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0x9E2756: g_list_prepend (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0x42726CA: sipe_buddy_menu (sipe.c:6588)
             ??? empty line in my sources
    ==8260==    by 0x42479FE: sipe_purple_blist_node_menu (purple-plugin.c:228)
                    return sipe_buddy_menu((PurpleBuddy *) node);
    ==8260==    by 0x805F32C: ??? (in /usr/bin/finch)
    ==8260==    by 0x805F91A: ??? (in /usr/bin/finch)
    ==8260==    by 0xBFBF59: gnt_closure_marshal_BOOLEAN__VOID (in /usr/lib/libgnt.so.0.0.0)
    ==8260==    by 0xB25BE2: g_closure_invoke (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260==    by 0xB380EF: ??? (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260==    by 0xB40FCC: g_signal_emit_valist (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260== 321 (12 direct, 309 indirect) bytes in 1 blocks are definitely lost in loss record 3,085 of 3,247
    ==8260==    at 0x400677E: malloc (vg_replace_malloc.c:195)
    ==8260==    by 0x9EDFA4: g_malloc (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0xA06050: g_slice_alloc (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0x9E2756: g_list_prepend (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0x426854B: sipe_get_access_levels_menu (sipe.c:6745)
                 ???  /* libpurple memory leak workaround */
    ==8260==    by 0x427278C: sipe_buddy_menu (sipe.c:6840)
                 ??? empty line in sipe_get_access_control_menu
    ==8260==    by 0x42479FE: sipe_purple_blist_node_menu (purple-plugin.c:228)
                    return sipe_buddy_menu((PurpleBuddy *) node);
    ==8260==    by 0x805F32C: ??? (in /usr/bin/finch)
    ==8260==    by 0x805F91A: ??? (in /usr/bin/finch)
    ==8260==    by 0xBFBF59: gnt_closure_marshal_BOOLEAN__VOID (in /usr/lib/libgnt.so.0.0.0)
    ==8260==    by 0xB25BE2: g_closure_invoke (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260==    by 0xB380EF: ??? (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260== 666 (12 direct, 654 indirect) bytes in 1 blocks are definitely lost in loss record 3,130 of 3,247
    ==8260==    at 0x400677E: malloc (vg_replace_malloc.c:195)
    ==8260==    by 0x9EDFA4: g_malloc (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0xA06050: g_slice_alloc (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0x9E2756: g_list_prepend (in /lib/libglib-2.0.so.0.2600.0)
    ==8260==    by 0x4272865: sipe_buddy_menu (sipe.c:6791)
                    domain = entry->data;
    ==8260==    by 0x42479FE: sipe_purple_blist_node_menu (purple-plugin.c:228)
                    return sipe_buddy_menu((PurpleBuddy *) node);
    ==8260==    by 0x805F32C: ??? (in /usr/bin/finch)
    ==8260==    by 0x805F91A: ??? (in /usr/bin/finch)
    ==8260==    by 0xBFBF59: gnt_closure_marshal_BOOLEAN__VOID (in /usr/lib/libgnt.so.0.0.0)
    ==8260==    by 0xB25BE2: g_closure_invoke (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260==    by 0xB380EF: ??? (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260==    by 0xB40FCC: g_signal_emit_valist (in /lib/libgobject-2.0.so.0.2600.0)
    ==8260==
    
     
  • Jochen De Smet

    Jochen De Smet - 2010-12-03

    Any more comments anyone? I'd like to get this checked in and post the next couple patches for review

     
  • Anonymous - 2010-12-03

    There are some rejects now in POTFILES.in and miranda-transport.c, so please update the patch to be applicable on newest code in git. Also you should use Unix line endings (LF) instead of Windows (CRLF).

    patch --strip=1 < /home/haakon/blist.patch 
    patching file po/POTFILES.in
    Hunk #1 FAILED at 12.
    1 out of 1 hunk FAILED -- saving rejects to file po/POTFILES.in.rej
    patching file src/api/sipe-backend.h
    patching file src/api/sipe-core.h
    patching file src/core/Makefile.mingw
    patching file src/core/sipe.c
    Hunk #38 succeeded at 6914 (offset 8 lines).
    patching file src/miranda/miranda-transport.c
    Hunk #1 FAILED at 96.
    Hunk #2 succeeded at 143 (offset 5 lines).
    Hunk #3 succeeded at 155 (offset 5 lines).
    Hunk #4 succeeded at 201 (offset 7 lines).
    1 out of 4 hunks FAILED -- saving rejects to file src/miranda/miranda-transport.c.rej
    patching file src/purple/Makefile.am
    patching file src/purple/purple-buddy.c
    

    The functionality in Pidgin seems OK, I tried contact search, creating and deleting groups and buddies, renaming contacts, all worked as expected. Please fix the problems above and wait with commit until Stefan approves everything is correct.

     
  • Stefan Becker

    Stefan Becker - 2010-12-04

    I already corrected the i18n check failures yesterday.

     
  • Jochen De Smet

    Jochen De Smet - 2010-12-10

    So is that a yes or no on commit approval ?

     
  • Stefan Becker

    Stefan Becker - 2010-12-11

    Commit it if your are confident that it is OK. It can always be reverted afterwards if it turns out to be broken.

     

Log in to post a comment.