[PATCH -next 2/2] bcache: Use max_t() when comparing different types

Post by Geert Uytterhoeven
drivers/md/bcache/btree.c: In function =E2=80=98insert_u64s_remaining=
drivers/md/bcache/btree.c:1816: warning: comparison of distinct point=

er types lacks a cast
[]

Post by Geert Uytterhoeven
diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c

[]

Post by Geert Uytterhoeven
@@ -1813,7 +1813,7 @@ static size_t insert_u64s_remaining(struct btre=

e *b)

Post by Geert Uytterhoeven
if (b->keys.ops->is_extents)
ret -=3D KEY_MAX_U64S;
=20
- return max(ret, 0L);
+ return max_t(ssize_t, ret, 0L);

why not
return max(ret, 0);

I think it odd that:

drivers/md/bcache/bset.h:static inline size_t bch_btree_keys_u64s_remai=
ning(struct btree_keys *b)

and:

static size_t insert_u64s_remaining(struct btree *b)
{
ssize_t ret =3D bch_btree_keys_u64s_remaining(&b->keys);

/*
* Might land in the middle of an existing extent and have to split it
*/
if (b->keys.ops->is_extents)
ret -=3D KEY_MAX_U64S;

return max(ret, 0L);
}

so the only use of that size_t inline is cast to ssize_t

Geert Uytterhoeven

2014-02-03 13:47:20 UTC

drivers/md/bcache/btree.c: In function =E2=80=98insert_u64s_remainin=
drivers/md/bcache/btree.c:1816: warning: comparison of distinct poin=

ter types lacks a cast

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c

[]

@@ -1813,7 +1813,7 @@ static size_t insert_u64s_remaining(struct btr=

ee *b)

if (b->keys.ops->is_extents)
ret -=3D KEY_MAX_U64S;
- return max(ret, 0L);
+ return max_t(ssize_t, ret, 0L);

why not
return max(ret, 0);

Indeed, that also works, on both 32-bit and 64-bit.
Will resend, now all the issues moved from -next to Linus' tree.

Post by Joe Perches
drivers/md/bcache/bset.h:static inline size_t bch_btree_keys_u64s_rem=

aining(struct btree_keys *b)

Post by Joe Perches
static size_t insert_u64s_remaining(struct btree *b)
{
ssize_t ret =3D bch_btree_keys_u64s_remaining(&b->keys);
/*
* Might land in the middle of an existing extent and have to=

split it

Post by Joe Perches
*/
if (b->keys.ops->is_extents)
ret -=3D KEY_MAX_U64S;
return max(ret, 0L);
}
so the only use of that size_t inline is cast to ssize_t

If the value returned by bch_btree_keys_u64s_remaining() is smaller
than KEY_MAX_U64S, it must stay negative after the subtraction,
instead of wrapping around to a big positive number. That's why ret is
ssize_t and not size_t.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-=
m68k.org

In personal conversations with technical people, I call myself a hacker=
=2E But
when I'm talking to journalists I just say "programmer" or something li=
ke that.
-- Linus Torvalds

Geert Uytterhoeven

2014-02-06 09:00:35 UTC

drivers/md/bcache/btree.c: In function =E2=80=98insert_u64s_remaini=
drivers/md/bcache/btree.c:1816: warning: comparison of distinct poi=

nter types lacks a cast

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c

[]

@@ -1813,7 +1813,7 @@ static size_t insert_u64s_remaining(struct bt=

ree *b)

if (b->keys.ops->is_extents)
ret -=3D KEY_MAX_U64S;
- return max(ret, 0L);
+ return max_t(ssize_t, ret, 0L);

why not
return max(ret, 0);

Indeed, that also works, on both 32-bit and 64-bit.
Will resend, now all the issues moved from -next to Linus' tree.

However, sparse doesn't like it, so we'll have to go for v1?

Post by Joe Perches
drivers/md/bcache/btree.c:1816:16: sparse: incompatible types in com=

parison expression (different type sizes)
In file included from arch/x86/include/asm/percpu.h:44:0,
from arch/x86/include/asm/preempt.h:5,
from include/linux/preempt.h:18,
from include/linux/spinlock.h:50,
from include/linux/wait.h:8,
from include/linux/fs.h:6,
from include/linux/highmem.h:4,
from include/linux/bio.h:23,
from drivers/md/bcache/bcache.h:181,
from drivers/md/bcache/btree.c:23:
drivers/md/bcache/btree.c: In function 'insert_u64s_remaining':
include/linux/kernel.h:718:17: warning: comparison of distinct
pointer types lacks a cast [enabled by default]
(void) (&_max1 =3D=3D &_max2); \
^
drivers/md/bcache/btree.c:1816:9: note: in expansion of macro 'max'
return max(ret, 0);
^

vim +1816 drivers/md/bcache/btree.c

1800 status);
1801 return true;
1802 } else
1803 return false;
1804 }
1805
1806 static size_t insert_u64s_remaining(struct btree *b)
1807 {
1808 ssize_t ret =3D bch_btree_keys_u64s_remaining(&b->keys)=
;
1809
1810 /*
1811 * Might land in the middle of an existing extent and
have to split it
1812 */
1813 if (b->keys.ops->is_extents)
1814 ret -=3D KEY_MAX_U64S;
1815

Post by Geert Uytterhoeven
1816 return max(ret, 0);

1817 }
1818
1819 static bool bch_btree_insert_keys(struct btree *b, struct btree=
_op *op,
1820 struct keylist *insert_keys,
1821 struct bkey *replace_key)
1822 {
1823 bool ret =3D false;
1824 int oldsize =3D bch_count_data(&b->keys);

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-=
m68k.org

In personal conversations with technical people, I call myself a hacker=
=2E But
when I'm talking to journalists I just say "programmer" or something li=
ke that.
-- Linus Torvalds

Joe Perches

2014-02-06 09:06:47 UTC

drivers/md/bcache/btree.c: In function =E2=80=98insert_u64s_remai=
drivers/md/bcache/btree.c:1816: warning: comparison of distinct p=

ointer types lacks a cast

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.=

@@ -1813,7 +1813,7 @@ static size_t insert_u64s_remaining(struct =

btree *b)

if (b->keys.ops->is_extents)
ret -=3D KEY_MAX_U64S;
- return max(ret, 0L);
+ return max_t(ssize_t, ret, 0L);

why not
return max(ret, 0);

Indeed, that also works, on both 32-bit and 64-bit.
Will resend, now all the issues moved from -next to Linus' tree.

=20
However, sparse doesn't like it, so we'll have to go for v1?

Seems so.

Andrew Morton

2014-02-06 20:38:24 UTC

drivers/md/bcache/btree.c:1816: warning: comparison of distinct pointer types lacks a cast

[]

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c

[]

@@ -1813,7 +1813,7 @@ static size_t insert_u64s_remaining(struct btree *b)
if (b->keys.ops->is_extents)
ret -= KEY_MAX_U64S;
- return max(ret, 0L);
+ return max_t(ssize_t, ret, 0L);

why not
return max(ret, 0);

Indeed, that also works, on both 32-bit and 64-bit.
Will resend, now all the issues moved from -next to Linus' tree.

However, sparse doesn't like it, so we'll have to go for v1?

Seems so.

Kent, was there any secret reason why insert_u64s_remaining():ret has
type ssize_t? The function returns size_t and
bch_btree_keys_u64s_remaining() returns size_t so I think I'll do the
obvious:

--- a/drivers/md/bcache/btree.c~bcache-drop-l-suffix-when-comparing-ssize_t-with-0-fix
+++ a/drivers/md/bcache/btree.c
@@ -1805,7 +1805,7 @@ static bool btree_insert_key(struct btre

static size_t insert_u64s_remaining(struct btree *b)
{
- ssize_t ret = bch_btree_keys_u64s_remaining(&b->keys);
+ size_t ret = bch_btree_keys_u64s_remaining(&b->keys);

/*
* Might land in the middle of an existing extent and have to split it
@@ -1813,7 +1813,7 @@ static size_t insert_u64s_remaining(stru
if (b->keys.ops->is_extents)
ret -= KEY_MAX_U64S;

- return max(ret, 0);
+ return max_t(size_t, ret, 0);
}

static bool bch_btree_insert_keys(struct btree *b, struct btree_op *op,
_

Geert Uytterhoeven

2014-02-06 20:45:36 UTC

Post by Andrew Morton

drivers/md/bcache/btree.c:1816: warning: comparison of distinct pointer types lacks a cast

[]

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c

[]

@@ -1813,7 +1813,7 @@ static size_t insert_u64s_remaining(struct btree *b)
if (b->keys.ops->is_extents)
ret -= KEY_MAX_U64S;
- return max(ret, 0L);
+ return max_t(ssize_t, ret, 0L);

why not
return max(ret, 0);

Indeed, that also works, on both 32-bit and 64-bit.
Will resend, now all the issues moved from -next to Linus' tree.

However, sparse doesn't like it, so we'll have to go for v1?

Seems so.

Kent, was there any secret reason why insert_u64s_remaining():ret has
type ssize_t? The function returns size_t and
bch_btree_keys_u64s_remaining() returns size_t so I think I'll do the
--- a/drivers/md/bcache/btree.c~bcache-drop-l-suffix-when-comparing-ssize_t-with-0-fix
+++ a/drivers/md/bcache/btree.c
@@ -1805,7 +1805,7 @@ static bool btree_insert_key(struct btre
static size_t insert_u64s_remaining(struct btree *b)
{
- ssize_t ret = bch_btree_keys_u64s_remaining(&b->keys);
+ size_t ret = bch_btree_keys_u64s_remaining(&b->keys);
/*
* Might land in the middle of an existing extent and have to split it
@@ -1813,7 +1813,7 @@ static size_t insert_u64s_remaining(stru
if (b->keys.ops->is_extents)
ret -= KEY_MAX_U64S;

I think the reason is the line above: with size_t, ret may become a big
positive number when the subtraction wraps below zero.

Post by Andrew Morton
- return max(ret, 0);
+ return max_t(size_t, ret, 0);

That part is OK, cfr. my v1 (which I had planned to send out as v3 again).

Post by Andrew Morton
}
static bool bch_btree_insert_keys(struct btree *b, struct btree_op *op,

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Andrew Morton

2014-02-06 20:50:58 UTC

Post by Andrew Morton
--- a/drivers/md/bcache/btree.c~bcache-drop-l-suffix-when-comparing-ssize_t-with-0-fix
+++ a/drivers/md/bcache/btree.c
@@ -1805,7 +1805,7 @@ static bool btree_insert_key(struct btre
static size_t insert_u64s_remaining(struct btree *b)
{
- ssize_t ret = bch_btree_keys_u64s_remaining(&b->keys);
+ size_t ret = bch_btree_keys_u64s_remaining(&b->keys);
/*
* Might land in the middle of an existing extent and have to split it
@@ -1813,7 +1813,7 @@ static size_t insert_u64s_remaining(stru
if (b->keys.ops->is_extents)
ret -= KEY_MAX_U64S;

I think the reason is the line above: with size_t, ret may become a big
positive number when the subtraction wraps below zero.

Well, I assumed that case would be a bug - otherwise the programmer
would have commented such a subtlety. Wouldn't he?

Post by Andrew Morton
- return max(ret, 0);
+ return max_t(size_t, ret, 0);

That part is OK, cfr. my v1 (which I had planned to send out as v3 again).

It needs to be ssize_t.

Geert Uytterhoeven

2014-02-06 20:53:34 UTC

Post by Andrew Morton

I think the reason is the line above: with size_t, ret may become a big
positive number when the subtraction wraps below zero.

Well, I assumed that case would be a bug - otherwise the programmer
would have commented such a subtlety. Wouldn't he?