Skip to contentSkip to conversation

relay: When unsigned ret cannot store a negative value

PATCH In review roel kluin 4 participants 5 messages Nov 29, 2008 – Dec 3, 2008 Relay

lore
Share & tools
lore

roel kluin

author
#0 lore raw

PATCH 1/1 relay: When unsigned ret cannot store a negative value

When unsigned ret cannot store a negative value

Signed-off-by Roel Kluin <[email protected]>

---
UNTESTED! is this the way to go?

kernel/relay.c +4 -2
index 32b0bef..c9d62e5 100644
@@ -1220,7 +1220,8 @@ static int subbuf_splice_actor(struct file *in,
 			       unsigned int flags,
 			       int *nonpad_ret)
 {
-	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages, ret;
+	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages;
+	ssize_t ret;
 	struct rchan_buf *rbuf = in->private_data;
 	unsigned int subbuf_size = rbuf->chan->subbuf_size;
 	uint64_t pos = (uint64_t) *ppos;

@@ -1289,7 +1290,8 @@ static int subbuf_splice_actor(struct file *in,
 	if (!spd.nr_pages)
 		return 0;
 
-	ret = *nonpad_ret = splice_to_pipe(pipe, &spd);
+	ret = splice_to_pipe(pipe, &spd);
+	*nonpad_ret = ret;
 	if (ret < 0 || ret < total_len)
 		return ret;
maintainer
#1 lore raw
Quoted 21 lines · On Sat, 29 Nov 2008 10:38:06 -0500 roel kluin <[email protected]> wrote:

When unsigned ret cannot store a negative value

Signed-off-by Roel Kluin <[email protected]>

---
UNTESTED! is this the way to go?

kernel/relay.c +2 -1
index 32b0bef..c9d62e5 100644
@@ -1220,7 +1220,8 @@ static int subbuf_splice_actor(struct file *in,
 			       unsigned int flags,
 			       int *nonpad_ret)
 {
-	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages, ret;
+	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages;
+	ssize_t ret;
 	struct rchan_buf *rbuf = in->private_data;
 	unsigned int subbuf_size = rbuf->chan->subbuf_size;
 	uint64_t pos = (uint64_t) *ppos;

kernel/relay.c static int subbuf_splice_actor(struct file *in, · line 1297

Quoted 9 lines
kernel/relay.c +2 -1
@@ -1289,7 +1290,8 @@ static int subbuf_splice_actor(struct file *in,
 	if (!spd.nr_pages)
 		return 0;
 
-	ret = *nonpad_ret = splice_to_pipe(pipe, &spd);
+	ret = splice_to_pipe(pipe, &spd);
+	*nonpad_ret = ret;
 	if (ret < 0 || ret < total_len)
 		return ret;

Yeah, this code needs help. subbuf_splice_actor() returns `int', but carefully calculates and returns an unsigned type.

I suspect that quite a bit of code in there (including relay_file_splice_read()) should be gone through and have its choice of types reviewed and fixed. Probably by converting things to ssize_t.

#2 lore raw
Quoted 37 lines · Andrew Morton
Quoted 21 lines · On Sat, 29 Nov 2008 10:38:06 -0500 roel kluin <[email protected]> wrote:

When unsigned ret cannot store a negative value

Signed-off-by Roel Kluin <[email protected]>

---
UNTESTED! is this the way to go?

kernel/relay.c +2 -1
index 32b0bef..c9d62e5 100644
@@ -1220,7 +1220,8 @@ static int subbuf_splice_actor(struct file *in,
 			       unsigned int flags,
 			       int *nonpad_ret)
 {
-	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages, ret;
+	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages;
+	ssize_t ret;
 	struct rchan_buf *rbuf = in->private_data;
 	unsigned int subbuf_size = rbuf->chan->subbuf_size;
 	uint64_t pos = (uint64_t) *ppos;

kernel/relay.c static int subbuf_splice_actor(struct file *in, · line 1297

Quoted 9 lines
kernel/relay.c +2 -1
@@ -1289,7 +1290,8 @@ static int subbuf_splice_actor(struct file *in,
 	if (!spd.nr_pages)
 		return 0;
 
-	ret = *nonpad_ret = splice_to_pipe(pipe, &spd);
+	ret = splice_to_pipe(pipe, &spd);
+	*nonpad_ret = ret;
 	if (ret < 0 || ret < total_len)
 		return ret;

Yeah, this code needs help. subbuf_splice_actor() returns `int', but carefully calculates and returns an unsigned type.

Quoted 5 lines

I suspect that quite a bit of code in there (including relay_file_splice_read()) should be gone through and have its choice of types reviewed and fixed. Probably by converting things to ssize_t.

And maybe generic_file_splice_read() too, from whence it came. Speaking of which, is there some reason why this patch:

http://lkml.indiana.edu/hypermail/linux/kernel/0810.3/0094.html

never got picked up?

maintainer
#3 lore raw
Quoted 39 lines · On Tue, 02 Dec 2008 22:29:51 -0600 Tom Zanussi <[email protected]> wrote:
Quoted 37 lines · Andrew Morton
Quoted 21 lines · On Sat, 29 Nov 2008 10:38:06 -0500 roel kluin <[email protected]> wrote:

When unsigned ret cannot store a negative value

Signed-off-by Roel Kluin <[email protected]>

---
UNTESTED! is this the way to go?

kernel/relay.c +2 -1
index 32b0bef..c9d62e5 100644
@@ -1220,7 +1220,8 @@ static int subbuf_splice_actor(struct file *in,
 			       unsigned int flags,
 			       int *nonpad_ret)
 {
-	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages, ret;
+	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages;
+	ssize_t ret;
 	struct rchan_buf *rbuf = in->private_data;
 	unsigned int subbuf_size = rbuf->chan->subbuf_size;
 	uint64_t pos = (uint64_t) *ppos;

kernel/relay.c static int subbuf_splice_actor(struct file *in, · line 1297

Quoted 9 lines
kernel/relay.c +2 -1
@@ -1289,7 +1290,8 @@ static int subbuf_splice_actor(struct file *in,
 	if (!spd.nr_pages)
 		return 0;
 
-	ret = *nonpad_ret = splice_to_pipe(pipe, &spd);
+	ret = splice_to_pipe(pipe, &spd);
+	*nonpad_ret = ret;
 	if (ret < 0 || ret < total_len)
 		return ret;

Yeah, this code needs help. subbuf_splice_actor() returns `int', but carefully calculates and returns an unsigned type.

Quoted 7 lines
Quoted 5 lines

I suspect that quite a bit of code in there (including relay_file_splice_read()) should be gone through and have its choice of types reviewed and fixed. Probably by converting things to ssize_t.

And maybe generic_file_splice_read() too, from whence it came.

sure.

Quoted 7 lines

Speaking
of which, is there some reason why this patch:

http://lkml.indiana.edu/hypermail/linux/kernel/0810.3/0094.html

never got picked up?

Incompetence I guess.

Damn man, that's your third email address :(

#4 lore raw
Quoted 54 lines · Andrew Morton
Quoted 39 lines · On Tue, 02 Dec 2008 22:29:51 -0600 Tom Zanussi <[email protected]> wrote:
Quoted 37 lines · Andrew Morton
Quoted 21 lines · On Sat, 29 Nov 2008 10:38:06 -0500 roel kluin <[email protected]> wrote:

When unsigned ret cannot store a negative value

Signed-off-by Roel Kluin <[email protected]>

---
UNTESTED! is this the way to go?

kernel/relay.c +2 -1
index 32b0bef..c9d62e5 100644
@@ -1220,7 +1220,8 @@ static int subbuf_splice_actor(struct file *in,
 			       unsigned int flags,
 			       int *nonpad_ret)
 {
-	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages, ret;
+	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages;
+	ssize_t ret;
 	struct rchan_buf *rbuf = in->private_data;
 	unsigned int subbuf_size = rbuf->chan->subbuf_size;
 	uint64_t pos = (uint64_t) *ppos;

kernel/relay.c static int subbuf_splice_actor(struct file *in, · line 1297

Quoted 9 lines
kernel/relay.c +2 -1
@@ -1289,7 +1290,8 @@ static int subbuf_splice_actor(struct file *in,
 	if (!spd.nr_pages)
 		return 0;
 
-	ret = *nonpad_ret = splice_to_pipe(pipe, &spd);
+	ret = splice_to_pipe(pipe, &spd);
+	*nonpad_ret = ret;
 	if (ret < 0 || ret < total_len)
 		return ret;

Yeah, this code needs help. subbuf_splice_actor() returns `int', but carefully calculates and returns an unsigned type.

Quoted 7 lines
Quoted 5 lines

I suspect that quite a bit of code in there (including relay_file_splice_read()) should be gone through and have its choice of types reviewed and fixed. Probably by converting things to ssize_t.

And maybe generic_file_splice_read() too, from whence it came.

sure.

Quoted 3 lines

Speaking
of which, is there some reason why this patch:

Quoted 6 lines
Quoted 4 lines

http://lkml.indiana.edu/hypermail/linux/kernel/0810.3/0094.html

never got picked up?

Incompetence I guess.

No problem, thanks for finally picking it up. :-)

Quoted 2 lines

Damn man, that's your third email address :(

Yeah, the purpose of the gmail address is to avoid that but I forget to use it - will try to be less careless in the future. ;-)