[PATCH 1/3] xfsprogs: fix wrong variable type in write_once function

[PATCH 1/3] xfsprogs: fix wrong variable type in write_once function

[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);
>>>     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/pread
related functions.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---

Actrually, this's V3. V3 did more changes in io/pread.c, fix 'ssize_t'
problem in io/pread.c (V2 only did in io/pwrite.c).

Thanks,
Zorro

 io/pread.c  | 12 ++++++------
 io/pwrite.c | 14 +++++++-------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/io/pread.c b/io/pread.c
index 7591276d..60650aa3 100644
--- a/io/pread.c
+++ b/io/pread.c
@@ -172,12 +172,12 @@ dump_buffer(
 }
 
 #ifdef HAVE_PREADV
-static int
+static ssize_t
 do_preadv(
 	int		fd,
 	off64_t		offset,
-	ssize_t		count,
-	ssize_t		buffer_size)
+	size_t		count,
+	size_t		buffer_size)
 {
 	int		vecs = 0;
 	ssize_t		oldlen = 0;
@@ -208,12 +208,12 @@ do_preadv(
 #define do_preadv(fd, offset, count, buffer_size) (0)
 #endif
 
-static int
+static ssize_t
 do_pread(
 	int		fd,
 	off64_t		offset,
-	ssize_t		count,
-	ssize_t		buffer_size)
+	size_t		count,
+	size_t		buffer_size)
 {
 	if (!vectors)
 		return pread(fd, buffer, min(count, buffer_size), offset);
diff --git a/io/pwrite.c b/io/pwrite.c
index 26f79579..3df976a8 100644
--- a/io/pwrite.c
+++ b/io/pwrite.c
@@ -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;
-- 
2.13.6

[PATCH 2/3] xfsprogs: fix wrong do_pwritev definition

[Zorro Lang]

In io/pwrite.c, if not define HAVE_PWRITEV, we will use:
  #define do_pwritev(fd, offset, count, buffer_size) (0)

But the real do_pwritev() function is:
  do_pwritev(fd, offset, count, buffer_size, pwritev2_flags);

There's one more 'pwritev2_flags' argument.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 io/pwrite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io/pwrite.c b/io/pwrite.c
index 3df976a8..a89edfd0 100644
--- a/io/pwrite.c
+++ b/io/pwrite.c
@@ -102,7 +102,7 @@ do_pwritev(
 	return bytes;
 }
 #else
-#define do_pwritev(fd, offset, count, buffer_size) (0)
+#define do_pwritev(fd, offset, count, buffer_size, pwritev2_flags) (0)
 #endif
 
 static ssize_t
-- 
2.13.6

[PATCH 3/3] xfsprogs: remove useless do_preadv and do_pwritev arguments

[Zorro Lang]

do_preadv and do_pwritev all have a 'buffer_size' argument, but they
never used it. Instead of it, they use global 'buffersize' variable,
which is initialized in alloc_buffer(). As the 'buffer_size' is
useless, so remove it for clear code.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 io/pread.c  | 7 +++----
 io/pwrite.c | 5 ++---
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/io/pread.c b/io/pread.c
index 60650aa3..98e992b0 100644
--- a/io/pread.c
+++ b/io/pread.c
@@ -176,8 +176,7 @@ static ssize_t
 do_preadv(
 	int		fd,
 	off64_t		offset,
-	size_t		count,
-	size_t		buffer_size)
+	size_t		count)
 {
 	int		vecs = 0;
 	ssize_t		oldlen = 0;
@@ -205,7 +204,7 @@ do_preadv(
 	return bytes;
 }
 #else
-#define do_preadv(fd, offset, count, buffer_size) (0)
+#define do_preadv(fd, offset, count) (0)
 #endif
 
 static ssize_t
@@ -218,7 +217,7 @@ do_pread(
 	if (!vectors)
 		return pread(fd, buffer, min(count, buffer_size), offset);
 
-	return do_preadv(fd, offset, count, buffer_size);
+	return do_preadv(fd, offset, count);
 }
 
 static int
diff --git a/io/pwrite.c b/io/pwrite.c
index a89edfd0..f75c6164 100644
--- a/io/pwrite.c
+++ b/io/pwrite.c
@@ -66,7 +66,6 @@ do_pwritev(
 	int		fd,
 	off64_t		offset,
 	size_t		count,
-	size_t		buffer_size,
 	int 		pwritev2_flags)
 {
 	int vecs = 0;
@@ -102,7 +101,7 @@ do_pwritev(
 	return bytes;
 }
 #else
-#define do_pwritev(fd, offset, count, buffer_size, pwritev2_flags) (0)
+#define do_pwritev(fd, offset, count, pwritev2_flags) (0)
 #endif
 
 static ssize_t
@@ -116,7 +115,7 @@ do_pwrite(
 	if (!vectors)
 		return pwrite(fd, buffer, min(count, buffer_size), offset);
 
-	return do_pwritev(fd, offset, count, buffer_size, pwritev2_flags);
+	return do_pwritev(fd, offset, count, pwritev2_flags);
 }
 
 static int
-- 
2.13.6

Re: [PATCH 1/3] xfsprogs: fix wrong variable type in write_once function

[Eric Sandeen]

On 11/11/17 11:21 AM, Zorro Lang wrote:
> 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);
>>>>     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/pread
> related functions.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>

I may rewrite the changelog a bit because this now touches much
more than write_once, but the content seems fine:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
> 
> Actrually, this's V3. V3 did more changes in io/pread.c, fix 'ssize_t'
> problem in io/pread.c (V2 only did in io/pwrite.c).
> 
> Thanks,
> Zorro
> 
>  io/pread.c  | 12 ++++++------
>  io/pwrite.c | 14 +++++++-------
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/io/pread.c b/io/pread.c
> index 7591276d..60650aa3 100644
> --- a/io/pread.c
> +++ b/io/pread.c
> @@ -172,12 +172,12 @@ dump_buffer(
>  }
>  
>  #ifdef HAVE_PREADV
> -static int
> +static ssize_t
>  do_preadv(
>  	int		fd,
>  	off64_t		offset,
> -	ssize_t		count,
> -	ssize_t		buffer_size)
> +	size_t		count,
> +	size_t		buffer_size)
>  {
>  	int		vecs = 0;
>  	ssize_t		oldlen = 0;
> @@ -208,12 +208,12 @@ do_preadv(
>  #define do_preadv(fd, offset, count, buffer_size) (0)
>  #endif
>  
> -static int
> +static ssize_t
>  do_pread(
>  	int		fd,
>  	off64_t		offset,
> -	ssize_t		count,
> -	ssize_t		buffer_size)
> +	size_t		count,
> +	size_t		buffer_size)
>  {
>  	if (!vectors)
>  		return pread(fd, buffer, min(count, buffer_size), offset);
> diff --git a/io/pwrite.c b/io/pwrite.c
> index 26f79579..3df976a8 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -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;
> 

Re: [PATCH 2/3] xfsprogs: fix wrong do_pwritev definition

[Eric Sandeen]

On 11/11/17 11:21 AM, Zorro Lang wrote:
> In io/pwrite.c, if not define HAVE_PWRITEV, we will use:
>   #define do_pwritev(fd, offset, count, buffer_size) (0)
> 
> But the real do_pwritev() function is:
>   do_pwritev(fd, offset, count, buffer_size, pwritev2_flags);
> 
> There's one more 'pwritev2_flags' argument.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  io/pwrite.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/io/pwrite.c b/io/pwrite.c
> index 3df976a8..a89edfd0 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -102,7 +102,7 @@ do_pwritev(
>  	return bytes;
>  }
>  #else
> -#define do_pwritev(fd, offset, count, buffer_size) (0)
> +#define do_pwritev(fd, offset, count, buffer_size, pwritev2_flags) (0)
>  #endif
>  
>  static ssize_t
> 

Re: [PATCH 3/3] xfsprogs: remove useless do_preadv and do_pwritev arguments

[Eric Sandeen]

On 11/11/17 11:21 AM, Zorro Lang wrote:
> do_preadv and do_pwritev all have a 'buffer_size' argument, but they
> never used it. Instead of it, they use global 'buffersize' variable,
> which is initialized in alloc_buffer(). As the 'buffer_size' is
> useless, so remove it for clear code.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>

I'm going to hold off on this one for 4.14; I want to think a bit about
why we have this global in the first place, and if it makes more sense
to eliminate the global and pass the buffer size around properly.

Thanks,
-Eric

> ---
>  io/pread.c  | 7 +++----
>  io/pwrite.c | 5 ++---
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/io/pread.c b/io/pread.c
> index 60650aa3..98e992b0 100644
> --- a/io/pread.c
> +++ b/io/pread.c
> @@ -176,8 +176,7 @@ static ssize_t
>  do_preadv(
>  	int		fd,
>  	off64_t		offset,
> -	size_t		count,
> -	size_t		buffer_size)
> +	size_t		count)
>  {
>  	int		vecs = 0;
>  	ssize_t		oldlen = 0;
> @@ -205,7 +204,7 @@ do_preadv(
>  	return bytes;
>  }
>  #else
> -#define do_preadv(fd, offset, count, buffer_size) (0)
> +#define do_preadv(fd, offset, count) (0)
>  #endif
>  
>  static ssize_t
> @@ -218,7 +217,7 @@ do_pread(
>  	if (!vectors)
>  		return pread(fd, buffer, min(count, buffer_size), offset);
>  
> -	return do_preadv(fd, offset, count, buffer_size);
> +	return do_preadv(fd, offset, count);
>  }
>  
>  static int
> diff --git a/io/pwrite.c b/io/pwrite.c
> index a89edfd0..f75c6164 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -66,7 +66,6 @@ do_pwritev(
>  	int		fd,
>  	off64_t		offset,
>  	size_t		count,
> -	size_t		buffer_size,
>  	int 		pwritev2_flags)
>  {
>  	int vecs = 0;
> @@ -102,7 +101,7 @@ do_pwritev(
>  	return bytes;
>  }
>  #else
> -#define do_pwritev(fd, offset, count, buffer_size, pwritev2_flags) (0)
> +#define do_pwritev(fd, offset, count, pwritev2_flags) (0)
>  #endif
>  
>  static ssize_t
> @@ -116,7 +115,7 @@ do_pwrite(
>  	if (!vectors)
>  		return pwrite(fd, buffer, min(count, buffer_size), offset);
>  
> -	return do_pwritev(fd, offset, count, buffer_size, pwritev2_flags);
> +	return do_pwritev(fd, offset, count, pwritev2_flags);
>  }
>  
>  static int
> 

Re: [PATCH 3/3] xfsprogs: remove useless do_preadv and do_pwritev arguments

[Eric Sandeen]

On 11/11/17 11:21 AM, Zorro Lang wrote:
> do_preadv and do_pwritev all have a 'buffer_size' argument, but they
> never used it. Instead of it, they use global 'buffersize' variable,
> which is initialized in alloc_buffer(). As the 'buffer_size' is
> useless, so remove it for clear code.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>

Hi Zorro, going through old patches and remembered that I never came
back to this one, sorry.

I think that just removing it is ok.  do_preadv & do_pwritev
both use iov and buffersize, each is a global variable; there is no
need to pass in buffer size any more than there is a need to
pass in the iov itself.  I'll go ahead & merge this as is.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks,
-Eric

> ---
>  io/pread.c  | 7 +++----
>  io/pwrite.c | 5 ++---
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/io/pread.c b/io/pread.c
> index 60650aa3..98e992b0 100644
> --- a/io/pread.c
> +++ b/io/pread.c
> @@ -176,8 +176,7 @@ static ssize_t
>  do_preadv(
>  	int		fd,
>  	off64_t		offset,
> -	size_t		count,
> -	size_t		buffer_size)
> +	size_t		count)
>  {
>  	int		vecs = 0;
>  	ssize_t		oldlen = 0;
> @@ -205,7 +204,7 @@ do_preadv(
>  	return bytes;
>  }
>  #else
> -#define do_preadv(fd, offset, count, buffer_size) (0)
> +#define do_preadv(fd, offset, count) (0)
>  #endif
>  
>  static ssize_t
> @@ -218,7 +217,7 @@ do_pread(
>  	if (!vectors)
>  		return pread(fd, buffer, min(count, buffer_size), offset);
>  
> -	return do_preadv(fd, offset, count, buffer_size);
> +	return do_preadv(fd, offset, count);
>  }
>  
>  static int
> diff --git a/io/pwrite.c b/io/pwrite.c
> index a89edfd0..f75c6164 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -66,7 +66,6 @@ do_pwritev(
>  	int		fd,
>  	off64_t		offset,
>  	size_t		count,
> -	size_t		buffer_size,
>  	int 		pwritev2_flags)
>  {
>  	int vecs = 0;
> @@ -102,7 +101,7 @@ do_pwritev(
>  	return bytes;
>  }
>  #else
> -#define do_pwritev(fd, offset, count, buffer_size, pwritev2_flags) (0)
> +#define do_pwritev(fd, offset, count, pwritev2_flags) (0)
>  #endif
>  
>  static ssize_t
> @@ -116,7 +115,7 @@ do_pwrite(
>  	if (!vectors)
>  		return pwrite(fd, buffer, min(count, buffer_size), offset);
>  
> -	return do_pwritev(fd, offset, count, buffer_size, pwritev2_flags);
> +	return do_pwritev(fd, offset, count, pwritev2_flags);
>  }
>  
>  static int
> 

Re: [PATCH 3/3] xfsprogs: remove useless do_preadv and do_pwritev arguments

[Zorro Lang]

On Thu, Sep 27, 2018 at 01:38:30PM -0500, Eric Sandeen wrote:
> On 11/11/17 11:21 AM, Zorro Lang wrote:
> > do_preadv and do_pwritev all have a 'buffer_size' argument, but they
> > never used it. Instead of it, they use global 'buffersize' variable,
> > which is initialized in alloc_buffer(). As the 'buffer_size' is
> > useless, so remove it for clear code.
> > 
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> 
> Hi Zorro, going through old patches and remembered that I never came
> back to this one, sorry.
> 
> I think that just removing it is ok.  do_preadv & do_pwritev
> both use iov and buffersize, each is a global variable; there is no
> need to pass in buffer size any more than there is a need to
> pass in the iov itself.  I'll go ahead & merge this as is.

Hi Eric,

Thanks so much, I already forgot this patch, I don't know if it can
be merged directly now, hope there's not conflict :-P

Feel free to tell me if you need a new version.

Thanks,
Zorro

> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> Thanks,
> -Eric
> 
> > ---
> >  io/pread.c  | 7 +++----
> >  io/pwrite.c | 5 ++---
> >  2 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/io/pread.c b/io/pread.c
> > index 60650aa3..98e992b0 100644
> > --- a/io/pread.c
> > +++ b/io/pread.c
> > @@ -176,8 +176,7 @@ static ssize_t
> >  do_preadv(
> >  	int		fd,
> >  	off64_t		offset,
> > -	size_t		count,
> > -	size_t		buffer_size)
> > +	size_t		count)
> >  {
> >  	int		vecs = 0;
> >  	ssize_t		oldlen = 0;
> > @@ -205,7 +204,7 @@ do_preadv(
> >  	return bytes;
> >  }
> >  #else
> > -#define do_preadv(fd, offset, count, buffer_size) (0)
> > +#define do_preadv(fd, offset, count) (0)
> >  #endif
> >  
> >  static ssize_t
> > @@ -218,7 +217,7 @@ do_pread(
> >  	if (!vectors)
> >  		return pread(fd, buffer, min(count, buffer_size), offset);
> >  
> > -	return do_preadv(fd, offset, count, buffer_size);
> > +	return do_preadv(fd, offset, count);
> >  }
> >  
> >  static int
> > diff --git a/io/pwrite.c b/io/pwrite.c
> > index a89edfd0..f75c6164 100644
> > --- a/io/pwrite.c
> > +++ b/io/pwrite.c
> > @@ -66,7 +66,6 @@ do_pwritev(
> >  	int		fd,
> >  	off64_t		offset,
> >  	size_t		count,
> > -	size_t		buffer_size,
> >  	int 		pwritev2_flags)
> >  {
> >  	int vecs = 0;
> > @@ -102,7 +101,7 @@ do_pwritev(
> >  	return bytes;
> >  }
> >  #else
> > -#define do_pwritev(fd, offset, count, buffer_size, pwritev2_flags) (0)
> > +#define do_pwritev(fd, offset, count, pwritev2_flags) (0)
> >  #endif
> >  
> >  static ssize_t
> > @@ -116,7 +115,7 @@ do_pwrite(
> >  	if (!vectors)
> >  		return pwrite(fd, buffer, min(count, buffer_size), offset);
> >  
> > -	return do_pwritev(fd, offset, count, buffer_size, pwritev2_flags);
> > +	return do_pwritev(fd, offset, count, pwritev2_flags);
> >  }
> >  
> >  static int
> >