[Pvfs2-developers] request unify and cleanup

[Phil Carns]

I like this cleanup too.

One minor comment/suggestion.  The elements in the big 
PINT_server_req_table[] array have to be in order because they are 
indexed like this:

	PINT_server_req_table[req->op].params...

However, when the table is defined, that op value is also included in 
the table itself, like this:

	{PVFS_SERV_CREATE, &pvfs2_create_params}

The latter kind of gives the illusion that the order isn't important 
because it implies that maybe something would search for for a match on 
that op_type == PVFS_SERV_CREATE field.

I don't actually think it should search (indexing directly off of the 
req->op value as your patch already does is much cleaner and faster), 
but maybe if that op_type field is going to be there then we could at 
least use it for assert tests like this whenever it is accessed?

	assert(req->op == PINT_server_req_table[req->op].op_type)

Otherwise it might be good to just cut it from the table so it doesn't 
give the wrong impression.

Oh, and I guess actually one other comment too.  There is a stale 
document running around in doc/design/new_operation.txt.  I think it was 
probably already mildly out of date to begin with, but we should 
probably fix it after this patch (or else just delete it).

-Phil


Sam Lang wrote:
> 
> On Feb 9, 2008, at 4:29 PM, Pete Wyckoff wrote:
> 
>> slang at mcs.anl.gov wrote on Sat, 09 Feb 2008 15:59 -0600:
>>> I would do that if I could think of a way.  Those functions have to 
>>> access
>>> the request specific field in the request structure. delete for example
>>> needs to get at:
>>>
>>> req->u.delete.handle
>>>
>>> I couldn't come up with a generic function for doing that.  I was 
>>> tempted
>>> to move the fsid+handle to the common parts of the request, so that 
>>> every
>>> request had those fields (for some they would just be NULL), but that 
>>> would
>>> have been a much larger change, and a protocol change to boot.
>>>
>>> If I were able to assume that the fsid and handle were always the 
>>> first two
>>> fields in the request (for the requests that operated on a handle), I 
>>> could
>>> just operate on the req->u union (a poor man's polymorphism), but I 
>>> don't
>>> know if that's an assumption/requirement we want to make (and would be
>>> another protocol change for the requests that don't have those as the 
>>> first
>>> two).
>>>
>>> If we wanted to go down this path further, I would probably want to 
>>> work up
>>> some sort of bindings generator that would do a lot of the manual 
>>> labor for
>>> us.
>>
>> Oh, I see.  Missed that.  Yeah, the unions are a real pain.  Not
>> worth trying to fix that here too, I agree.  Funny, there already is
>> a target_handle in the server structure which may store exactly the
>> individual u.delete.handle you are working hard to extract.  Still,
>> not worth the change if there is any risk of bugs here.
> 
> The server struct (defined in pvfs2-server.h) has the 
> target_handle/target_fs_id fields, but its the server request struct 
> (defined in pvfs2-req-proto.h) that we need to extract the handle and 
> fsid out of.
> 
>>
>>
>> I see now that PINT_server_req_get_object_ref() returns NULL handle
>> and fs_id when no get_object_ref method is defined.  Which means at
>> least you don't need special methods for mgmt_event_mon, get_config,
>> noop, etc.  If you want to excise those at least.
> 
> Yep, can do.
> -sam
>>
>>
>>         -- Pete
>>
> 
> _______________________________________________
> Pvfs2-developers mailing list
> Pvfs2-developers at beowulf-underground.org
> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers