GNOME Bugzilla – Bug 460518
Allow None where NULL is allowed in C API
Last modified: 2011-11-24 17:33:32 UTC
The function find_network_password_sync requires that all arguments are actual strings: import gnomekeyring as gk import gtk print gk.find_network_password_sync("user", None, "example.org", None, "ssh", None, 0) This program fails with the error: Traceback (most recent call last):
+ Trace 150647
None, "ssh", None, 0) TypeError: find_network_password_sync() argument 2 must be string, not None
This seems to contradict the C API which allows NULL to be passed here. Also I would recommend to default all arguments to None so that only the relevant arguments can be passed like this: gk.find_network_password_sync(user="user", server="example.org", protocol="ssh")
Someone needs to check against the documentation, wherever NULL is allowed then None should be allowed in Python. In the defs, the corresponding parameters need a (null-ok). In .override, ParseTuple calls need 'z' instead of 's' as template char.
Well, the documentation for gnome-keyring basically consists of this document: http://svn.gnome.org/viewcvs/gnome-keyring/trunk/doc/keyring-intro.txt. Unfortunately the API undocumented apart from that. In this document there is the following example: In most cases, applications must use standard attributes. There is a convenience function in gnome-keyring to aid in setting these attributes: gnome_keyring_set_network_password (NULL /* default keyring */, "gnomer" /* user */ NULL, /* domain */ "master.gnome.org", /* server */ NULL, /* object */ "ssh", /* protocol */ NULL, /* authtype */ 0, /* port, default */ "mypassword", /* password */ set_network_cb, NULL, NULL); (This is for the async version, but this shouldn't matter.)
I have to correct myself: libgnome-keyring is now documented! Here's part of the API documentation to gnome_keyring_find_network_password_sync (of course I quoted the wrong section in the last comment): /** * gnome_keyring_find_network_password_sync: * @user: The user name or %NULL. * @domain: The domain name %NULL. * @server: The server or %NULL. * @object: The remote object or %NULL. * @protocol: The network protorol or %NULL. * @authtype: The authentication type or %NULL. * @port: The network port or zero. * @results: A location to return a %GList of #GnomeKeyringNetworkPasswordData pointers. * * [...] */
Created attachment 92468 [details] [review] proposed patch The attached patch should fix the problem. According to the API documentation, both gnome_keyring_set_network_password_sync and gnome_keyring_find_network_password_sync allow most parameters to be NULL (with the notable exception of the password argument in _set_network_password).
Created attachment 92469 [details] [review] make arguments to find_network_password_sync optional Another patch that makes the arguments to find_network_password_sync optional, allowing to supply only the necessary arguments: find_network_password_sync(host="...", protocol="..."). Unfortunately this can't be done for set_network_password_sync, since this would mean reordering the arguments. (The password argument is non-optional and would need to move to the front. The only solution I see would to make the password argument optional as well and catch this in the overrides file.)
Sounds fine I guess. Just make sure you don't miss anything else you'll need later; API freeze is on July 30th.
Created attachment 92489 [details] [review] another suggested API change: make parameters to set_network_password_sync optional I've committed the first two patches now. But thinking about my last comment, I've cooked up another patch that implements the exception idea: All arguments to set_network_password_sync are marked as optional, but if no password is supplied, TypeError is raised by hand.
committed
Created attachment 199841 [details] [review] patch to allow None as password when unlocking a keyring The functions unlock[_sync]() can be called with None as their second parameter (password). This causes the keyring to prompt for the password to the keyring. In the function defs though, the annotation (null-ok) was missing. Please find my patch attached. Best dtk
Review of attachment 199841 [details] [review]: This is the same as my patch. Totally sane. I'm surprised it hasn't already been applied.