CEPH_RBD_API: options on image create

CEPH_RBD_API: options on image create

[Mykola Golub]

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


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

-- 
Mykola Golub

Re: CEPH_RBD_API: options on image create

[Jason Dillaman]

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.

Perhaps rbd_create4 and rbd_clone3 should move the order and features options to rbd_image_options_t as well?

-- 

Jason Dillaman 


----- Original Message -----
> From: "Mykola Golub" <mgolub@mirantis.com>
> To: ceph-devel@vger.kernel.org
> Cc: "Jason Dillaman" <dillaman@redhat.com>, "Josh Durgin" <jdurgin@redhat.com>
> Sent: Wednesday, September 30, 2015 2:50:45 AM
> Subject: CEPH_RBD_API: options on image create
> 
> 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
> 
> 
> 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);
> 
> --
> Mykola Golub
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: CEPH_RBD_API: options on image create

[Josh Durgin]

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.

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.

> 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

Re: CEPH_RBD_API: options on image create

[Mykola Golub]

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Mykola Golub

Re: CEPH_RBD_API: options on image create

[Mykola Golub]

On Wed, Oct 14, 2015 at 03:34:52PM -0400, 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?

Then rbd_image_options_set() will fail with EINVAL, because the option
type (size) is a part of interface.

I do this by analogy to setsockopt(2):

http://pubs.opengroup.org/onlinepubs/009695399/functions/setsockopt.html

Note option type documented for every option there, and it works
fairly well.

Following a common practice is an additional argument to this
approach to me.

> Therefore, I would vote for passing strings a la librados
> rados_conf_set.

I'd rather don't use this approach, as it adds unnecessary work on
both user (encode the option to string) and backend (decode from
string) sides.

> Perhaps rbd_create4 and rbd_clone3 should move the order and
> features options to rbd_image_options_t as well?

My initial thought was that a user would want to set features and
orders more frequently than other options, so keeping them as
additional arguments would be useful. But now thinking more about it,
I agree that they can be moved to options.

> -- 
> 
> Jason Dillaman 
> 
> 
> ----- Original Message -----
> > From: "Mykola Golub" <mgolub@mirantis.com>
> > To: ceph-devel@vger.kernel.org
> > Cc: "Jason Dillaman" <dillaman@redhat.com>, "Josh Durgin" <jdurgin@redhat.com>
> > Sent: Wednesday, September 30, 2015 2:50:45 AM
> > Subject: CEPH_RBD_API: options on image create
> > 
> > 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
> > 
> > 
> > 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);
> > 
> > --
> > Mykola Golub
> > --
> > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Mykola Golub

Re: CEPH_RBD_API: options on image create

[Jason Dillaman]

> > 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?
> 
> Then rbd_image_options_set() will fail with EINVAL, because the option
> type (size) is a part of interface.
> 
> I do this by analogy to setsockopt(2):
> 
> http://pubs.opengroup.org/onlinepubs/009695399/functions/setsockopt.html
> 
> Note option type documented for every option there, and it works
> fairly well.
> 
> Following a common practice is an additional argument to this
> approach to me.

Except for the following cases:

sizeof(char*) == sizeof(uint32_t) (32bit)
sizeof(char*) == sizeof(uint64_t) (64bit)

-- 

Jason Dillaman 

Re: CEPH_RBD_API: options on image create

[Mykola Golub]

On Thu, Oct 15, 2015 at 08:05:07AM -0400, Jason Dillaman wrote:
> > > 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?
> > 
> > Then rbd_image_options_set() will fail with EINVAL, because the option
> > type (size) is a part of interface.
> > 
> > I do this by analogy to setsockopt(2):
> > 
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/setsockopt.html
> > 
> > Note option type documented for every option there, and it works
> > fairly well.
> > 
> > Following a common practice is an additional argument to this
> > approach to me.
> 
> Except for the following cases:
> 
> sizeof(char*) == sizeof(uint32_t) (32bit)
> sizeof(char*) == sizeof(uint64_t) (64bit)

But we don't need them to match between different platforms, no? Is
linking 64bit code with 32bit possible (supported)?

Also, for this particular (char*) case, length would actually be the
length of the string, not the pointer length. From my example:

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

-- 
Mykola Golub

Re: CEPH_RBD_API: options on image create

[Jason Dillaman]

> 
> But we don't need them to match between different platforms, no? Is
> linking 64bit code with 32bit possible (supported)?
> 
> Also, for this particular (char*) case, length would actually be the
> length of the string, not the pointer length. From my example:
> 
> 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);
> 

My original example was a string of length 4 vs a 4-byte int, but you said you were thinking of sizeof(type) instead.  I think this style of interface is great if you need to pass any arbitrary data along, but will we ever expect to pass along anything besides a string or an (u)int(32/64)?

On the flip-side, what will the C++ interface look like?  An equivalent API would imply passing a boost::any.  While certainly future-proof, something about that doesn't sit right with me as an API.  I think I would lean more towards something like xyz_set(const std::string&), xyz_set(uint64_t), et al.

I've been witness to too many type-casting issues in the past (in fact just hit one last night within CephContext), which makes me lean more towards having the compiler be able to enforce type-correctness.

--

Jason

Re: CEPH_RBD_API: options on image create

[Mykola Golub]

On Thu, Oct 15, 2015 at 08:47:58AM -0400, Jason Dillaman wrote:
> > 
> > But we don't need them to match between different platforms, no? Is
> > linking 64bit code with 32bit possible (supported)?
> > 
> > Also, for this particular (char*) case, length would actually be the
> > length of the string, not the pointer length. From my example:
> > 
> > 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);
> > 
> 
> My original example was a string of length 4 vs a 4-byte int, but
> you said you were thinking of sizeof(type) instead.  I think this
> style of interface is great if you need to pass any arbitrary data
> along, but will we ever expect to pass along anything besides a
> string or an (u)int(32/64)?

I don't know, sure you have much more experience here, so if you
hardly expect other types in future, I would be rather then for
rbd_image_options_set_{uint64,str}() functions.

> On the flip-side, what will the C++ interface look like?  An
> equivalent API would imply passing a boost::any.  While certainly
> future-proof, something about that doesn't sit right with me as an
> API.  I think I would lean more towards something like xyz_set(const
> std::string&), xyz_set(uint64_t), et al.

For C++ I was also thinking about xyz_set(const std::string&),
xyz_set(uint64_t) variants, i.e:

int rbd::Image::options::set(int optname, const std::string& val);
int rbd::Image::options::set(int optname, uint64_t val);
...

-- 
Mykola Golub

Re: CEPH_RBD_API: options on image create

[Sage Weil]

On Thu, 15 Oct 2015, Mykola Golub wrote:
> On Thu, Oct 15, 2015 at 08:47:58AM -0400, Jason Dillaman wrote:
> > > 
> > > But we don't need them to match between different platforms, no? Is
> > > linking 64bit code with 32bit possible (supported)?
> > > 
> > > Also, for this particular (char*) case, length would actually be the
> > > length of the string, not the pointer length. From my example:
> > > 
> > > 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);
> > > 
> > 
> > My original example was a string of length 4 vs a 4-byte int, but
> > you said you were thinking of sizeof(type) instead.  I think this
> > style of interface is great if you need to pass any arbitrary data
> > along, but will we ever expect to pass along anything besides a
> > string or an (u)int(32/64)?
> 
> I don't know, sure you have much more experience here, so if you
> hardly expect other types in future, I would be rather then for
> rbd_image_options_set_{uint64,str}() functions.

Having a str and int variant seems like the best path to me.  Maybe int64 
though, so that signed values are possible?

sage


> > On the flip-side, what will the C++ interface look like?  An
> > equivalent API would imply passing a boost::any.  While certainly
> > future-proof, something about that doesn't sit right with me as an
> > API.  I think I would lean more towards something like xyz_set(const
> > std::string&), xyz_set(uint64_t), et al.
> 
> For C++ I was also thinking about xyz_set(const std::string&),
> xyz_set(uint64_t) variants, i.e:
> 
> int rbd::Image::options::set(int optname, const std::string& val);
> int rbd::Image::options::set(int optname, uint64_t val);
> ...
> 
> -- 
> Mykola Golub
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: CEPH_RBD_API: options on image create

[Josh Durgin]

On 10/15/2015 06:45 AM, Sage Weil wrote:
> On Thu, 15 Oct 2015, Mykola Golub wrote:
>> On Thu, Oct 15, 2015 at 08:47:58AM -0400, Jason Dillaman wrote:
>>>>
>>>> But we don't need them to match between different platforms, no? Is
>>>> linking 64bit code with 32bit possible (supported)?
>>>>
>>>> Also, for this particular (char*) case, length would actually be the
>>>> length of the string, not the pointer length. From my example:
>>>>
>>>> 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);
>>>>
>>>
>>> My original example was a string of length 4 vs a 4-byte int, but
>>> you said you were thinking of sizeof(type) instead.  I think this
>>> style of interface is great if you need to pass any arbitrary data
>>> along, but will we ever expect to pass along anything besides a
>>> string or an (u)int(32/64)?
>>
>> I don't know, sure you have much more experience here, so if you
>> hardly expect other types in future, I would be rather then for
>> rbd_image_options_set_{uint64,str}() functions.

This is my favorite option too.

> Having a str and int variant seems like the best path to me.  Maybe int64
> though, so that signed values are possible?

Don't think we need any signed values currently. It doesn't cause any
ABI problems to add signed ints or other types later, since it'll
already be overloaded in C++, and for C they need to be new functions
anyway.

>>> On the flip-side, what will the C++ interface look like?  An
>>> equivalent API would imply passing a boost::any.  While certainly
>>> future-proof, something about that doesn't sit right with me as an
>>> API.  I think I would lean more towards something like xyz_set(const
>>> std::string&), xyz_set(uint64_t), et al.
>>
>> For C++ I was also thinking about xyz_set(const std::string&),
>> xyz_set(uint64_t) variants, i.e:
>>
>> int rbd::Image::options::set(int optname, const std::string& val);
>> int rbd::Image::options::set(int optname, uint64_t val);

Sounds good to me.

Josh

Re: CEPH_RBD_API: options on image create

[Mykola Golub]

Thank you all for your comments! I will come back with PR and pull
request.

-- 
Mykola Golub

On Thu, Oct 15, 2015 at 11:29:56AM -0700, Josh Durgin wrote:
> On 10/15/2015 06:45 AM, Sage Weil wrote:
> >On Thu, 15 Oct 2015, Mykola Golub wrote:
> >>On Thu, Oct 15, 2015 at 08:47:58AM -0400, Jason Dillaman wrote:
> >>>>
> >>>>But we don't need them to match between different platforms, no? Is
> >>>>linking 64bit code with 32bit possible (supported)?
> >>>>
> >>>>Also, for this particular (char*) case, length would actually be the
> >>>>length of the string, not the pointer length. From my example:
> >>>>
> >>>>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);
> >>>>
> >>>
> >>>My original example was a string of length 4 vs a 4-byte int, but
> >>>you said you were thinking of sizeof(type) instead.  I think this
> >>>style of interface is great if you need to pass any arbitrary data
> >>>along, but will we ever expect to pass along anything besides a
> >>>string or an (u)int(32/64)?
> >>
> >>I don't know, sure you have much more experience here, so if you
> >>hardly expect other types in future, I would be rather then for
> >>rbd_image_options_set_{uint64,str}() functions.
> 
> This is my favorite option too.
> 
> >Having a str and int variant seems like the best path to me.  Maybe int64
> >though, so that signed values are possible?
> 
> Don't think we need any signed values currently. It doesn't cause any
> ABI problems to add signed ints or other types later, since it'll
> already be overloaded in C++, and for C they need to be new functions
> anyway.
> 
> >>>On the flip-side, what will the C++ interface look like?  An
> >>>equivalent API would imply passing a boost::any.  While certainly
> >>>future-proof, something about that doesn't sit right with me as an
> >>>API.  I think I would lean more towards something like xyz_set(const
> >>>std::string&), xyz_set(uint64_t), et al.
> >>
> >>For C++ I was also thinking about xyz_set(const std::string&),
> >>xyz_set(uint64_t) variants, i.e:
> >>
> >>int rbd::Image::options::set(int optname, const std::string& val);
> >>int rbd::Image::options::set(int optname, uint64_t val);
> 
> Sounds good to me.
> 
> Josh
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CEPH_RBD_API: options on image create

[Mykola Golub]

On Fri, Oct 16, 2015 at 08:32:12AM +0300, Mykola Golub wrote:
> Thank you all for your comments! I will come back with PR and pull
> request.

Here it is: https://github.com/ceph/ceph/pull/6369

-- 
Mykola Golub