Re: initial gencache implementation
Rafal Szczesniak wrote: > > On Fri, Sep 06, 2002 at 10:17:19AM +1000, Tim Potter wrote: > > On Thu, Sep 05, 2002 at 12:15:36PM +0200, Rafal Szczesniak wrote: > > > > > This is first implementation of caching mechanism. It includes > > > both lib/gencache.c code and utils/net_cache.c as command-line > > > control/testing tool. > > > > > > comments are welcome > > > > Rafal, that looks pretty good. Since you ask, I do have a few comments. > > (-: > > I'm glad to hear that :) > > > You assume that any cached data will be in null terminated string format > > which is not always the case. > > I assume that on gencache base we can implement any higher-level > caching function like namecache. Then, it's up to such implementation > how to 'pack' the structures into string form. Null terminated string > format is much easier to watch with 'net cache list' command. Thus > we have comfortable and easy mean to watch what's in the cache file. > > > This is just my personal opinion but I would prefer for gencache_set to > > crash > > To avoid mistake, I should ask what exactly do you mean by 'crash' ? > > > if you pass it a NULL pointer for the key or value parameter. > > Returning false in this case only hides the error until further along > > in the execution path by which time it will be more difficult to track > > down the original error. > > Good point. Just explain me this 'crash' thing. SMB_ASSERT or smb_panic(). Causes the program to exit, and calls the panic action. Good for debugging - and people complain fast if it's broken in the feild. > > Some other minor things: > > > > - memleak of cache_fname in gencache_init > > - memleak of keybuf.dptr in gencache_set > > Thank you. The latter was already fixed - I just forgot to send fixed > version :) > > > I don't think you need to strdup the key before passing it to tdb_fetch > > in gencache_set. You can just use the passed in parameter. > > I thought about that but unfortunatelly tdb_store doesn't have const > args, so compiler complains about passing pointers to non-const args. > I think tdb_store should have const-ed args (since it copies them anyway), > but it's quite other topic. I would like to see a patch for this at some stage - it frustrates me too... Andrew Bartlett -- Andrew Bartlett [EMAIL PROTECTED] Manager, Authentication Subsystems, Samba Team [EMAIL PROTECTED] Student Network Administrator, Hawker College [EMAIL PROTECTED] http://samba.org http://build.samba.org http://hawkerc.net
Re: initial gencache implementation
On Fri, Sep 06, 2002 at 10:17:19AM +1000, Tim Potter wrote: > On Thu, Sep 05, 2002 at 12:15:36PM +0200, Rafal Szczesniak wrote: > > > This is first implementation of caching mechanism. It includes > > both lib/gencache.c code and utils/net_cache.c as command-line > > control/testing tool. > > > > comments are welcome > > Rafal, that looks pretty good. Since you ask, I do have a few comments. > (-: I'm glad to hear that :) > You assume that any cached data will be in null terminated string format > which is not always the case. I assume that on gencache base we can implement any higher-level caching function like namecache. Then, it's up to such implementation how to 'pack' the structures into string form. Null terminated string format is much easier to watch with 'net cache list' command. Thus we have comfortable and easy mean to watch what's in the cache file. > This is just my personal opinion but I would prefer for gencache_set to > crash To avoid mistake, I should ask what exactly do you mean by 'crash' ? > if you pass it a NULL pointer for the key or value parameter. > Returning false in this case only hides the error until further along > in the execution path by which time it will be more difficult to track > down the original error. Good point. Just explain me this 'crash' thing. > Some other minor things: > > - memleak of cache_fname in gencache_init > - memleak of keybuf.dptr in gencache_set Thank you. The latter was already fixed - I just forgot to send fixed version :) > I don't think you need to strdup the key before passing it to tdb_fetch > in gencache_set. You can just use the passed in parameter. I thought about that but unfortunatelly tdb_store doesn't have const args, so compiler complains about passing pointers to non-const args. I think tdb_store should have const-ed args (since it copies them anyway), but it's quite other topic. -- cheers, ++ |Rafal 'Mimir' Szczesniak <[EMAIL PROTECTED]> | |*BSD, GNU/Linux and Samba / |__/
Re: initial gencache implementation
On Fri, Sep 06, 2002 at 02:10:23AM +, [EMAIL PROTECTED] wrote: > > You assume that any cached data will be in null terminated string format > > which is not always the case. > > I understand this is a design property - it's up to the caller to mess with > structs etc. This keeps the cache simple, and allows for human inspection > with 'net cache' etc. OK - fair enough, I missed that bit. Tim.
Re: initial gencache implementation
On Fri, Sep 06, 2002 at 10:17:19AM +1000, Tim Potter wrote: > On Thu, Sep 05, 2002 at 12:15:36PM +0200, Rafal Szczesniak wrote: > > > This is first implementation of caching mechanism. It includes > > both lib/gencache.c code and utils/net_cache.c as command-line > > control/testing tool. > > > > comments are welcome > > Rafal, that looks pretty good. Since you ask, I do have a few comments. > (-: > > You assume that any cached data will be in null terminated string format > which is not always the case. I understand this is a design property - it's up to the caller to mess with structs etc. This keeps the cache simple, and allows for human inspection with 'net cache' etc. > This is just my personal opinion but I would prefer for gencache_set to > crash if you pass it a NULL pointer for the key or value parameter. > Returning false in this case only hides the error until further along > in the execution path by which time it will be more difficult to track > down the original error. Agreed. Andrew Bartlett
Re: initial gencache implementation
On Thu, Sep 05, 2002 at 12:15:36PM +0200, Rafal Szczesniak wrote: > This is first implementation of caching mechanism. It includes > both lib/gencache.c code and utils/net_cache.c as command-line > control/testing tool. > > comments are welcome Rafal, that looks pretty good. Since you ask, I do have a few comments. (-: You assume that any cached data will be in null terminated string format which is not always the case. This is just my personal opinion but I would prefer for gencache_set to crash if you pass it a NULL pointer for the key or value parameter. Returning false in this case only hides the error until further along in the execution path by which time it will be more difficult to track down the original error. Some other minor things: - memleak of cache_fname in gencache_init - memleak of keybuf.dptr in gencache_set I don't think you need to strdup the key before passing it to tdb_fetch in gencache_set. You can just use the passed in parameter. Tim.
