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