[PATCH] fs/direct-io.c: Fix compilation warning for uninitialized variables

[PATCH] fs/direct-io.c: Fix compilation warning for uninitialized variables

[pramod.gurav.etc]

From: Pramod Gurav <pramod.gurav.etc@gmail.com>

Fixes below warning while compiling the kernel.

fs/direct-io.c: In function ‘__blockdev_direct_IO’:
fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:16: note: ‘to’ was declared here
fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:10: note: ‘from’ was declared here

Signed-off-by: Pramod Gurav <pramod.gurav.etc@gmail.com>

CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 fs/direct-io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..6ad5d2c 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -910,7 +910,7 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
 
 	while (sdio->block_in_file < sdio->final_block_in_request) {
 		struct page *page;
-		size_t from, to;
+		size_t from = 0, to = 0;
 		page = dio_get_page(dio, sdio, &from, &to);
 		if (IS_ERR(page)) {
 			ret = PTR_ERR(page);
-- 
1.7.9.5

Re: [PATCH] fs/direct-io.c: Fix compilation warning for uninitialized variables

[Christoph Hellwig]

Looks good to me,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Al, can you pick this up?  It's the only warnings in many of my usual
kernel builds at the moment.

Re: [PATCH] fs/direct-io.c: Fix compilation warning for uninitialized variables

[pramod gurav]

Thanks Christoph for the review. :)

On Sun, Jul 13, 2014 at 5:20 PM, Christoph Hellwig <hch@infradead.org> wrote:
> Looks good to me,
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Al, can you pick this up?  It's the only warnings in many of my usual
> kernel builds at the moment.
>



-- 
Thanks and Regards
Pramod

Re: [PATCH] fs/direct-io.c: Fix compilation warning for uninitialized variables

[Boaz Harrosh]

On 07/13/2014 02:50 PM, Christoph Hellwig wrote:
> pramod.gurav.etc@gmail.com wrote ...
>> diff --git a/fs/direct-io.c b/fs/direct-io.c
>> index 98040ba388ac..c0a9854d2bc7 100644
>> --- a/fs/direct-io.c
>> +++ b/fs/direct-io.c
>> @@ -910,7 +910,8 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
>>  
>>  	while (sdio->block_in_file < sdio->final_block_in_request) {
>>  		struct page *page;
>> -		size_t from, to;
>> +		size_t from = 0;
>> +		size_t to = 0;
>>  		page = dio_get_page(dio, sdio, &from, &to);
>>  		if (IS_ERR(page)) {
>>  			ret = PTR_ERR(page);
> Looks good to me,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Al, can you pick this up?  It's the only warnings in many of my usual
> kernel builds at the moment.
> 
<>

Paul Bolle <pebolle@tiscali.nl> wrote ...
<>
> @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
>  	while (sdio->block_in_file < sdio->final_block_in_request) {
>  		struct page *page;
>  		size_t from, to;
> -		page = dio_get_page(dio, sdio, &from, &to);
> +
> +		page = dio_get_page(dio, sdio);
>  		if (IS_ERR(page)) {
>  			ret = PTR_ERR(page);
>  			goto out;
>  		}
> +		from = sdio->head ? 0 : sdio->from;
> +		to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> +		sdio->head++;
>  
>  		while (from < to) {
>  			unsigned this_chunk_bytes;	/* # of bytes mapped */
<>

This is the wrong fix. GCC is wrong here. As shown by Paul Bolle if
you move the from / to set from dio_get_page() to here the warning goes away.

The minimal fix must use uninitialized_var() in this case. See patch below

But I think the proper fix Is the one Paul Bolle <pebolle@tiscali.nl> sent (above)


----
From: Boaz Harrosh <boaz@plexistor.com>
Date: Wed, 16 Jul 2014 20:02:29 +0300
Subject: [PATCH] do_direct_IO: Fix compiler warning
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This fixes
../fs/direct-io.c: In function ‘do_blockdev_direct_IO’:
../fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    u = (to - from) >> blkbits;
            ^
../fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    u = (to - from) >> blkbits;

I use: gcc version 4.8.3 20140624 (Red Hat 4.8.3-1)

GCC is wrong here so we should use the uninitialized_var() macro
and not silence it with = 0;

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 fs/direct-io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..156e6f0 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -910,7 +910,8 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
 
 	while (sdio->block_in_file < sdio->final_block_in_request) {
 		struct page *page;
-		size_t from, to;
+		size_t uninitialized_var(from), uninitialized_var(to);
+
 		page = dio_get_page(dio, sdio, &from, &to);
 		if (IS_ERR(page)) {
 			ret = PTR_ERR(page);
-- 
1.9.3

Re: [PATCH] fs/direct-io.c: Fix compilation warning for uninitialized variables

[Jason Cooper]

On Wed, Jul 16, 2014 at 08:08:41PM +0300, Boaz Harrosh wrote:
> On 07/13/2014 02:50 PM, Christoph Hellwig wrote:
> > pramod.gurav.etc@gmail.com wrote ...
> >> diff --git a/fs/direct-io.c b/fs/direct-io.c
> >> index 98040ba388ac..c0a9854d2bc7 100644
> >> --- a/fs/direct-io.c
> >> +++ b/fs/direct-io.c
> >> @@ -910,7 +910,8 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
> >>  
> >>  	while (sdio->block_in_file < sdio->final_block_in_request) {
> >>  		struct page *page;
> >> -		size_t from, to;
> >> +		size_t from = 0;
> >> +		size_t to = 0;
> >>  		page = dio_get_page(dio, sdio, &from, &to);
> >>  		if (IS_ERR(page)) {
> >>  			ret = PTR_ERR(page);
> > Looks good to me,
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> > Al, can you pick this up?  It's the only warnings in many of my usual
> > kernel builds at the moment.
> > 
> <>
> 
> Paul Bolle <pebolle@tiscali.nl> wrote ...
> <>
> > @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
> >  	while (sdio->block_in_file < sdio->final_block_in_request) {
> >  		struct page *page;
> >  		size_t from, to;
> > -		page = dio_get_page(dio, sdio, &from, &to);
> > +
> > +		page = dio_get_page(dio, sdio);
> >  		if (IS_ERR(page)) {
> >  			ret = PTR_ERR(page);
> >  			goto out;
> >  		}
> > +		from = sdio->head ? 0 : sdio->from;
> > +		to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> > +		sdio->head++;
> >  
> >  		while (from < to) {
> >  			unsigned this_chunk_bytes;	/* # of bytes mapped */
> <>
> 
> This is the wrong fix. GCC is wrong here. As shown by Paul Bolle if
> you move the from / to set from dio_get_page() to here the warning goes away.

Actually, that was me.

> The minimal fix must use uninitialized_var() in this case. See patch below

I didn't know about uninitialized_var() when I wrote the patch.  As the
comment for it states:

/*
 * A trick to suppress uninitialized variable warning without generating
 * any code
 */


So,

> But I think the proper fix Is the one Paul Bolle <pebolle@tiscali.nl> sent (above)
> 
> 
> ----
> From: Boaz Harrosh <boaz@plexistor.com>
> Date: Wed, 16 Jul 2014 20:02:29 +0300
> Subject: [PATCH] do_direct_IO: Fix compiler warning
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> This fixes
> ../fs/direct-io.c: In function ‘do_blockdev_direct_IO’:
> ../fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     u = (to - from) >> blkbits;
>             ^
> ../fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     u = (to - from) >> blkbits;
> 
> I use: gcc version 4.8.3 20140624 (Red Hat 4.8.3-1)
> 
> GCC is wrong here so we should use the uninitialized_var() macro
> and not silence it with = 0;
> 
> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

> ---
>  fs/direct-io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 98040ba..156e6f0 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -910,7 +910,8 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
>  
>  	while (sdio->block_in_file < sdio->final_block_in_request) {
>  		struct page *page;
> -		size_t from, to;
> +		size_t uninitialized_var(from), uninitialized_var(to);
> +
>  		page = dio_get_page(dio, sdio, &from, &to);
>  		if (IS_ERR(page)) {
>  			ret = PTR_ERR(page);
> -- 
> 1.9.3
> 

Re: [PATCH] fs/direct-io.c: Fix compilation warning for uninitialized variables

[Christoph Hellwig]

On Wed, Jul 16, 2014 at 08:08:41PM +0300, Boaz Harrosh wrote:
> This is the wrong fix. GCC is wrong here. As shown by Paul Bolle if
> you move the from / to set from dio_get_page() to here the warning goes away.
> 
> The minimal fix must use uninitialized_var() in this case. See patch below
> 
> But I think the proper fix Is the one Paul Bolle <pebolle@tiscali.nl> sent (above)

I don't think the initialization is wrong.  The fix of moving the code
defintively looks nicer, while I think uninitialized_var is horrible
wart that won't get anywhere near my code.

Either way we should merge one of those fixes ASAP..

Re: [PATCH] fs/direct-io.c: Fix compilation warning for uninitialized variables

[Paul Bolle]

[Added Borislav and Sam to cc.]

Christoph Hellwig schreef op wo 16-07-2014 om 10:58 [-0700]:
> On Wed, Jul 16, 2014 at 08:08:41PM +0300, Boaz Harrosh wrote:
> > This is the wrong fix. GCC is wrong here. As shown by Paul Bolle if
> > you move the from / to set from dio_get_page() to here the warning goes away.
> > 
> > The minimal fix must use uninitialized_var() in this case. See patch below
> > 
> > But I think the proper fix Is the one Paul Bolle <pebolle@tiscali.nl> sent (above)
> 
> I don't think the initialization is wrong.  The fix of moving the code
> defintively looks nicer,

Whether that was Jason's idea or mine doesn't matter much - though I do
think Boaz quoted part of my fix, but it was just a _draft_.

Anyhow, after all that I got involved in a short thread;
https://lkml.org/lkml/2014/7/8/168 is my contribution. My point is,
basically, that the false positives of -Wmaybe-uninitialized are a "code
smell". They tend to disappear if one reorganizes the code a bit.
Borislav disagrees quite strongly. I didn't really bother with keeping
that thread alive because I feared we'd mainly see more colorful
language.

>  while I think uninitialized_var is horrible wart

Agree totally.

>  that won't get anywhere near my code.
>
> Either way we should merge one of those fixes ASAP..

I'd like my builds to be warning free too. And it would be nice to know
whether there's consensus on how to deal with the false positives of
-Wmaybe-uninitialized that make it into mainline.


Paul Bolle

direct-io: squelch maybe-uninitialized warning in do_direct_IO()

[Boaz Harrosh]

From: Paul Bolle <pebolle@tiscali.nl>

The following warnings:

  fs/direct-io.c: In function ‘__blockdev_direct_IO’:
  fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  fs/direct-io.c:913:16: note: ‘to’ was declared here
  fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  fs/direct-io.c:913:10: note: ‘from’ was declared here

are false positive because dio_get_page() either fails, or sets both
'from' and 'to'.

Maybe it's better to move initializing "to" and "from" out of
dio_get_page(). That _might_ make it easier for both the the reader and
the compiler to understand what's going on. Something like this:

Christoph Hellwig said ...
The fix of moving the code defintively looks nicer, while I think
uninitialized_var is horrible wart that won't get anywhere near my code.

Boaz Harrosh I agree with Christoph and Paul

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 fs/direct-io.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..2f024fc 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
  * L1 cache.
  */
 static inline struct page *dio_get_page(struct dio *dio,
-		struct dio_submit *sdio, size_t *from, size_t *to)
+		struct dio_submit *sdio)
 {
-	int n;
 	if (dio_pages_present(sdio) == 0) {
 		int ret;
 
@@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
 			return ERR_PTR(ret);
 		BUG_ON(dio_pages_present(sdio) == 0);
 	}
-	n = sdio->head++;
-	*from = n ? 0 : sdio->from;
-	*to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
-	return dio->pages[n];
+	return dio->pages[sdio->head];
 }
 
 /**
@@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
 	while (sdio->block_in_file < sdio->final_block_in_request) {
 		struct page *page;
 		size_t from, to;
-		page = dio_get_page(dio, sdio, &from, &to);
+
+		page = dio_get_page(dio, sdio);
 		if (IS_ERR(page)) {
 			ret = PTR_ERR(page);
 			goto out;
 		}
+		from = sdio->head ? 0 : sdio->from;
+		to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
+		sdio->head++;
 
 		while (from < to) {
 			unsigned this_chunk_bytes;	/* # of bytes mapped */
-- 
1.9.3

Re: direct-io: squelch maybe-uninitialized warning in do_direct_IO()

[Boaz Harrosh]

On 07/17/2014 12:37 PM, Boaz Harrosh wrote:
> From: Paul Bolle <pebolle@tiscali.nl>
> 
> The following warnings:
> 
>   fs/direct-io.c: In function ‘__blockdev_direct_IO’:
>   fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   fs/direct-io.c:913:16: note: ‘to’ was declared here
>   fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   fs/direct-io.c:913:10: note: ‘from’ was declared here
> 
> are false positive because dio_get_page() either fails, or sets both
> 'from' and 'to'.
> 
> Maybe it's better to move initializing "to" and "from" out of
> dio_get_page(). That _might_ make it easier for both the the reader and
> the compiler to understand what's going on. Something like this:
> 
> Christoph Hellwig said ...
> The fix of moving the code defintively looks nicer, while I think
> uninitialized_var is horrible wart that won't get anywhere near my code.
> 
> Boaz Harrosh I agree with Christoph and Paul
> 

So Here, everyone likes this one (I think) please ACK/Review and let us merge
it.

> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>

Paul this is your patch I put you as Signed-off-by please acknowledge

> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>


Thanks
Boaz

> ---
>  fs/direct-io.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 98040ba..2f024fc 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
>   * L1 cache.
>   */
>  static inline struct page *dio_get_page(struct dio *dio,
> -		struct dio_submit *sdio, size_t *from, size_t *to)
> +		struct dio_submit *sdio)
>  {
> -	int n;
>  	if (dio_pages_present(sdio) == 0) {
>  		int ret;
>  
> @@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
>  			return ERR_PTR(ret);
>  		BUG_ON(dio_pages_present(sdio) == 0);
>  	}
> -	n = sdio->head++;
> -	*from = n ? 0 : sdio->from;
> -	*to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> -	return dio->pages[n];
> +	return dio->pages[sdio->head];
>  }
>  
>  /**
> @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
>  	while (sdio->block_in_file < sdio->final_block_in_request) {
>  		struct page *page;
>  		size_t from, to;
> -		page = dio_get_page(dio, sdio, &from, &to);
> +
> +		page = dio_get_page(dio, sdio);
>  		if (IS_ERR(page)) {
>  			ret = PTR_ERR(page);
>  			goto out;
>  		}
> +		from = sdio->head ? 0 : sdio->from;
> +		to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> +		sdio->head++;
>  
>  		while (from < to) {
>  			unsigned this_chunk_bytes;	/* # of bytes mapped */
> 

Re: direct-io: squelch maybe-uninitialized warning in do_direct_IO()

[Paul Bolle]

Boaz,

On Thu, 2014-07-17 at 12:37 +0300, Boaz Harrosh wrote:
> From: Paul Bolle <pebolle@tiscali.nl>
> 
> The following warnings:
> 
>   fs/direct-io.c: In function ‘__blockdev_direct_IO’:
>   fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   fs/direct-io.c:913:16: note: ‘to’ was declared here
>   fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   fs/direct-io.c:913:10: note: ‘from’ was declared here
> 
> are false positive because dio_get_page() either fails, or sets both
> 'from' and 'to'.
> 
> Maybe it's better to move initializing "to" and "from" out of
> dio_get_page(). That _might_ make it easier for both the the reader and
> the compiler to understand what's going on. Something like this:
> 
> Christoph Hellwig said ...
> The fix of moving the code defintively looks nicer, while I think
> uninitialized_var is horrible wart that won't get anywhere near my code.
> 
> Boaz Harrosh I agree with Christoph and Paul
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>

This is simply not coorect!

> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
> ---
>  fs/direct-io.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 98040ba..2f024fc 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
>   * L1 cache.
>   */
>  static inline struct page *dio_get_page(struct dio *dio,
> -		struct dio_submit *sdio, size_t *from, size_t *to)
> +		struct dio_submit *sdio)
>  {
> -	int n;
>  	if (dio_pages_present(sdio) == 0) {
>  		int ret;
>  
> @@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
>  			return ERR_PTR(ret);
>  		BUG_ON(dio_pages_present(sdio) == 0);
>  	}
> -	n = sdio->head++;
> -	*from = n ? 0 : sdio->from;
> -	*to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> -	return dio->pages[n];
> +	return dio->pages[sdio->head];
>  }
>  
>  /**
> @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
>  	while (sdio->block_in_file < sdio->final_block_in_request) {
>  		struct page *page;
>  		size_t from, to;
> -		page = dio_get_page(dio, sdio, &from, &to);
> +
> +		page = dio_get_page(dio, sdio);
>  		if (IS_ERR(page)) {
>  			ret = PTR_ERR(page);
>  			goto out;
>  		}
> +		from = sdio->head ? 0 : sdio->from;
> +		to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> +		sdio->head++;
>  
>  		while (from < to) {
>  			unsigned this_chunk_bytes;	/* # of bytes mapped */


Paul Bolle

Re: direct-io: squelch maybe-uninitialized warning in do_direct_IO()

[Paul Bolle]

Boaz,

On Thu, 2014-07-17 at 12:40 +0300, Boaz Harrosh wrote:
> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> 
> Paul this is your patch I put you as Signed-off-by please acknowledge

I never signed off on that patch! Speaking from memory, I just sent a
message stating that "something like this" might silence the warning. I
also stated that I needed to have another look at it.

This touches code I did not write and do not yet understand well enough.
Getting there requires a clean schedule, clean head, and/or a clean
desk. I certainly won't sign off on it until one or more of those
conditions are met.

Please don't do this (adding a Signed-off-by on a patch I drafted)
again!


Paul Bolle

Re: direct-io: squelch maybe-uninitialized warning in do_direct_IO()

[Boaz Harrosh]

On 07/17/2014 12:48 PM, Paul Bolle wrote:
> Boaz,
> 
> On Thu, 2014-07-17 at 12:37 +0300, Boaz Harrosh wrote:
>> From: Paul Bolle <pebolle@tiscali.nl>
>>
>> The following warnings:
>>
>>   fs/direct-io.c: In function ‘__blockdev_direct_IO’:
>>   fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>   fs/direct-io.c:913:16: note: ‘to’ was declared here
>>   fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>   fs/direct-io.c:913:10: note: ‘from’ was declared here
>>
>> are false positive because dio_get_page() either fails, or sets both
>> 'from' and 'to'.
>>
>> Maybe it's better to move initializing "to" and "from" out of
>> dio_get_page(). That _might_ make it easier for both the the reader and
>> the compiler to understand what's going on. Something like this:
>>
>> Christoph Hellwig said ...
>> The fix of moving the code defintively looks nicer, while I think
>> uninitialized_var is horrible wart that won't get anywhere near my code.
>>
>> Boaz Harrosh I agree with Christoph and Paul
>>
>> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> 
> This is simply not coorect!

Sorry you are absolutely right, I only saw this after "send", is why
I ping you to make sure.

I have reviewed it fully, your initial code is correct *and tested).
Here is a new V2 patch without your sign-off

Thanks
Boaz

> 
>> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
>> ---
>>  fs/direct-io.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/direct-io.c b/fs/direct-io.c
>> index 98040ba..2f024fc 100644
>> --- a/fs/direct-io.c
>> +++ b/fs/direct-io.c
>> @@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
>>   * L1 cache.
>>   */
>>  static inline struct page *dio_get_page(struct dio *dio,
>> -		struct dio_submit *sdio, size_t *from, size_t *to)
>> +		struct dio_submit *sdio)
>>  {
>> -	int n;
>>  	if (dio_pages_present(sdio) == 0) {
>>  		int ret;
>>  
>> @@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
>>  			return ERR_PTR(ret);
>>  		BUG_ON(dio_pages_present(sdio) == 0);
>>  	}
>> -	n = sdio->head++;
>> -	*from = n ? 0 : sdio->from;
>> -	*to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
>> -	return dio->pages[n];
>> +	return dio->pages[sdio->head];
>>  }
>>  
>>  /**
>> @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
>>  	while (sdio->block_in_file < sdio->final_block_in_request) {
>>  		struct page *page;
>>  		size_t from, to;
>> -		page = dio_get_page(dio, sdio, &from, &to);
>> +
>> +		page = dio_get_page(dio, sdio);
>>  		if (IS_ERR(page)) {
>>  			ret = PTR_ERR(page);
>>  			goto out;
>>  		}
>> +		from = sdio->head ? 0 : sdio->from;
>> +		to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
>> +		sdio->head++;
>>  
>>  		while (from < to) {
>>  			unsigned this_chunk_bytes;	/* # of bytes mapped */
> 
> 
> Paul Bolle
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v2] direct-io: squelch maybe-uninitialized warning in do_direct_IO()

[Boaz Harrosh]

From: Paul Bolle <pebolle@tiscali.nl>

The following warnings:

  fs/direct-io.c: In function ‘__blockdev_direct_IO’:
  fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  fs/direct-io.c:913:16: note: ‘to’ was declared here
  fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  fs/direct-io.c:913:10: note: ‘from’ was declared here

are false positive because dio_get_page() either fails, or sets both
'from' and 'to'.

Maybe it's better to move initializing "to" and "from" out of
dio_get_page(). That _might_ make it easier for both the the reader and
the compiler to understand what's going on. Something like this:

Christoph Hellwig said ...
The fix of moving the code definitively looks nicer, while I think
uninitialized_var is horrible wart that won't get anywhere near my code.

Boaz Harrosh: I agree with Christoph and Paul

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 fs/direct-io.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..194d0d1 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
  * L1 cache.
  */
 static inline struct page *dio_get_page(struct dio *dio,
-		struct dio_submit *sdio, size_t *from, size_t *to)
+					struct dio_submit *sdio)
 {
-	int n;
 	if (dio_pages_present(sdio) == 0) {
 		int ret;
 
@@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
 			return ERR_PTR(ret);
 		BUG_ON(dio_pages_present(sdio) == 0);
 	}
-	n = sdio->head++;
-	*from = n ? 0 : sdio->from;
-	*to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
-	return dio->pages[n];
+	return dio->pages[sdio->head];
 }
 
 /**
@@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
 	while (sdio->block_in_file < sdio->final_block_in_request) {
 		struct page *page;
 		size_t from, to;
-		page = dio_get_page(dio, sdio, &from, &to);
+
+		page = dio_get_page(dio, sdio);
 		if (IS_ERR(page)) {
 			ret = PTR_ERR(page);
 			goto out;
 		}
+		from = sdio->head ? 0 : sdio->from;
+		to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
+		sdio->head++;
 
 		while (from < to) {
 			unsigned this_chunk_bytes;	/* # of bytes mapped */
-- 
1.9.3

Re: [PATCH v2] direct-io: squelch maybe-uninitialized warning in do_direct_IO()

[Paul Bolle]

Boaz Harrosh schreef op do 17-07-2014 om 14:11 [+0300]:
> From: Paul Bolle <pebolle@tiscali.nl>

Thanks for dropping my Signed-off-by tag.

Would you mind also dropping the From: line? I prefer you to also be
author of this patch.

Note that I'm pretty sure I can't claim copyright on the three chunks of
changes to the code nor on the few lines you copied into the commit
explanation. Besides, I'm perfectly OK with you using them in your patch
(especially since you've done review etc.).


Paul Bolle

[PATCH v3] direct-io: fix uninitialized warning in do_direct_IO()

[Boaz Harrosh]

From: Boaz Harrosh <boaz@plexistor.com>

The following warnings:

  fs/direct-io.c: In function ‘__blockdev_direct_IO’:
  fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  fs/direct-io.c:913:16: note: ‘to’ was declared here
  fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  fs/direct-io.c:913:10: note: ‘from’ was declared here

are false positive because dio_get_page() either fails, or sets both
'from' and 'to'.

Paul Bolle said ...
Maybe it's better to move initializing "to" and "from" out of
dio_get_page(). That _might_ make it easier for both the the reader and
the compiler to understand what's going on. Something like this:

Christoph Hellwig said ...
The fix of moving the code definitively looks nicer, while I think
uninitialized_var is horrible wart that won't get anywhere near my code.

Boaz Harrosh: I agree with Christoph and Paul

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 fs/direct-io.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..194d0d1 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
  * L1 cache.
  */
 static inline struct page *dio_get_page(struct dio *dio,
-		struct dio_submit *sdio, size_t *from, size_t *to)
+					struct dio_submit *sdio)
 {
-	int n;
 	if (dio_pages_present(sdio) == 0) {
 		int ret;
 
@@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
 			return ERR_PTR(ret);
 		BUG_ON(dio_pages_present(sdio) == 0);
 	}
-	n = sdio->head++;
-	*from = n ? 0 : sdio->from;
-	*to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
-	return dio->pages[n];
+	return dio->pages[sdio->head];
 }
 
 /**
@@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
 	while (sdio->block_in_file < sdio->final_block_in_request) {
 		struct page *page;
 		size_t from, to;
-		page = dio_get_page(dio, sdio, &from, &to);
+
+		page = dio_get_page(dio, sdio);
 		if (IS_ERR(page)) {
 			ret = PTR_ERR(page);
 			goto out;
 		}
+		from = sdio->head ? 0 : sdio->from;
+		to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
+		sdio->head++;
 
 		while (from < to) {
 			unsigned this_chunk_bytes;	/* # of bytes mapped */
-- 
1.9.3

Re: [PATCH v3] direct-io: fix uninitialized warning in do_direct_IO()

[Christoph Hellwig]

Looks good to me,

Reviewed-by: Christoph Hellwig <hch@lst.de>

While it looks obvious, did you make sure it passes xfstests, which
has a lot of direct I/O tests?

Re: [PATCH v3] direct-io: fix uninitialized warning in do_direct_IO()

[Boaz Harrosh]

On 07/21/2014 02:36 PM, Christoph Hellwig wrote:
> Looks good to me,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> While it looks obvious, did you make sure it passes xfstests, which
> has a lot of direct I/O tests?
> 

Thank you Christoph. OK So finally I did last night. I ran
ext4. Just that I'm not used to run ext4 or any of those
stuff so it took me time. I have the usual 2 failures I get
also from 3.15. So I'd say its good - tested

Thanks
Boaz