GNOME Bugzilla – Bug 509985
Refactor keyring logic with better error handling
Last modified: 2013-09-14 16:50:00 UTC
As I was trying to figure out why Evolution 2.21.5 stopped taking my passwords, I noticed it wasn't being very helpful about failed keyring operations. I'd see cryptic messages like these on the terminal: Couldn't Get password 2 I still think the password API should be using GErrors, but that will require an API break. So in the meantime the least it can do is print more meaningful warnings to the terminal. So I refactored the keyring logic and beefed up the error handling a bit. Now you'll see warnings like: e-data-server-ui-WARNING **: Unable to find password(s) in keyring e-data-server-ui-WARNING **: Keyring reports: The gnome-keyring-daemon application is not running
Created attachment 103018 [details] [review] Proposed patch
Milan, can you look at this?
a) ep_keyring_validate: you have there defined these variables twice: + const gchar *user_value = NULL; + const gchar *server_value = NULL; b) I think you should check if both user_value and server_value in this function are non-null after the 'for' cycle. c) I'm not sure if it's good idea to make two g_warnings in same time (I do not like the idea to have more empty lines on the console than is necessary.) Look into ep_keyring_delete_passwords. d) consider infinite loop in ep_keyring_delete_passwords when ep_keyring_validate returns FALSE e) I do not see any difference between ep_clear_passwords_keyring and ep_forget_passwords_keyring. Do we need them both? Just a note, here "!strcmp (uri->protocol, "ldap") && !uri->user" I would prefer opposite order, but that's nothing serious, just a habit. Do not you want to have some fix_uri function to not have same thing couple of times there?
Created attachment 103177 [details] [review] Revised patch (In reply to comment #3) > a) ep_keyring_validate: you have there defined these variables twice: > + const gchar *user_value = NULL; > + const gchar *server_value = NULL; Fixed. > b) I think you should check if both user_value and server_value in this > function are non-null after the 'for' cycle. I check them one at a time along with their user/server counterparts. I'm not sure how checking them both at once would help. Maybe I'm missing something. > c) I'm not sure if it's good idea to make two g_warnings in same time (I do > not like the idea to have more empty lines on the console than is necessary.) > Look into ep_keyring_delete_passwords. That occurred to me too. Combined the messages into one long warning. > d) consider infinite loop in ep_keyring_delete_passwords when > ep_keyring_validate returns FALSE Good catch. Fixed. > e) I do not see any difference between ep_clear_passwords_keyring and > ep_forget_passwords_keyring. Do we need them both? There's differences in the keyfile variants of these operations, and I think I'd rather keep the keyring/keyfile/dispatcher pattern of functions intact for each operation. > Just a note, here "!strcmp (uri->protocol, "ldap") && !uri->user" I would > prefer opposite order, but that's nothing serious, just a habit. Do not you > want to have some fix_uri function to not have same thing couple of times > there? Good idea. Fixed.
ad b) You've right, I overlooked it. ad e) I'm fine with it. If it's truth that the uri->protocol cannot be NULL in any case, then I'm fine with this patch, you can commit to trunk.
(In reply to comment #5) > If it's truth that the uri->protocol cannot be NULL in any case, then I'm fine > with this patch, you can commit to trunk. That's a good point, actually. I beefed up the error checking in and around ep_keyring_uri_new() before committing since the URI should not be trusted. Committed to trunk (revision #8403).