[PATCH 0/2] libceph: odds and ends

[PATCH 0/2] libceph: odds and ends

[Alex Elder]

(These patches, along with the patches I'll be
posting in the next few minutes, are available in
the "review/wip-stripe-v2" branch of the ceph-client
git repository.)


This series just fleshes out some code in libceph.
The first completes the set of definitions for limit
values (like U8_MAX) to include the signed minimum
and maximum values.  The second just adds some error
checking and type casting to timespec encoding and
decoding routines.

					-Alex

[PATCH 1/2] libceph: add signed type limits
[PATCH 2/2] libceph: validate timespec conversions

[PATCH 1/2] libceph: add signed type limits

[Alex Elder]

A ceph timespec contains 32-bit unsigned values for its seconds and
nanoseconds components.  For a standard timespec, both fields are
signed, and the seconds field is almost surely 64 bits.

Add some explicit casts so the fact that this conversion is taking
place is obvious.  Also trip a bug if we ever try to put out of
range (negative or too big) values into a ceph timespec.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/decode.h |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
index 9575a52..379f715 100644
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -154,14 +154,19 @@ bad:
 static inline void ceph_decode_timespec(struct timespec *ts,
 					const struct ceph_timespec *tv)
 {
-	ts->tv_sec = le32_to_cpu(tv->tv_sec);
-	ts->tv_nsec = le32_to_cpu(tv->tv_nsec);
+	ts->tv_sec = (__kernel_time_t)le32_to_cpu(tv->tv_sec);
+	ts->tv_nsec = (long)le32_to_cpu(tv->tv_nsec);
 }
 static inline void ceph_encode_timespec(struct ceph_timespec *tv,
 					const struct timespec *ts)
 {
-	tv->tv_sec = cpu_to_le32(ts->tv_sec);
-	tv->tv_nsec = cpu_to_le32(ts->tv_nsec);
+	BUG_ON(ts->tv_sec < 0);
+	BUG_ON(ts->tv_sec > (__kernel_time_t)U32_MAX);
+	BUG_ON(ts->tv_nsec < 0);
+	BUG_ON(ts->tv_nsec > (long)U32_MAX);
+
+	tv->tv_sec = cpu_to_le32((u32)ts->tv_sec);
+	tv->tv_nsec = cpu_to_le32((u32)ts->tv_nsec);
 }

 /*
-- 
1.7.9.5

Re: [PATCH 1/2] libceph: add signed type limits

[Alex Elder]

(Sorry, the subject line didn't match the content.
Please ignore the previous one--this is the right
patch.)


Flesh out the limits defined in <linux/ceph/decode.h> to include the
maximum and minimum values for signed type S8, S16, S32, and S64.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/decode.h |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
index 689f1df..9575a52 100644
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -10,10 +10,20 @@

 /* This seemed to be the easiest place to define these */

-#define	U8_MAX	((u8)  (~0U))
-#define	U16_MAX	((u16) (~0U))
-#define	U32_MAX	((u32) (~0U))
-#define	U64_MAX	((u64) (~0ULL))
+#define	U8_MAX	((u8)(~0U))
+#define	U16_MAX	((u16)(~0U))
+#define	U32_MAX	((u32)(~0U))
+#define	U64_MAX	((u64)(~0ULL))
+
+#define	S8_MAX	((s8)(U8_MAX >> 1))
+#define	S16_MAX	((s16)(U16_MAX >> 1))
+#define	S32_MAX	((s32)(U32_MAX >> 1))
+#define	S64_MAX	((s64)(U64_MAX >> 1LL))
+
+#define	S8_MIN	((s8)(-S8_MAX - 1))
+#define	S16_MIN	((s16)(-S16_MAX - 1))
+#define	S32_MIN	((s32)(-S32_MAX - 1))
+#define	S64_MIN	((s64)(-S64_MAX - 1LL))

 /*
  * in all cases,
-- 
1.7.9.5

Re: [PATCH 1/2] libceph: add signed type limits

[Josh Durgin]

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

On 04/21/2013 02:14 PM, Alex Elder wrote:
> (Sorry, the subject line didn't match the content.
> Please ignore the previous one--this is the right
> patch.)
>
>
> Flesh out the limits defined in <linux/ceph/decode.h> to include the
> maximum and minimum values for signed type S8, S16, S32, and S64.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   include/linux/ceph/decode.h |   18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index 689f1df..9575a52 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -10,10 +10,20 @@
>
>   /* This seemed to be the easiest place to define these */
>
> -#define	U8_MAX	((u8)  (~0U))
> -#define	U16_MAX	((u16) (~0U))
> -#define	U32_MAX	((u32) (~0U))
> -#define	U64_MAX	((u64) (~0ULL))
> +#define	U8_MAX	((u8)(~0U))
> +#define	U16_MAX	((u16)(~0U))
> +#define	U32_MAX	((u32)(~0U))
> +#define	U64_MAX	((u64)(~0ULL))
> +
> +#define	S8_MAX	((s8)(U8_MAX >> 1))
> +#define	S16_MAX	((s16)(U16_MAX >> 1))
> +#define	S32_MAX	((s32)(U32_MAX >> 1))
> +#define	S64_MAX	((s64)(U64_MAX >> 1LL))
> +
> +#define	S8_MIN	((s8)(-S8_MAX - 1))
> +#define	S16_MIN	((s16)(-S16_MAX - 1))
> +#define	S32_MIN	((s32)(-S32_MAX - 1))
> +#define	S64_MIN	((s64)(-S64_MAX - 1LL))
>
>   /*
>    * in all cases,
>

[PATCH 2/2] libceph: validate timespec conversions

[Alex Elder]

A ceph timespec contains 32-bit unsigned values for its seconds and
nanoseconds components.  For a standard timespec, both fields are
signed, and the seconds field is almost surely 64 bits.

Add some explicit casts so the fact that this conversion is taking
place is obvious.  Also trip a bug if we ever try to put out of
range (negative or too big) values into a ceph timespec.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/decode.h |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
index 9575a52..379f715 100644
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -154,14 +154,19 @@ bad:
 static inline void ceph_decode_timespec(struct timespec *ts,
 					const struct ceph_timespec *tv)
 {
-	ts->tv_sec = le32_to_cpu(tv->tv_sec);
-	ts->tv_nsec = le32_to_cpu(tv->tv_nsec);
+	ts->tv_sec = (__kernel_time_t)le32_to_cpu(tv->tv_sec);
+	ts->tv_nsec = (long)le32_to_cpu(tv->tv_nsec);
 }
 static inline void ceph_encode_timespec(struct ceph_timespec *tv,
 					const struct timespec *ts)
 {
-	tv->tv_sec = cpu_to_le32(ts->tv_sec);
-	tv->tv_nsec = cpu_to_le32(ts->tv_nsec);
+	BUG_ON(ts->tv_sec < 0);
+	BUG_ON(ts->tv_sec > (__kernel_time_t)U32_MAX);
+	BUG_ON(ts->tv_nsec < 0);
+	BUG_ON(ts->tv_nsec > (long)U32_MAX);
+
+	tv->tv_sec = cpu_to_le32((u32)ts->tv_sec);
+	tv->tv_nsec = cpu_to_le32((u32)ts->tv_nsec);
 }

 /*
-- 
1.7.9.5

Re: [PATCH 2/2] libceph: validate timespec conversions

[Matt W. Benjamin]

----- "Alex Elder" <elder@inktank.com> wrote:

> A ceph timespec contains 32-bit unsigned values for its seconds and
> nanoseconds components.  For a standard timespec, both fields are
> signed, and the seconds field is almost surely 64 bits.

Is the Ceph timespec going to change at some point?

> 
> Add some explicit casts so the fact that this conversion is taking
> place is obvious.  Also trip a bug if we ever try to put out of
> range (negative or too big) values into a ceph timespec.
> 

-- 
Matt Benjamin
The Linux Box
206 South Fifth Ave. Suite 150
Ann Arbor, MI  48104

http://linuxbox.com

tel.  734-761-4689 
fax.  734-769-8938 
cel.  734-216-5309 

Re: [PATCH 2/2] libceph: validate timespec conversions

[Sage Weil]

On Mon, 22 Apr 2013, Matt W. Benjamin wrote:
> 
> ----- "Alex Elder" <elder@inktank.com> wrote:
> 
> > A ceph timespec contains 32-bit unsigned values for its seconds and
> > nanoseconds components.  For a standard timespec, both fields are
> > signed, and the seconds field is almost surely 64 bits.
> 
> Is the Ceph timespec going to change at some point?

I don't think so.  32-bits is enough for the billion nanoseconds in a 
second.  And I'm not sure if the signedness is used/useful... the ceph 
utime_t code always normalizes the ns result to be in [0, 1 billion).

sage

> 
> > 
> > Add some explicit casts so the fact that this conversion is taking
> > place is obvious.  Also trip a bug if we ever try to put out of
> > range (negative or too big) values into a ceph timespec.
> > 
> 
> -- 
> Matt Benjamin
> The Linux Box
> 206 South Fifth Ave. Suite 150
> Ann Arbor, MI  48104
> 
> http://linuxbox.com
> 
> tel.  734-761-4689 
> fax.  734-769-8938 
> cel.  734-216-5309 
> --
> 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: [PATCH 2/2] libceph: validate timespec conversions

[Matt W. Benjamin]

I was thinking about the seconds component.

----- "Sage Weil" <sage@inktank.com> wrote:

> On Mon, 22 Apr 2013, Matt W. Benjamin wrote:
> > 
> > ----- "Alex Elder" <elder@inktank.com> wrote:
> > 
> > > A ceph timespec contains 32-bit unsigned values for its seconds
> and
> > > nanoseconds components.  For a standard timespec, both fields are
> > > signed, and the seconds field is almost surely 64 bits.
> > 
> > Is the Ceph timespec going to change at some point?
> 
> I don't think so.  32-bits is enough for the billion nanoseconds in a
> 
> second.  And I'm not sure if the signedness is used/useful... the ceph
> 
> utime_t code always normalizes the ns result to be in [0, 1 billion).
> 
> sage


-- 
Matt Benjamin
The Linux Box
206 South Fifth Ave. Suite 150
Ann Arbor, MI  48104

http://linuxbox.com

tel.  734-761-4689 
fax.  734-769-8938 
cel.  734-216-5309 

Re: [PATCH 2/2] libceph: validate timespec conversions

[Sage Weil]

On Mon, 22 Apr 2013, Matt W. Benjamin wrote:
> I was thinking about the seconds component.

Oh, right.. that's the unix epoch(alypse) in 2038 or something?

That we should probably fix.  :)

s

> 
> ----- "Sage Weil" <sage@inktank.com> wrote:
> 
> > On Mon, 22 Apr 2013, Matt W. Benjamin wrote:
> > > 
> > > ----- "Alex Elder" <elder@inktank.com> wrote:
> > > 
> > > > A ceph timespec contains 32-bit unsigned values for its seconds
> > and
> > > > nanoseconds components.  For a standard timespec, both fields are
> > > > signed, and the seconds field is almost surely 64 bits.
> > > 
> > > Is the Ceph timespec going to change at some point?
> > 
> > I don't think so.  32-bits is enough for the billion nanoseconds in a
> > 
> > second.  And I'm not sure if the signedness is used/useful... the ceph
> > 
> > utime_t code always normalizes the ns result to be in [0, 1 billion).
> > 
> > sage
> 
> 
> -- 
> Matt Benjamin
> The Linux Box
> 206 South Fifth Ave. Suite 150
> Ann Arbor, MI  48104
> 
> http://linuxbox.com
> 
> tel.  734-761-4689 
> fax.  734-769-8938 
> cel.  734-216-5309 
> --
> 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: [PATCH 2/2] libceph: validate timespec conversions

[Alex Elder]

On 04/22/2013 11:12 AM, Matt W. Benjamin wrote:
> I was thinking about the seconds component.

I wondered the same thing.  It will most likely
have to some time in the next 25 years or so.

					-Alex

> ----- "Sage Weil" <sage@inktank.com> wrote:
> 
>> On Mon, 22 Apr 2013, Matt W. Benjamin wrote:
>>>
>>> ----- "Alex Elder" <elder@inktank.com> wrote:
>>>
>>>> A ceph timespec contains 32-bit unsigned values for its seconds
>> and
>>>> nanoseconds components.  For a standard timespec, both fields are
>>>> signed, and the seconds field is almost surely 64 bits.
>>>
>>> Is the Ceph timespec going to change at some point?
>>
>> I don't think so.  32-bits is enough for the billion nanoseconds in a
>>
>> second.  And I'm not sure if the signedness is used/useful... the ceph
>>
>> utime_t code always normalizes the ns result to be in [0, 1 billion).
>>
>> sage
> 
> 

Re: [PATCH 2/2] libceph: validate timespec conversions

[Josh Durgin]

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

On 04/21/2013 02:15 PM, Alex Elder wrote:
> A ceph timespec contains 32-bit unsigned values for its seconds and
> nanoseconds components.  For a standard timespec, both fields are
> signed, and the seconds field is almost surely 64 bits.
>
> Add some explicit casts so the fact that this conversion is taking
> place is obvious.  Also trip a bug if we ever try to put out of
> range (negative or too big) values into a ceph timespec.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   include/linux/ceph/decode.h |   13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index 9575a52..379f715 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -154,14 +154,19 @@ bad:
>   static inline void ceph_decode_timespec(struct timespec *ts,
>   					const struct ceph_timespec *tv)
>   {
> -	ts->tv_sec = le32_to_cpu(tv->tv_sec);
> -	ts->tv_nsec = le32_to_cpu(tv->tv_nsec);
> +	ts->tv_sec = (__kernel_time_t)le32_to_cpu(tv->tv_sec);
> +	ts->tv_nsec = (long)le32_to_cpu(tv->tv_nsec);
>   }
>   static inline void ceph_encode_timespec(struct ceph_timespec *tv,
>   					const struct timespec *ts)
>   {
> -	tv->tv_sec = cpu_to_le32(ts->tv_sec);
> -	tv->tv_nsec = cpu_to_le32(ts->tv_nsec);
> +	BUG_ON(ts->tv_sec < 0);
> +	BUG_ON(ts->tv_sec > (__kernel_time_t)U32_MAX);
> +	BUG_ON(ts->tv_nsec < 0);
> +	BUG_ON(ts->tv_nsec > (long)U32_MAX);
> +
> +	tv->tv_sec = cpu_to_le32((u32)ts->tv_sec);
> +	tv->tv_nsec = cpu_to_le32((u32)ts->tv_nsec);
>   }
>
>   /*
>