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

[Simon Wilkinson]

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.