| Message ID | 20170518133351.1867577-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 3c91ee6964114b1a4f54ae6b397c889576d91cf4 |
| Headers | show |
On Thu, May 18, 2017 at 03:33:29PM +0200, Arnd Bergmann wrote: > A rewrite of btrfs_submit_direct_hook appears to have introduced a warning: > > fs/btrfs/inode.c: In function 'btrfs_submit_direct_hook': > fs/btrfs/inode.c:8467:14: error: 'bio' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > Where the 'bio' variable was previously initialized unconditionally, it > is now set in the "while (submit_len > 0)" loop that would never execute > if submit_len is zero. > > Assuming this cannot happen in practice, we can avoid the warning > by simply replacing the while{} loop with a do{}while() loop so > the compiler knows that it will always be entered at least once. > Thanks for the fix. I think it's a false positve one and I've updated it in v2 with a 'struct bio *bio = NULL' to make compiler happy, could you please help reveiw it? Thanks, -liubo > Fixes: 0fd27e06c61b ("Btrfs: use bio_clone_bioset_partial to simplify DIO submit") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > --- > fs/btrfs/inode.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 8c37b4fa4cbb..c62cf9593cb3 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -8497,7 +8497,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip, > /* bio split */ > ASSERT(map_length <= INT_MAX); > atomic_inc(&dip->pending_bios); > - while (submit_len > 0) { > + do { > clone_len = min_t(int, submit_len, map_length); > > /* > @@ -8540,7 +8540,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip, > start_sector << 9, &map_length, NULL, 0); > if (ret) > goto out_err; > - } > + } while (submit_len > 0); > > submit: > ret = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum, > -- > 2.9.0 >
On Fri, May 19, 2017 at 8:10 PM, Liu Bo <bo.li.liu@oracle.com> wrote: > On Thu, May 18, 2017 at 03:33:29PM +0200, Arnd Bergmann wrote: >> A rewrite of btrfs_submit_direct_hook appears to have introduced a warning: >> >> fs/btrfs/inode.c: In function 'btrfs_submit_direct_hook': >> fs/btrfs/inode.c:8467:14: error: 'bio' may be used uninitialized in this function [-Werror=maybe-uninitialized] >> >> Where the 'bio' variable was previously initialized unconditionally, it >> is now set in the "while (submit_len > 0)" loop that would never execute >> if submit_len is zero. >> >> Assuming this cannot happen in practice, we can avoid the warning >> by simply replacing the while{} loop with a do{}while() loop so >> the compiler knows that it will always be entered at least once. >> > > Thanks for the fix. I think it's a false positve one and I've updated it in v2 > with a 'struct bio *bio = NULL' to make compiler happy, could you please help > reveiw it? Right, it is a false positive and adding the =NULL initialization shuts up the warning. The reason my patch used a different approach is to make the code more robust, see https://rusty.ozlabs.org/?p=232 Generally speaking initializing a local variable to an illegal value, and later using the variable without a check for that original value is error-prone. Even though the code is correct at the moment, someone else might modify it later. My first (broken) solution avoided this by checking for the condition that led to the warning, my newer solution is nicer as it makes it much clearer to the reader what is going on, compared to the NULL initialization that does not help readability but makes it slightly harder to understand why you wrote the code specifically that way. Arnd
On Fri, May 19, 2017 at 09:20:53PM +0200, Arnd Bergmann wrote: > On Fri, May 19, 2017 at 8:10 PM, Liu Bo <bo.li.liu@oracle.com> wrote: > > On Thu, May 18, 2017 at 03:33:29PM +0200, Arnd Bergmann wrote: > >> A rewrite of btrfs_submit_direct_hook appears to have introduced a warning: > >> > >> fs/btrfs/inode.c: In function 'btrfs_submit_direct_hook': > >> fs/btrfs/inode.c:8467:14: error: 'bio' may be used uninitialized in this function [-Werror=maybe-uninitialized] > >> > >> Where the 'bio' variable was previously initialized unconditionally, it > >> is now set in the "while (submit_len > 0)" loop that would never execute > >> if submit_len is zero. > >> > >> Assuming this cannot happen in practice, we can avoid the warning > >> by simply replacing the while{} loop with a do{}while() loop so > >> the compiler knows that it will always be entered at least once. > >> > > > > Thanks for the fix. I think it's a false positve one and I've updated it in v2 > > with a 'struct bio *bio = NULL' to make compiler happy, could you please help > > reveiw it? > > Right, it is a false positive and adding the =NULL initialization shuts up the > warning. The reason my patch used a different approach is to make the > code more robust, see https://rusty.ozlabs.org/?p=232 > > Generally speaking initializing a local variable to an illegal value, and later > using the variable without a check for that original value is error-prone. > Even though the code is correct at the moment, someone else might > modify it later. My first (broken) solution avoided this by checking for > the condition that led to the warning, my newer solution is nicer as it > makes it much clearer to the reader what is going on, compared to > the NULL initialization that does not help readability but makes > it slightly harder to understand why you wrote the code specifically that > way. I like this approach better, so I'll undo "= NULL" and apply your patch. Thanks.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 8c37b4fa4cbb..c62cf9593cb3 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -8497,7 +8497,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip, /* bio split */ ASSERT(map_length <= INT_MAX); atomic_inc(&dip->pending_bios); - while (submit_len > 0) { + do { clone_len = min_t(int, submit_len, map_length); /* @@ -8540,7 +8540,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip, start_sector << 9, &map_length, NULL, 0); if (ret) goto out_err; - } + } while (submit_len > 0); submit: ret = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum,
A rewrite of btrfs_submit_direct_hook appears to have introduced a warning: fs/btrfs/inode.c: In function 'btrfs_submit_direct_hook': fs/btrfs/inode.c:8467:14: error: 'bio' may be used uninitialized in this function [-Werror=maybe-uninitialized] Where the 'bio' variable was previously initialized unconditionally, it is now set in the "while (submit_len > 0)" loop that would never execute if submit_len is zero. Assuming this cannot happen in practice, we can avoid the warning by simply replacing the while{} loop with a do{}while() loop so the compiler knows that it will always be entered at least once. Fixes: 0fd27e06c61b ("Btrfs: use bio_clone_bioset_partial to simplify DIO submit") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- --- fs/btrfs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.9.0