userdb cache expiry

[Tim Dijkstra]

On Wed, 20 Sep 2006 13:13:33 -0400
Havoc Pennington <hp at redhat.com> wrote:

> Tim Dijkstra wrote:
> > FWIW, I commented out the part that gets the user from the cache
> > (AFAICS there is only one function that directly access the cache). Not
> > the part that puts it in, because that would create a memleak, which
> > would abort the test (I didn't feel like investigating it further). So
> > the test is even a bit unfair, in addition to looking up the user the
> > nocache case also tries to add the user all time. 
> > Despite all this there doesn't seem to be significant difference
> > between the to cases; 
> > 
> 
> Hmm, who knows then - maybe the issue is only on certain configurations, 
> or maybe it vanished due to other changes over time, who knows.
> 
> If we can cook up a clean patch to keep the same basic code structure 
> but maintain a cache size of 0, that will let us test the theory that 
> it's OK to drop the cache, but still keep the code there if it turns out 
> to be needed.

I can try that maybe, but I'am kind of new to the dbus source code, so I
have some questions: I'm wondering what the normal way of handeling
memory alloc/free is in dbus. The way it now works is that higher level
functions get a pointer to a DbusUserInfo in the cache. At shutdown the
cache is cleared out. If don't have a cache we're leaking memory unless
we change all the higher level functions to free those structs, but that
doesn't seem like a small change.

... looking at the code ...

The whole design of dbus-userdb.c, seems to rest on the assumption that
user data can never change. This obviously is not true, users can get
new homedirs, be added to groups, even removed altogether. And dbus
wouldn't know it.

The least intrusive change seems to be to use the cache as a memory
store. We do not, however, want to add a new DBusUserInfo after every
lookup call. I wonder if you think updating a user in the cache would be
OK? Some function promise a constant reference to DBusUserInfo, but that
wouldn't be really entirely true anymore,

... looking at it some more ...

Well it doesn't seem so bad afterall (but you knew that;). I assume that
functions named like _dbus_* are for internal use only right?
If that's the case the only functions that touch the cache are:

_dbus_user_database_lookup
_dbus_user_database_lookup_group
And the wrappers around those:
_dbus_user_database_get_{uid,username,gid,groupname,groups}

If we want the whole thing thread-safe, we should demand that those
functions are only called when the database lock is held and that they
shouldn't assume the data is valid after releasing the lock. Only for
_dbus_user_database_get_groups this doesn't seem the case right know (It
doesn't lock the database, nor do its callers, it doesn't leak a
reference to the database however).

I think it would be better to change _dbus_user_database_get_groups to
_dbus_groups_from_uid (just like _dbus_homedir_from_username) and do the 
database handling inside it. That way calling functions do not have know 
about the database at all.

The only place where a reference to the cache leaks out of
user-db{-util}.c is in _dbus_is_console_user, which calls 
_dbus_user_at_console(info->username, error). But that is all under the
lock.

Sorry, this mail turned out to be more of a brain-dump then I meant it
to be. I'll see if I can cook up a patch.

grts Tim