GNOME Bugzilla – Bug 376991
[code-cleanup] Refactor e-passwords.c
Last modified: 2007-11-02 02:59:39 UTC
I spent some time refactoring the internals of libedataserverui/e-passwords.c. There is no API change and no change in functionality. It does bump the GLib requirement to 2.12 (the latest stable release), and there's a small but completely optional ABI break. Here's what I did (and didn't) do: - Use GKeyFile instead of the deprecated "gnome-config" API for reading from and writing to the account passwords file (~/.gnome2_private/Evolution). - Use GAsyncQueue instead of EMsgPort for passing messages between threads. Actually, this patch eliminates the last use of the 'ln' field of EMsg. So I removed that field in the patch, which causes an ABI break in libedataserver. But removing the field is completely optional. - Use the new Base64 API in GLib 2.12 for encoding and decoding passwords in the account passwords file. All of the Base64 stuff at the end of e-passwords.c drops out. - Supply key and value destroy functions when creating the passwords hash table. Also use the new g_hash_table_remove_all() in GLib 2.12 where appropriate. - Rewrote the message dispatching function, and documented how it works. Also rewrote and documented the code that cleans up the message queue after closing the password dialog. - Use gtk_dialog_run() to obtain the password dialog's response, instead of just showing the dialog and connecting to its "response" signal. - Make e_passwords_shutdown() a stub. I just let the data structures live until program termination, and there's no need to do a final sync of the account passwords file. (e_passwords_init() still has to be called from the main thread to initialize the 'main_thread' variable.) - Add XXX comments where I had to take extra steps to maintain backwards compatibility. - For the most part I left the keyring logic alone, mainly because I'm not familiar with the keyring API which is, as far as I can tell, undocumented.
Created attachment 76841 [details] [review] Proposed patch
Hey Matthew, Awesome patch. Im reviewing it. On a glance the NONKeyring part looked fine, but Im reviewing more in it. But In they keyring part I can see some obvious compilation errors. I think in ep_remember_password you have a variable key, but you it wasnt declared. I guess it should be declared and decoded? Ill review further and get back. The solution Sounds very clean to me.
I guess that if you use msg->key it should be fine in place of key. The e-msgport.h change forces an abi break. Can you just deprecate it with a comment or so?
Created attachment 77339 [details] [review] Revised patch Oops, my bad for not catching the keyring errors. Here's a revised patch that fixes all compilation errors and warnings in the keyring logic. I also added a "deprecated" comment to the EMsgPort field instead of removing it, as you suggested. I should also mention the GLib requirement again, since there seemed to be some controversy over using GLib 2.10 features in Evolution 2.8. This patch uses GLib 2.12 features. But to my knowledge we still don't have a clear policy about what versions of library dependencies we require. Maybe poke Harish again?
Hey Mathew, it seems to be fine now. I still suggest to poke harish at the library dependency version. Otherwise this is pretty cool. Thanks Matthew. Ive Marked the patch state as accepted-commit_now, but just close on the dep version issue before commit it.
Thanks for the review. I opened a bug on the dependency version issue (bug #380534), so I'll just make this bug depend on it.
While using this patch with Keyring, I just got the following crash once.
Created attachment 77362 [details] Stack trace
Just another crash I had. Im not sure. But these all started happening after I started using this patch with Keyring. It is not continous, but over a prolonged period. (gdb) thread apply all bt
+ Trace 90365
Thread 1 (Thread -1233930576 (LWP 30506))
Hmm, in both backtraces it looks like ep_msg_dispatch() is getting a message with a bad dispatch function pointer, and then calling it. Not sure how that could happen. Are you seeing this at all with the NON-keyring logic? Darn it, I was trying so hard not to disrupt the keyring stuff. Marking the patch as "needs-work" until this gets sorted out.
I havent tried without keyring. It is too late today. May be I'll try that out tomorrow and let you know.
I tried compiling the patch against CVS HEAD with keyring logic enabled and testing it on a couple IMAP accounts of mine. Seems to be working fine for me so far. I'll keep testing it.
I have a expired GW account password for addressbook. It just keeps popping every 1 minute. On that in 15 minutes or so, it easily crashes for me. Ill also try the NON-KEYRING code today. For me the keyring logic changes looked OK. Could be generic also. Donno.
Created attachment 84279 [details] [review] Revised patch I had another go at this. In this new revision I've completely rewritten the threading logic to be easier to understand. The details are documented in the source code, but basically I'm now using a GThreadPool to set up a dedicated thread for dispatching messages. This eliminates the need to worry about whether the message originated from the main thread, and therefore calling e_password_init() is no longer necessary. I also improved how messages that arrive while the password dialog is visible are dealt with. Again, the details are documented in the source code. Note that this revision utilizes my EFlag proposal (bug #415891). In addition, I'm formally deprecating the following functions: e_passwords_init() e_passwords_shutdown() e_passwords_cancel() [1] [1] The only call to e_passwords_cancel() is from dead code in Evolution. The message passing logic is kept cleaner if we don't have to deal with asynchronous cancellations.
Created attachment 84477 [details] [review] Patch for Evolution This patch eliminates calls to the deprecated e-passwords functions, which are now just stubs that do nothing.
Created attachment 84623 [details] [review] Revised patch This revision fixes bug #418255. It required a small change to ep_msg_send().
Is EFlag in svn now?
EFlag is in Subversion now. There's still the question of whether we still want/need to support canceling password operations via e_password_cancel(). I had proposed deprecating this function since the only call point is from dead code in mail-session.c (see bug #419051), and my patch makes the function a "no-op" to keep the design simpler. But you indicated in [1] that there may still be use cases for a non-interactive mode in the mailer. If that's the case then I'll need go back and add password cancellation support to my patch. The patch in comment #16 has been in Fedora Rawhide since March, and seems to be working pretty well. [1] http://bugzilla.gnome.org/show_bug.cgi?id=419051#c2
Created attachment 88548 [details] [review] Revised patch for Evolution-Data-Server Revised patch applies cleanly to 1.11.2. I haven't changed any logic.
Matthew the patch seems fine. I will try the patch for a week. I think you should put the cancel stuff back. Also I would prefer to get this in for 2.11.3 and not later that.
Matthew, Im having a few problems while using this. 1. My passwords dialog come-up often even though I have remembered and my server is up and responding well. 2. I had one bad password dialog that came up and I couldn't kill that. Clicked 'X', OK, Cancel but nothing really worked. I had to force-shutdown as later on it hung. I dont think it is still in usable state. Im OK, if you want some one else to try this out too. Chen/Sankar ? For now, Im reverting your patch and using trunk.
Hmm, I'll take another look at the keyring part. The GKeyFile part has been working well in Fedora for some time now, so hopefully I'm getting close. Changing the patch status to 'still-needs-even-more-work' (or something thereabouts) for now.
Setting the patch status right.
I think the simpler parts of this patch are getting overshadowed by the threading changes. I probably shouldn't have tried to cram so much into one patch. So I'm breaking this into separate bugs, starting with bug #474000 (Use GLib's Base64 API).
Created attachment 97294 [details] [review] Revised patch for Evolution-Data-Server With bug #474000 now closed, here's a updated patch for Subversion trunk.
Continuing to break this into separate bugs, here's the next piece: Bug 487229 – Use GKeyFile instead of gnome-config
Third piece builds off the previous piece: Bug 488156 – Minimize use of WITH_GNOME_KEYRING in e-passwords.c
Most of the working pieces of the patch here have now been committed under separate bugs (see above). My first attempt at cleaning up the threading code is still problematic, so I'm just going to reject the remaining portions of the patch and close this bug as WONTFIX. I may take another shot at this some time down the road.