xfsprogs: fix wrong variable type in write_once function

PATCH In review Zorro Lang 2 participants 3 messages Nov 11, 2017

lore

Share & tools

lore

Zorro Lang

author

2017-11-11 13:58 UTC #0 lore raw

PATCH v2 1/1 xfsprogs: fix wrong variable type in write_once function

The 'Coverity Scan' found a problem in new write_once() function:

272 size_t bytes;
273 bytes = do_pwrite(file->fd, offset, count, count, pwritev2_flags);

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
CID 1420710: Control flow issues (NO_EFFECT)
This less-than-zero comparison of an unsigned value is never true. "bytes < 0UL".

274 if (bytes < 0)
275 return -1;

That's unreasonable. do_pwrite return 'ssize_t' type value, which can be less than zero, but we use a 'size_t' to get the return value. So change the size_t to ssize_t for it can store the return value correctly.

By the chance, correct all 'ssize_t' type problems in pwrite related functions.

Signed-off-by Zorro Lang <[email protected]>

---

V2 changed more than V1, as below:
1) The return value type of do_pwritev and do_pwrite, from 'int' to 'ssize_t'
2) The arguments type of do_pwritev and do_pwrite, from 'ssize_t' to 'size_t'

Thanks,
Zorro

 io/pwrite.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

io/pwrite.c +7 -7

index 26f79579..3df976a8 100644
@@ -61,12 +61,12 @@ pwrite_help(void)
 }
 
 #ifdef HAVE_PWRITEV
-static int
+static ssize_t
 do_pwritev(
 	int		fd,
 	off64_t		offset,
-	ssize_t		count,
-	ssize_t		buffer_size,
+	size_t		count,
+	size_t		buffer_size,
 	int 		pwritev2_flags)
 {
 	int vecs = 0;

@@ -105,12 +105,12 @@ do_pwritev(
 #define do_pwritev(fd, offset, count, buffer_size) (0)
 #endif
 
-static int
+static ssize_t
 do_pwrite(
 	int		fd,
 	off64_t		offset,
-	ssize_t		count,
-	ssize_t		buffer_size,
+	size_t		count,
+	size_t		buffer_size,
 	int		pwritev2_flags)
 {
 	if (!vectors)

@@ -269,7 +269,7 @@ write_once(
 	long long	*total,
 	int		pwritev2_flags)
 {
-	size_t bytes;
+	ssize_t bytes;
 	bytes = do_pwrite(file->fd, offset, count, count, pwritev2_flags);
 	if (bytes < 0)
 		return -1;

Signature · 1 line

2.13.6

Eric Sandeen

Reviewed-by on v2 1/1

2017-11-11 15:46 UTC #1 lore raw

Quoted 37 lines · Zorro Lang

The 'Coverity Scan' found a problem in new write_once() function:
272 size_t bytes;
273 bytes = do_pwrite(file->fd, offset, count, count, pwritev2_flags);
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
CID 1420710: Control flow issues (NO_EFFECT)
This less-than-zero comparison of an unsigned value is never true. "bytes < 0UL".
274 if (bytes < 0)
275 return -1;
That's unreasonable. do_pwrite return 'ssize_t' type value, which can be less than zero, but we use a 'size_t' to get the return value. So change the size_t to ssize_t for it can store the return value correctly.
By the chance, correct all 'ssize_t' type problems in pwrite related functions.
Signed-off-by Zorro Lang <[email protected]>
---
V2 changed more than V1, as below:
1) The return value type of do_pwritev and do_pwrite, from 'int' to 'ssize_t'
2) The arguments type of do_pwritev and do_pwrite, from 'ssize_t' to 'size_t'
Thanks,
Zorro
 io/pwrite.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
io/pwrite.c +0 -0
index 26f79579..3df976a8 100644
@@ -61,12 +61,12 @@ pwrite_help(void)
 }
 

io/pwrite.c pwrite_help(void) · line 69

Quoted 10 lines

io/pwrite.c +3 -3

 #ifdef HAVE_PWRITEV
-static int
+static ssize_t
 do_pwritev(
 	int		fd,
 	off64_t		offset,
-	ssize_t		count,
-	ssize_t		buffer_size,
+	size_t		count,
+	size_t		buffer_size,

I had not noticed this before, but this is odd; buffer_size is not used in this function. Instead, we test the global "buffersize"

I'm not sure what's going on there, need to read the code bit more. That's probably a separate fix though.

I think the rest looks ok,

Reviewed-by Eric Sandeen <[email protected]>

Quoted 30 lines

io/pwrite.c +4 -4

 	int 		pwritev2_flags)
 {
 	int vecs = 0;

@@ -105,12 +105,12 @@ do_pwritev(
 #define do_pwritev(fd, offset, count, buffer_size) (0)
 #endif
 
-static int
+static ssize_t
 do_pwrite(
 	int		fd,
 	off64_t		offset,
-	ssize_t		count,
-	ssize_t		buffer_size,
+	size_t		count,
+	size_t		buffer_size,
 	int		pwritev2_flags)
 {
 	if (!vectors)

@@ -269,7 +269,7 @@ write_once(
 	long long	*total,
 	int		pwritev2_flags)
 {
-	size_t bytes;
+	ssize_t bytes;
 	bytes = do_pwrite(file->fd, offset, count, count, pwritev2_flags);
 	if (bytes < 0)
 		return -1;

Zorro Lang

author

2017-11-11 16:22 UTC #2 lore raw

Quoted 52 lines · Eric Sandeen

Quoted 37 lines · Zorro Lang
The 'Coverity Scan' found a problem in new write_once() function:
272 size_t bytes;
273 bytes = do_pwrite(file->fd, offset, count, count, pwritev2_flags);
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
CID 1420710: Control flow issues (NO_EFFECT)
This less-than-zero comparison of an unsigned value is never true. "bytes < 0UL".
274 if (bytes < 0)
275 return -1;
That's unreasonable. do_pwrite return 'ssize_t' type value, which can be less than zero, but we use a 'size_t' to get the return value. So change the size_t to ssize_t for it can store the return value correctly.
By the chance, correct all 'ssize_t' type problems in pwrite related functions.
Signed-off-by Zorro Lang <[email protected]>
---
V2 changed more than V1, as below:
1) The return value type of do_pwritev and do_pwrite, from 'int' to 'ssize_t'
2) The arguments type of do_pwritev and do_pwrite, from 'ssize_t' to 'size_t'
Thanks,
Zorro
 io/pwrite.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
io/pwrite.c +0 -0
index 26f79579..3df976a8 100644
@@ -61,12 +61,12 @@ pwrite_help(void)
 }
 
io/pwrite.c pwrite_help(void) · line 69
Quoted 10 lines
io/pwrite.c +3 -3
 #ifdef HAVE_PWRITEV
-static int
+static ssize_t
 do_pwritev(
 	int		fd,
 	off64_t		offset,
-	ssize_t		count,
-	ssize_t		buffer_size,
+	size_t		count,
+	size_t		buffer_size,
I had not noticed this before, but this is odd; buffer_size is not used in this function. Instead, we test the global "buffersize"

Quoted 2 lines

I'm not sure what's going on there, need to read the code bit more. That's probably a separate fix though.

The buffer_size argument in do_pwritev/do_preadv is useless. I'll remove it in a separate patch. But due to this patch changed the 'buffer_size' place, I'll send this patch and 'remove buffer_size' patch together. To prevent merge conflict.

Thanks,
Zorro

Quoted 41 lines

I think the rest looks ok,
Reviewed-by Eric Sandeen <[email protected]>
Quoted 22 lines
io/pwrite.c +3 -3
 	int 		pwritev2_flags)
 {
 	int vecs = 0;
@@ -105,12 +105,12 @@ do_pwritev(
 #define do_pwritev(fd, offset, count, buffer_size) (0)
 #endif
 
-static int
+static ssize_t
 do_pwrite(
 	int		fd,
 	off64_t		offset,
-	ssize_t		count,
-	ssize_t		buffer_size,
+	size_t		count,
+	size_t		buffer_size,
 	int		pwritev2_flags)
 {
 	if (!vectors)
io/pwrite.c write_once( · line 275
Quoted 8 lines
io/pwrite.c +1 -1
@@ -269,7 +269,7 @@ write_once(
 	long long	*total,
 	int		pwritev2_flags)
 {
-	size_t bytes;
+	ssize_t bytes;
 	bytes = do_pwrite(file->fd, offset, count, count, pwritev2_flags);
 	if (bytes < 0)
 		return -1;
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html