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

[Felix Frank]

Hi Simon,

thanks for the thorough inspection. Some of the points you brought up 
are indeed quite messy atm, but will eventually clear up once the 
rxosd-picture is more complete.

> 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).

Will do.

> 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 

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.

> 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 

That seems to be entirely possible.

> initialisation of the storeVariables function being performed in a 
> cache-type specific initialisation function, which is also hung off the 
> storeOps and fetchOps structures.

The current design intends protocol-specific initialization functions. 
We shall investigate whether protocol-specific steps can be dislodged 
from cachetype-specific ones, and how that would affect codee readability.

> 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.

I really can't see how we would multiple protocols in a manageable 
fashion. I could imagine evil preprocessor tricks, but that code is a 
jungle as is.
Speaking of which, is it possible to tell the compiler to inline given 
functions? This is of course not generally possible here, but would be 
feasible when not building with --enable-object-storage (yes, we are 
going to add a new build time option ;-)

>> +#ifdef AFS_CACHE_BYPASS
> I suspect this isn't appropriate for 1.4

Oops.

> The rock (r) should be a void *, rather than a char *, as it's an 
> anonymous pointer. This goes for all of these functions,

Consider it done.

>> +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.

> Is this set of changes complete? You seem to have removed more 
> procedures than you've deleted stat counts from this file ...

It's funny, actually - I totally messed that up when cherrypicking. This 
patch should have been dropped and been something else entirely.

> Just noticed this - and it goes throughout the file. Personally, I think 
> it's cleaner if pointers are initialised to NULL, rather than '0'.

Agreed.

>> +    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 

Right - this is me being overzealous for no good reason whatsoever.

>> +    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.

This whole function is going to look a whole lot different after another 
patch or two.

> Hope that all helps!

It does! Thanks.

Cheers
  - Felix