[OpenAFS-devel] Patch to prepare the client for addition protocols

[Simon Wilkinson]

On 2 Jul 2009, at 07:30, Felix Frank wrote:
> The problem with that being that it does not really simplify anything.
> What's worse, the fetchOps/storeOps would need being determined  
> before calling afs_CacheFetchProc/CacheStoreProc. As will be seen in  
> the upcoming patches, that would not be desirable, I think.

I have a few points here,

*) The cache manager already has a cache type indirection mechanism -  
afs_cacheOps. Things which vary only by cache type should use this  
indirection layer, rather than adding an additional one.
*) Having both a cache type layer, and a protocol/cache type layer  
seems very untidy.
*) When you have an indirection layer, you should use it. Having the  
ability to have cache or protocol specific initialisation functions,  
and then still doing
if (cacheDiskType == AFS_FCACHE_TYPE_UFS ) {
   thing
} else {
   other thing
}
is just untidy programming.

>>> +rxfs_storeInit(struct vcache *avc, struct storeOps **ops, char  
>>> **rock)
>> As far as I can see callers to this function expect that the third  
>> parameter will always be a struct rxfs_storeVariables, so why not  
>> just return that, rather than an anoymous pointer? My earlier  
>> comment about rocks applies here, too.
>
> Of course, there are alternatives (such as rxosd_storeInit) coming  
> up that share the prototype. ATM, I don't think they are used  
> polymorphic in the way the storeOps/fetchOps are, so this wouldn't  
> be a problem here.
> Still, it seems more sound to me to keep the prototypes identical.

Using anonymous variables when you don't need to is completely  
unsound. It destroys any chance of the compiler warning you about type  
errors. Would you argue that every single argument function should be  
void *function(void *arg) ? There's no reason for these functions to  
share prototypes, so please let the compiler do its job and protect  
future developers against type errors.

Cheers,

Simon.