[02/10] ceph: add start/finish encoding helpers

Commit Message

Mike Christie April 28, 2015, 10:05 p.m. UTC

From: Mike Christie <[e-mail]>

This patch adds helpers to encode/decode the starting blocks
locking code. They are the equivalent of ENCODE_START and
DECODE_START_LEGACY_COMPAT_LEN in the userspace ceph code.

Signed-off-by: Mike Christie <[e-mail]>
---
 include/linux/ceph/decode.h | 55 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Alex Elder April 29, 2015, 9:54 p.m. UTC | #1

On 04/28/2015 05:05 PM, [e-mail] wrote:
> From: Mike Christie <[e-mail]>
>
> This patch adds helpers to encode/decode the starting blocks
> locking code. They are the equivalent of ENCODE_START and
> DECODE_START_LEGACY_COMPAT_LEN in the userspace ceph code.

Your subject line says "start/finish encoding" but really it's just
encode/decode.

I have a few suggestions and questions, below.  Also, I'm about to
get on a plane, so I won't be providing any more reviews for a while.

					-Alex

> Signed-off-by: Mike Christie <[e-mail]>
> ---
>   include/linux/ceph/decode.h | 55 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index a6ef9cc..96ec43d 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -217,6 +217,61 @@ static inline void ceph_encode_string(void **p, void *end,
>   	*p += len;
>   }
>
> +/*
> + * version and length starting block encoders/decoders
> + */
> +
> +/* current code version (u8) + compat code version (u8) + len of struct (u32) */
> +#define CEPH_ENCODING_START_BLK_LEN 6

I don't see this used. (I'm sure it will be soon though.)

Why not just explain what it is in code?

#define CEPH_ENCODING_START_BLK_LEN \
		(sizeof(u8) + sizeof(u8) + sizeof(u32))

> +/**
> + * ceph_start_encoding - start encoding block
> + * @p: buffer to encode data in
> + * @curr_ver: current (code) version of the encoding
> + * @compat_ver: oldest code version that can decode it
> + * @len: length of data that will be encoded in buffer
> + */
> +static inline void ceph_start_encoding(void **p, u8 curr_ver, u8 compat_ver,
> +				       u32 len)
> +{
> +	ceph_encode_8(p, curr_ver);
> +	ceph_encode_8(p, compat_ver);
> +	ceph_encode_32(p, len);
> +}
> +
> +/**
> + * ceph_start_decoding_compat - decode block with legacy support for older schemes
> + * @p: buffer to decode
> + * @end: end of decode buffer
> + * @curr_ver: current version of the encoding that the code supports/encode
> + * @compat_ver: oldest version that includes a __u8 compat version field
> + * @len_ver: oldest version that includes a __u32 length wrapper
> + * @len: buffer to return len of data in buffer
> + */
> +static inline int ceph_start_decoding_compat(void **p, void *end, u8 curr_ver,
> +					     u8 compat_ver, u8 len_ver, u32 *len)
> +{
> +	u8 struct_ver, struct_compat;
> +
> +	ceph_decode_8_safe(p, end, struct_ver, fail);
> +	if (struct_ver >= curr_ver) {

It seems to me it doesn't matter whether the current code
supports the struct compat version or not.  What matters
is whether the encoded header contains the compat field
or not.  I'm not sure what determines that.

> +		ceph_decode_8_safe(p, end, struct_compat, fail);
> +		if (curr_ver < struct_compat)
> +			return -EINVAL;
> +	}
> +
> +	if (struct_ver >= len_ver) {
> +		ceph_decode_32_safe(p, end, *len, fail);
> +	} else {
> +		*len = 0;
> +	}
> +
> +	return 0;
> +fail:
> +	return -ERANGE;
> +}
> +
> +
>   #define ceph_encode_need(p, end, n, bad)			\
>   	do {							\
>   		if (!likely(ceph_has_room(p, end, n)))		\
>

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alex Elder April 30, 2015, 12:22 p.m. UTC | #2

On 04/28/2015 05:05 PM, [e-mail] wrote:
> From: Mike Christie <[e-mail]>
>
> This patch adds helpers to encode/decode the starting blocks
> locking code. They are the equivalent of ENCODE_START and
> DECODE_START_LEGACY_COMPAT_LEN in the userspace ceph code.

Your subject line says "start/finish encoding" but really it's just
encode/decode.

I have a few suggestions and questions, below.

					-Alex

(I'm sorry if this got sent twice, I found it in my drafts folder
this morning so it seems it did not get sent.)

> Signed-off-by: Mike Christie <[e-mail]>
> ---
>   include/linux/ceph/decode.h | 55 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index a6ef9cc..96ec43d 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -217,6 +217,61 @@ static inline void ceph_encode_string(void **p, void *end,
>   	*p += len;
>   }
>
> +/*
> + * version and length starting block encoders/decoders
> + */
> +
> +/* current code version (u8) + compat code version (u8) + len of struct (u32) */
> +#define CEPH_ENCODING_START_BLK_LEN 6

I don't see this used. (I'm sure it will be soon though.)

Why not just explain what it is in code?

#define CEPH_ENCODING_START_BLK_LEN \
		(sizeof(u8) + sizeof(u8) + sizeof(u32))

> +/**
> + * ceph_start_encoding - start encoding block
> + * @p: buffer to encode data in
> + * @curr_ver: current (code) version of the encoding
> + * @compat_ver: oldest code version that can decode it
> + * @len: length of data that will be encoded in buffer
> + */
> +static inline void ceph_start_encoding(void **p, u8 curr_ver, u8 compat_ver,
> +				       u32 len)
> +{
> +	ceph_encode_8(p, curr_ver);
> +	ceph_encode_8(p, compat_ver);
> +	ceph_encode_32(p, len);
> +}
> +
> +/**
> + * ceph_start_decoding_compat - decode block with legacy support for older schemes
> + * @p: buffer to decode
> + * @end: end of decode buffer
> + * @curr_ver: current version of the encoding that the code supports/encode
> + * @compat_ver: oldest version that includes a __u8 compat version field
> + * @len_ver: oldest version that includes a __u32 length wrapper
> + * @len: buffer to return len of data in buffer
> + */
> +static inline int ceph_start_decoding_compat(void **p, void *end, u8 curr_ver,
> +					     u8 compat_ver, u8 len_ver, u32 *len)
> +{
> +	u8 struct_ver, struct_compat;
> +
> +	ceph_decode_8_safe(p, end, struct_ver, fail);
> +	if (struct_ver >= curr_ver) {

It seems to me it doesn't matter whether the current code
supports the struct compat version or not.  What matters
is whether the encoded header contains the compat field
or not.  I'm not sure what determines that.

> +		ceph_decode_8_safe(p, end, struct_compat, fail);
> +		if (curr_ver < struct_compat)
> +			return -EINVAL;
> +	}
> +
> +	if (struct_ver >= len_ver) {
> +		ceph_decode_32_safe(p, end, *len, fail);
> +	} else {
> +		*len = 0;
> +	}
> +
> +	return 0;
> +fail:
> +	return -ERANGE;
> +}
> +
> +
>   #define ceph_encode_need(p, end, n, bad)			\
>   	do {							\
>   		if (!likely(ceph_has_room(p, end, n)))		\
>

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mike Christie May 1, 2015, 7:39 p.m. UTC | #3

On 04/30/2015 07:22 AM, Alex Elder wrote:
>> +/**
>> + * ceph_start_decoding_compat - decode block with legacy support for
>> older schemes
>> + * @p: buffer to decode
>> + * @end: end of decode buffer
>> + * @curr_ver: current version of the encoding that the code
>> supports/encode
>> + * @compat_ver: oldest version that includes a __u8 compat version field
>> + * @len_ver: oldest version that includes a __u32 length wrapper
>> + * @len: buffer to return len of data in buffer
>> + */
>> +static inline int ceph_start_decoding_compat(void **p, void *end, u8
>> curr_ver,
>> +                         u8 compat_ver, u8 len_ver, u32 *len)
>> +{
>> +    u8 struct_ver, struct_compat;
>> +
>> +    ceph_decode_8_safe(p, end, struct_ver, fail);
>> +    if (struct_ver >= curr_ver) {
> 
> It seems to me it doesn't matter whether the current code
> supports the struct compat version or not.  What matters
> is whether the encoded header contains the compat field
> or not.  I'm not sure what determines that.

I am not sure if I understood this comment.

I thought different structs got the compat field in different versions.
So, I was concerned about a case where we might get a old struct sent to
us. If the compat field was added to some struct_abc in version 2 and
that is what we support in the kernel, but some old osd sent us a struct
that was version 1, then we do not want to do the compat check below.


> 
>> +        ceph_decode_8_safe(p, end, struct_compat, fail);
>> +        if (curr_ver < struct_compat)
>> +            return -EINVAL;
>> +    }
>> +
>> +    if (struct_ver >= len_ver) {
>> +        ceph_decode_32_safe(p, end, *len, fail);
>> +    } else {
>> +        *len = 0;
>> +    }
>> +
>> +    return 0;
>> +fail:
>> +    return -ERANGE;
>> +}
>> +
>> +
>>   #define ceph_encode_need(p, end, n, bad)            \
>>       do {                            \
>>           if (!likely(ceph_has_room(p, end, n)))        \
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mike Christie May 1, 2015, 7:47 p.m. UTC | #4

On 05/01/2015 02:39 PM, Mike Christie wrote:
> On 04/30/2015 07:22 AM, Alex Elder wrote:
>>> +/**
>>> + * ceph_start_decoding_compat - decode block with legacy support for
>>> older schemes
>>> + * @p: buffer to decode
>>> + * @end: end of decode buffer
>>> + * @curr_ver: current version of the encoding that the code
>>> supports/encode
>>> + * @compat_ver: oldest version that includes a __u8 compat version field
>>> + * @len_ver: oldest version that includes a __u32 length wrapper
>>> + * @len: buffer to return len of data in buffer
>>> + */
>>> +static inline int ceph_start_decoding_compat(void **p, void *end, u8
>>> curr_ver,
>>> +                         u8 compat_ver, u8 len_ver, u32 *len)
>>> +{
>>> +    u8 struct_ver, struct_compat;
>>> +
>>> +    ceph_decode_8_safe(p, end, struct_ver, fail);
>>> +    if (struct_ver >= curr_ver) {
>>
>> It seems to me it doesn't matter whether the current code
>> supports the struct compat version or not.  What matters
>> is whether the encoded header contains the compat field
>> or not.  I'm not sure what determines that.
> 
> I am not sure if I understood this comment.
> 
> I thought different structs got the compat field in different versions.
> So, I was concerned about a case where we might get a old struct sent to
> us. If the compat field was added to some struct_abc in version 2 and
> that is what we support in the kernel, but some old osd sent us a struct
> that was version 1, then we do not want to do the compat check below.
> 

Doh! I wrote the above mail, then realized what you meant.

I think I should have checked the compat_ver passed into the version above.

if (struct_ver >= compat_ver) {
	ceph_decode_8_safe(p, end, struct_compat, fail);
	if (curr_ver < struct_compat)

> 
>>
>>> +        ceph_decode_8_safe(p, end, struct_compat, fail);
>>> +        if (curr_ver < struct_compat)
>>> +            return -EINVAL;
>>> +    }
>>> +
>>> +    if (struct_ver >= len_ver) {
>>> +        ceph_decode_32_safe(p, end, *len, fail);
>>> +    } else {
>>> +        *len = 0;
>>> +    }
>>> +
>>> +    return 0;
>>> +fail:
>>> +    return -ERANGE;
>>> +}
>>> +
>>> +
>>>   #define ceph_encode_need(p, end, n, bad)            \
>>>       do {                            \
>>>           if (!likely(ceph_has_room(p, end, n)))        \
>>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alex Elder May 1, 2015, 7:53 p.m. UTC | #5

On 05/01/2015 02:47 PM, Mike Christie wrote:
> On 05/01/2015 02:39 PM, Mike Christie wrote:
>> On 04/30/2015 07:22 AM, Alex Elder wrote:
>>>> +/**
>>>> + * ceph_start_decoding_compat - decode block with legacy support for
>>>> older schemes
>>>> + * @p: buffer to decode
>>>> + * @end: end of decode buffer
>>>> + * @curr_ver: current version of the encoding that the code
>>>> supports/encode
>>>> + * @compat_ver: oldest version that includes a __u8 compat version field
>>>> + * @len_ver: oldest version that includes a __u32 length wrapper
>>>> + * @len: buffer to return len of data in buffer
>>>> + */
>>>> +static inline int ceph_start_decoding_compat(void **p, void *end, u8
>>>> curr_ver,
>>>> +                         u8 compat_ver, u8 len_ver, u32 *len)
>>>> +{
>>>> +    u8 struct_ver, struct_compat;
>>>> +
>>>> +    ceph_decode_8_safe(p, end, struct_ver, fail);
>>>> +    if (struct_ver >= curr_ver) {
>>>
>>> It seems to me it doesn't matter whether the current code
>>> supports the struct compat version or not.  What matters
>>> is whether the encoded header contains the compat field
>>> or not.  I'm not sure what determines that.
>>
>> I am not sure if I understood this comment.
>>
>> I thought different structs got the compat field in different versions.
>> So, I was concerned about a case where we might get a old struct sent to
>> us. If the compat field was added to some struct_abc in version 2 and
>> that is what we support in the kernel, but some old osd sent us a struct
>> that was version 1, then we do not want to do the compat check below.
>>
> 
> Doh! I wrote the above mail, then realized what you meant.

OK good...  And I should have known what determines
whether the header contains the compat field, but
I was already a little confused about what I was
looking at...

					-Alex

> I think I should have checked the compat_ver passed into the version above.
> 
> if (struct_ver >= compat_ver) {
> 	ceph_decode_8_safe(p, end, struct_compat, fail);
> 	if (curr_ver < struct_compat)
> 
>>
>>>
>>>> +        ceph_decode_8_safe(p, end, struct_compat, fail);
>>>> +        if (curr_ver < struct_compat)
>>>> +            return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if (struct_ver >= len_ver) {
>>>> +        ceph_decode_32_safe(p, end, *len, fail);
>>>> +    } else {
>>>> +        *len = 0;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +fail:
>>>> +    return -ERANGE;
>>>> +}
>>>> +
>>>> +
>>>>   #define ceph_encode_need(p, end, n, bad)            \
>>>>       do {                            \
>>>>           if (!likely(ceph_has_room(p, end, n)))        \
>>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to [e-mail]
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
index a6ef9cc..96ec43d 100644
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -217,6 +217,61 @@  static inline void ceph_encode_string(void **p, void *end,
 	*p += len;
 }
 
+/*
+ * version and length starting block encoders/decoders
+ */
+
+/* current code version (u8) + compat code version (u8) + len of struct (u32) */
+#define CEPH_ENCODING_START_BLK_LEN 6
+
+/**
+ * ceph_start_encoding - start encoding block
+ * @p: buffer to encode data in
+ * @curr_ver: current (code) version of the encoding
+ * @compat_ver: oldest code version that can decode it
+ * @len: length of data that will be encoded in buffer
+ */
+static inline void ceph_start_encoding(void **p, u8 curr_ver, u8 compat_ver,
+				       u32 len)
+{
+	ceph_encode_8(p, curr_ver);
+	ceph_encode_8(p, compat_ver);
+	ceph_encode_32(p, len);
+}
+
+/**
+ * ceph_start_decoding_compat - decode block with legacy support for older schemes
+ * @p: buffer to decode
+ * @end: end of decode buffer
+ * @curr_ver: current version of the encoding that the code supports/encode
+ * @compat_ver: oldest version that includes a __u8 compat version field
+ * @len_ver: oldest version that includes a __u32 length wrapper
+ * @len: buffer to return len of data in buffer
+ */
+static inline int ceph_start_decoding_compat(void **p, void *end, u8 curr_ver,
+					     u8 compat_ver, u8 len_ver, u32 *len)
+{
+	u8 struct_ver, struct_compat;
+
+	ceph_decode_8_safe(p, end, struct_ver, fail);
+	if (struct_ver >= curr_ver) {
+		ceph_decode_8_safe(p, end, struct_compat, fail);
+		if (curr_ver < struct_compat)
+			return -EINVAL;
+	}
+
+	if (struct_ver >= len_ver) {
+		ceph_decode_32_safe(p, end, *len, fail);
+	} else {
+		*len = 0;
+	}
+
+	return 0;
+fail:
+	return -ERANGE;
+}
+
+
 #define ceph_encode_need(p, end, n, bad)			\
 	do {							\
 		if (!likely(ceph_has_room(p, end, n)))		\