On 1 Jul 2009, at 15:17, Felix Frank wrote:
I'm glad to announce that initial patches concerning rxosd are ready
for discussion. These are against 1.4.10 (yes, I know).
This is by no means an exhaustive review, which ultimately will need
to be done against patches that are going to be applied to the OpenAFS
tree - there's really not much point picking over 1.4.x versions of
this code in great detail.
I like the general approach, and it's presentation. To my mind,
further review could be aided by merging patchsets 2 and 3 together,
and merging 4 and 6 together, as the intermediate states are actually
a little confusing (I had lots of complaints, which you then fixed
with sets 3 and 6).
I think I'd like to see this integrated better with the existing cache
type indirection, with storeOps and fetchOps hanging off the existing
struct afs_cacheOps. In addition, as you are replacing the current
FetchProc and StoreProcs, they can simply be removed from that
structure, rather than being replaced with identical values (which are
then never used). As I note below, I'd also like to see the
initialisation of the storeVariables function being performed in a
cache-type specific initialisation function, which is also hung off
the storeOps and fetchOps structures.
Two small concerns I have are regarding stack depth and performance -
this all adds an additional call to the stack, and a number of
additional calls to stores and fetches. I don't know how tight were
are for stack usage on some platforms.
Here are some more fairly nit picking comments ...
Patch 1/6
---------
+#ifdef AFS_CACHE_BYPASS
+#include "afs_bypasscache.h"
+#endif
I suspect this isn't appropriate for 1.4
Patch 3/6
---------
+afs_int32
+rxfs_storeUfsPrepare(char *r, afs_uint32 size, afs_uint32 *tlen)
The rock (r) should be a void *, rather than a char *, as it's an
anonymous pointer. This goes for all of these functions,
and all of these patches.
+afs_int32
+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.
I'd prefer that the allocation of tbuffer and tiov in this function
occurred in another cache-type specific function, rather than in
storeInit...
Patch 5/6
---------
Is this set of changes complete? You seem to have removed more
procedures than you've deleted stat counts from this file ...
Patch 6/6
---------
+afs_int32
+rxfs_fetchInit(...)
Similar comments as before - I'd rather see the cache-type specific
initialisations occur in a stand alone function, and the rock should
be replaced by something of type struct rxfs_fetchVariables
+ char *rock = 0;
Just noticed this - and it goes throughout the file. Personally, I
think it's cleaner if pointers are initialised to NULL, rather than '0'.
+ code = rxfs_fetchInit(acall, avc, abase, &length, adc, fP,
+ (struct fetchOps **)&ops, (char**)&rock);
Why the cast on fetchOps? It shouldn't be necessary, and just makes it
easier for someone to mess things up in the future.
+ if ( !code ) do {
Getting picky now, but I think this would be much clearer with an
additional set of braces, and the do being on a line of its own.
Hope that all helps!
Cheers,
Simon.
_______________________________________________
OpenAFS-devel mailing list
[email protected]
https://lists.openafs.org/mailman/listinfo/openafs-devel
