On Wed, Oct 14, 2015 at 08:12:37PM -0700, Josh Durgin wrote:
> On 10/14/2015 12:34 PM, Jason Dillaman wrote:
> >In general, I like the approach.
> >
> >I am concerned about passing a void* + length to specify the option value since you really can't protect against the user providing data in the incorrect format. For example, if the backend treated RBD_OPTION_STRIPE_UNIT as a 4byte int, what happens if someone passes a 2- or 8-byte int or a 4-byte char* string? Therefore, I would vote for passing strings a la librados rados_conf_set.
>
> It seems like that'd be a bit clunky from C, since you'd need to create
> and fill in buffers for each option.
>
> For safety we could have typed rbd_image_options_{get,set} for char*
> and uint64_t - it doesn't seem like we need any other types right now,
> since uint64_t is a superset of what we use int for.
I like this approach much more than using string, due to its
simplicity. I prefer it if my reasoning about my initial version I
sent in the response to Jason doesn't change your mind, guys.
>
> Another alternative is a single get/set that takes a tagged union, e.g.
>
> struct rbd_image_option {
> int option;
> int type;
> union {
> uint64_t ui
> int i
> char* s // NUL-terminated
> };
> }
>
> where type is an enum of RBD_OPTION_TYPE_{UINT64,INT,STRING} or
> similar.
Could do this way, it is ok to me too, though I like it a little less
than my version as I expect the code to encode/decode this would be a
little more complicated.
So, summarizing, I am listing the discussed approaches in the order I
like them from more to less.
1) initial variant (void*, lenght)
2) rbd_image_options_{get,set}_{uint64,str}
3) struct rbd_image_option
4) use string for options
Please tell me what you like more, I agree to do any way.
Thanks.
>
> >Perhaps rbd_create4 and rbd_clone3 should move the order and features options to rbd_image_options_t as well?
>
> Sounds good - no reason to keep mandatory parameters for options with
> defaults.
>
> >>>Hi,
> >>>
> >>>It was mentioned several times eralier that it would be nice to pass
> >>>options as key/value configuration pairs on image create instead of
> >>>expanding rbd_create/rbd_clone/rbd_copy for every possible
> >>>configuration override.
> >>>
> >>>What do you think about this API?
> >>>
> >>>Introduce rbd_image_options_t and functions to manipulate it:
> >>>
> >>>int rbd_image_options_create(rbd_image_options_t* opts);
> >>>
> >>>void rbd_image_options_destroy(rbd_image_options_t opts);
> >>>
> >>>int rbd_image_options_set(rbd_image_options_t opts, int optname,
> >>> const void* optval, size_t optlen);
> >>>
> >>>int rbd_image_options_get(rbd_image_options_t opts, int optname,
> >>> void* optval, size_t* optlen);
> >>>
> >>>void rbd_image_options_iterate(rbd_image_options_t opts,
> >>> void (*func)(int* optname, void* optval,
> >>> size_t* optlen));
> >>>
> >>>Functions that return a value return 0 on success, and -ERROR on
> >>>failure.
> >>>
> >>>optname is a constant like RBD_OPTION_STRIPE_UNIT,
> >>>RBD_OPTION_STRIPE_COUNT...
> >>>
> >>>Pass options as additional argument to rbd_create, rbd_clone (and may
> >>>be rbd_copy) functions:
> >>>
> >>>int rbd_create4(rados_ioctx_t io, const char *name, uint64_t size,
> >>> uint64_t features, int *order, rbd_image_options_t opts);
> >>>
> >>>int rbd_clone3(rados_ioctx_t p_ioctx, const char *p_name,
> >>> const char *p_snapname, rados_ioctx_t c_ioctx,
> >>> const char *c_name, uint64_t features, int *c_order,
> >>> rbd_image_options_t opts);
> >>>
> >>>int rbd_copy3(rbd_image_t src, rbd_image_t dest, rbd_image_options_t opts);
> >>>// possibly
>
> I'm ambivalent about a copy3. If you'd like to implement it, it should
> use the form that creates the destination image:
>
> int rbd_copy3(rbd_image_t src, rados_ioctx_t dest_io_ctx,
> const char *destname);
>
> >>>
> >>>
> >>>Example:
> >>>
> >>>rbd_image_options_t opts;
> >>>int r;
> >>>r = rbd_image_options_create(&opts);
> >>>assert(r == 0);
> >>>uint64_t stripe_unit = 65536;
> >>>r = rbd_image_options_set(opts, RBD_OPTION_STRIPE_UNIT,
> >>> &stripe_unit, size_of(stripe_unit));
> >>>assert(r == 0);
> >>>uint64_t stripe_count = 16;
> >>>r = rbd_image_options_set(opts, RBD_OPTION_STRIPE_COUNT,
> >>> &stripe_count, size_of(stripe_count));
> >>>assert(r == 0);
> >>>const char* journal_object_pool = "journal";
> >>>r = rbd_image_options_set(opts, RBD_OPTION_JOURNAL_OBJECT_POOL,
> >>> journal_object_pool, strlen(journal_object_pool) +
> >>> 1);
> >>>assert(r == 0);
> >>>r = rbd_create4(io, name, size, features, int *order, rbd_image_options_t
> >>>opts);
> >>>
> >>>cleanup:
> >>>rbd_image_options_destroy(opts);
>
> I like the API in general. The ability to reuse the same options or
> make small changes to them is nice.
>
> Josh
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Mykola Golub
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
