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.
_______________________________________________
OpenAFS-devel mailing list
[email protected]
https://lists.openafs.org/mailman/listinfo/openafs-devel
