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

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

[Jason Cooper]

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 not necessary because dio_get_page() either fails, or sets both
'from' and 'to'.

Make the compiler happy so we can more easily detect legitimate
warnings.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 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 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);
-- 
2.0.0

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

[Markus Mayer]

I did a bit of digging on this and I am wondering if the initialization
of "to" and "from" to 0 should instead be done in dio_get_page().

The warning is caused by the return in dio_get_page():

		ret = dio_refill_pages(dio, sdio);
		if (ret)
			return ERR_PTR(ret);

This code path would leave "to" and "from" uninitialized (even though the
caller currently never uses the variables in that case).

If both variables are initialized to 0 in dio_get_page(), it would
guarantee that any future callers of dio_get_page() also get to take
advantage of the initialization. If, however, the variables are only
initialized in do_direct_IO(), other callers would have to take care of
this themselves (or the same warnings will pop up there).

There aren't currently any other callers to dio_get_page(), so the
choice is probably not a terribly critical one right now, but I wanted
to at least point this out.

Regards,
-Markus

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

[Paul Bolle]

On Mon, 2014-06-23 at 12:54 -0700, Markus Mayer wrote:
> I did a bit of digging on this and I am wondering if the initialization
> of "to" and "from" to 0 should instead be done in dio_get_page().
> 
> The warning is caused by the return in dio_get_page():
> 
> 		ret = dio_refill_pages(dio, sdio);
> 		if (ret)
> 			return ERR_PTR(ret);
> 
> This code path would leave "to" and "from" uninitialized (even though the
> caller currently never uses the variables in that case).
> 
> If both variables are initialized to 0 in dio_get_page(), it would
> guarantee that any future callers of dio_get_page() also get to take
> advantage of the initialization. If, however, the variables are only
> initialized in do_direct_IO(), other callers would have to take care of
> this themselves (or the same warnings will pop up there).
> 
> There aren't currently any other callers to dio_get_page(), so the
> choice is probably not a terribly critical one right now, but I wanted
> to at least point this out.

I peeked at this a bit too.

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:

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba388ac..2f024fc16a07 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 */

This at least silences GCC.

But do_direct_IO() and friends are complicated enough that I'll have to
look at them for another day or two (or many, many more) before I'm
confident that this doesn't actually change anything.


Paul Bolle