[Pvfs2-developers] request unify and cleanup

[Sam Lang]

On Feb 11, 2008, at 9:01 AM, Phil Carns wrote:

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

Done.
>
>
> 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).
>
There's also add-client-syscall and add-server-req, which I believe  
Walt added a few years back.  We definitely don't need both, so maybe  
merging and updating the two into one is the right thing.
-sam

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