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:
[EMAIL PROTECTED] 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@beowulf-underground.org
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
_______________________________________________
Pvfs2-developers mailing list
Pvfs2-developers@beowulf-underground.org
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers