Re: [PATCH REPOST 0/6] rbd: consolidate osd request setup

[Josh Durgin <josh.durgin@inktank.com>]

On 01/04/2013 07:03 AM, Alex Elder wrote:
> This series consolidates and encapsulates the setup of all
> osd requests into a single function which takes variable
> arguments appropriate for the type of request.  The result
> groups together common code idioms and I think makes the
> spots that build these messages a little easier to read.
>
> 					-Alex
>
> [PATCH REPOST 1/6] rbd: don't assign extent info in rbd_do_request()
> [PATCH REPOST 2/6] rbd: don't assign extent info in rbd_req_sync_op()
> [PATCH REPOST 3/6] rbd: initialize off and len in rbd_create_rw_op()
> [PATCH REPOST 4/6] rbd: define generalized osd request op routines
> [PATCH REPOST 5/6] rbd: move call osd op setup into rbd_osd_req_op_create()
> [PATCH REPOST 6/6] rbd: move remaining osd op setup into
> rbd_osd_req_op_create()

I'm not sure about the varargs approach. It makes it easy to
accidentally use the wrong parameters. What do you think about
replacing calls to rbd_osd_req_create_op() with helpers for the various
kinds of requests that just call rbd_osd_req_create_op() themselves, so
that the arguments can be checked at compile time? This will probably
be more of an issue with multi-op osd requests in the future.

Eventually I think all this osd-request-related stuff should go into
libceph, but that's a cleanup for another day.

In any case, the new structure looks good to me.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>