2.11? but that's ancient unstable history?!

erm.. I meant 2.21.90; 2.11 was selected by default...

Matt/Milan, can you guys look at this?

Srag, I guess it's same as bug #513389, the line of question is a call of g_warning and the environment is configured to stop on such warning messages from glib (G_DEBUG environment variable probably). So it's a "critical" warning crash.

But critical warnings should be fixed... ;-)

Either of two things have to be changed:

1. If a configuration file not having a 'Passwords-Calendar' group is ok, then please remove the g_warning

2. If it's not ok, then we need to figure out why and fix it; does the configuration file need to be upgraded from one Evo. version to another, or...?

I was actually trying to debug bug #513389 (following the steps in http://bugzilla.gnome.org/show_bug.cgi?id=513389#c5), when this warning was triggered, and it prevented me from debugging the other error.  This is why warnings must be fixed or removed.

I agree, some critical warnings are nonsense, but this isn't true in general.

In this case, we only show the error returned from keyring, and we do not know which warnings are critical and which not (in terms of error text; we can check for error code, but I doubt it as a solution to print the error once as g_warning and once as fprintf(stderr,...)).

Probably most errors reported from this source file are harmless and can be printed to stderr directly, but please wait what Matt thinks.

It's acceptable for ep_get_password() to not find the requested password, so I think we should issue a g_message() for error code G_KEY_FILE_ERROR_KEY_NOT_FOUND and a g_warning() for anything else.  It's still useful to leave a breadcrumb on the terminal when the requested password is not found, and informational messages will not be trapped when running with 'G_DEBUG=fatal-warnings'.

Created attachment 104395 [details] [review]
Proposed patch

If my suggestion is acceptable...

If you want to change it even for file part, not only key ring, then it would be nice. Otherwise looks good and you can commit with Changelog entry.
Gustavo, can you check if it works as you expect, please? (I do not use keyring, so I cannot check functionality.)

Comment on attachment 104395 [details] [review]
Proposed patch

>Index: libedataserverui/e-passwords.c
>===================================================================
>--- libedataserverui/e-passwords.c	(revision 8453)
>+++ libedataserverui/e-passwords.c	(working copy)
>@@ -740,6 +740,14 @@ ep_get_password_keyfile (EPassMsg *msg)
> 	if (password != NULL) {
> 		msg->password = ep_password_decode (password);
> 		g_free (password);
>+
>+	/* Not finding the requested key is acceptable, but we still
>+	 * want to leave an informational message on the terminal. */
>+	} else if (g_error_matches (error, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_KEY_NOT_FOUND)) {
>+		g_message ("%s", error->message);
>+		g_error_free (error);
                ^^^^^^^^^^^^^^^^^^^^^
  What you want here is: g_clear_error (&error);

>+
>+	/* Issue a warning if anything else goes wrong. */

Here 'error' may point to already freed memory, due to g_error_free/g_clear_error mixup.

> 	} else if (error != NULL) {
> 		g_warning ("%s", error->message);
> 		g_error_free (error);


Anyway, generally the patch does not seem to help, assuming I don't have to rebuild evolution itself, only evolution-data-server...  I still see a g_warning, and not a g_message.  And I am not using gnome-keyring; stupid ubuntu decision :(

No, the two g_error_free() statements are in different code paths and cannot both occur.  Also, the patch /is/ for the keyfile.  The keyring code doesn't emit these warnings.

Setting the patch status to back to 'reviewed', though I think I've addressed all the concerns raised here.

Created attachment 104478 [details] [review]
Revised patch

I think I see why Gustavo didn't have any luck with the old patch.  "Key not found" and "group not found" are separate error codes, and I was only checking for the first.  Here's a revised patch that checks for both.

Matt, I think the same may apply for forget_password and forget_passwords.

Since, at few places, we just forget the password, if the connection fails or so. We do that, since we want to clear the password. If it isn't remembered, it may cause you few more warnings like this, though it clears the in-memory-cache.

(In reply to comment #11)
> No, the two g_error_free() statements are in different code paths and cannot
> both occur.

Right; sorry, I missed the else.

(In reply to comment #12)
> Created an attachment (id=104478) [edit]
> Revised patch
> 
> I think I see why Gustavo didn't have any luck with the old patch.  "Key not
> found" and "group not found" are separate error codes, and I was only checking
> for the first.  Here's a revised patch that checks for both.

Patch works.

Created attachment 105937 [details] [review]
Revised patch

This revision implements Srini's suggestions in comment #13.

Commit it Matt.

Committed to trunk (revision 8535).