Gnome keyring - lack of error on failing to unlock the keyring

I found some strange behaviour in simple_gnome_keyring_first_creds() and
the three similar functions. If we try to unlock the keyring and the
attempt fails, the code says, "give up and try the next provider" and
returns no error but does not set (or save) the credentials. Is that
right? I think it should return an error in that situation.

[[[
/* Get cached encrypted credentials from the simple provider's cache. */
static svn_error_t *
simple_gnome_keyring_first_creds(void **credentials,
                                 void **iter_baton,
                                 void *provider_baton,
                                 apr_hash_t *parameters,
                                 const char *realmstring,
                                 apr_pool_t *pool)
{
  svn_boolean_t non_interactive = apr_hash_get(parameters,
                                               SVN_AUTH_PARAM_NON_INTERACTIVE,
                                               APR_HASH_KEY_STRING) != NULL;
  const char *default_keyring = get_default_keyring_name(pool);
  if (! non_interactive)
    {
      svn_auth_gnome_keyring_unlock_prompt_func_t unlock_prompt_func =
        apr_hash_get(parameters,
                     SVN_AUTH_PARAM_GNOME_KEYRING_UNLOCK_PROMPT_FUNC,
                     APR_HASH_KEY_STRING);
      void *unlock_prompt_baton =
        apr_hash_get(parameters, SVN_AUTH_PARAM_GNOME_KEYRING_UNLOCK_PROMPT_BATON,
                     APR_HASH_KEY_STRING);

      char *keyring_password;

      if (unlock_prompt_func && check_keyring_is_locked(default_keyring))
        {
          SVN_ERR((*unlock_prompt_func)(&keyring_password,
                                        default_keyring,
                                        unlock_prompt_baton,
                                        pool));

          /* If keyring is locked give up and try the next provider. */
          if (! unlock_gnome_keyring(default_keyring, keyring_password, pool))
            return SVN_NO_ERROR;
        }
    }
  else
    {
      if (check_keyring_is_locked(default_keyring))
        {
          return svn_error_create(SVN_ERR_AUTHN_CREDS_UNAVAILABLE, NULL,
                                  _("GNOME Keyring is locked and "
                                    "we are non-interactive"));
        }
    }
  return svn_auth__simple_first_creds_helper
           (credentials,
            iter_baton, provider_baton,
            parameters, realmstring,
            password_get_gnome_keyring,
            SVN_AUTH__GNOME_KEYRING_PASSWORD_TYPE,
            pool);
}
]]]

It's the "return SVN_NO_ERROR;" in the middle of the code above. I think
it should return an SVN_ERR_AUTHN_FAILED error there.

I looked at the callers of "first_creds" functions, and they pretty much
all treat an error return the same as they treat a successful return
with NULL for the credentials. There are two issues: (1) it seems to me
like an error ought to be reported in this situation, and (2) if the
function returns at this point with no error, the caller expects that
'*credentials' has been set to some value, but it has not been set to
anything.

Agreed?

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2405559