* [patch] Btrfs: fix bitwise vs logical condition
@ 2012-01-20 7:54 Dan Carpenter
2012-01-20 14:24 ` Ilya Dryomov
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2012-01-20 7:54 UTC (permalink / raw)
To: Chris Mason, Ilya Dryomov; +Cc: linux-btrfs, kernel-janitors
The intent here was to do a logical && instead of a bitwise &. The
original condition tests whether they have the some of same bits set.
I have fixed that and rewritten it to be more clear.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Warning: This is a static analysis bug and I'm not very familiar with
the code. Please review carefully.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0b4e2af..0c54027 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2376,8 +2376,8 @@ static int should_balance_chunk(struct btrfs_root *root,
u64 chunk_type = btrfs_chunk_type(leaf, chunk);
/* type filter */
- if (!((chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) &
- (bctl->flags & BTRFS_BALANCE_TYPE_MASK))) {
+ if (!(chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) ||
+ !(bctl->flags & BTRFS_BALANCE_TYPE_MASK)) {
return 0;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [patch] Btrfs: fix bitwise vs logical condition
2012-01-20 7:54 [patch] Btrfs: fix bitwise vs logical condition Dan Carpenter
@ 2012-01-20 14:24 ` Ilya Dryomov
2012-01-20 15:21 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Ilya Dryomov @ 2012-01-20 14:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Chris Mason, linux-btrfs, kernel-janitors
On Fri, Jan 20, 2012 at 10:54:55AM +0300, Dan Carpenter wrote:
> The intent here was to do a logical && instead of a bitwise &. The
> original condition tests whether they have the some of same bits set.
> I have fixed that and rewritten it to be more clear.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Warning: This is a static analysis bug and I'm not very familiar with
> the code. Please review carefully.
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 0b4e2af..0c54027 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2376,8 +2376,8 @@ static int should_balance_chunk(struct btrfs_root *root,
> u64 chunk_type = btrfs_chunk_type(leaf, chunk);
>
> /* type filter */
> - if (!((chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) &
> - (bctl->flags & BTRFS_BALANCE_TYPE_MASK))) {
> + if (!(chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) ||
> + !(bctl->flags & BTRFS_BALANCE_TYPE_MASK)) {
> return 0;
> }
>
The intent here is definitely bitwise &. The original code tests
whether at least one of the bits set in (chunk_type & MASK) is set in
(bctl_flags & MASK), and if not returns 0. IOW,
u64 mask = chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK;
if (((bctl->flags & BTRFS_BALANCE_TYPE_MASK) & mask) == 0)
return 0;
Your patch does something completely different. However, I think we can
strengthen that check (and make it more idiomatic). Can you try the
below diff with your checker ?
Thanks,
Ilya
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7ffdb15..9c6a074 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2375,12 +2375,11 @@ static int should_balance_chunk(struct btrfs_root *root,
struct btrfs_balance_control *bctl = root->fs_info->balance_ctl;
struct btrfs_balance_args *bargs = NULL;
u64 chunk_type = btrfs_chunk_type(leaf, chunk);
+ u64 mask = chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK;
/* type filter */
- if (!((chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) &
- (bctl->flags & BTRFS_BALANCE_TYPE_MASK))) {
+ if (((bctl->flags & BTRFS_BALANCE_TYPE_MASK) & mask) != mask)
return 0;
- }
if (chunk_type & BTRFS_BLOCK_GROUP_DATA)
bargs = &bctl->data;
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [patch] Btrfs: fix bitwise vs logical condition
2012-01-20 14:24 ` Ilya Dryomov
@ 2012-01-20 15:21 ` Dan Carpenter
2012-01-20 16:00 ` Ilya Dryomov
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2012-01-20 15:21 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: Chris Mason, linux-btrfs, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]
On Fri, Jan 20, 2012 at 04:24:37PM +0200, Ilya Dryomov wrote:
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2375,12 +2375,11 @@ static int should_balance_chunk(struct btrfs_root *root,
> struct btrfs_balance_control *bctl = root->fs_info->balance_ctl;
> struct btrfs_balance_args *bargs = NULL;
> u64 chunk_type = btrfs_chunk_type(leaf, chunk);
> + u64 mask = chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK;
>
> /* type filter */
> - if (!((chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) &
> - (bctl->flags & BTRFS_BALANCE_TYPE_MASK))) {
> + if (((bctl->flags & BTRFS_BALANCE_TYPE_MASK) & mask) != mask)
I don't know if it matters, but semantically this is not
equivalent to the original. If mask has no flags set then this will
pass. This says that every flag in mask but has to be set in
->flags but in the original code, only one needed to be.
The original code was equivalent to:
if (!(chunk_type & bctl->flags & BTRFS_BLOCK_GROUP_TYPE_MASK &
BTRFS_BALANCE_TYPE_MASK)) {...
It's weird that we have BTRFS_BLOCK_GROUP_TYPE_MASK and
BTRFS_BALANCE_TYPE_MASK which are the same except that the bitfields
have been renamed. Can't we just reuse the first definition?
But really, if this isn't a bug, then I don't care. The original is
fine, or whatever you choose.
regards,
dan carpenter
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch] Btrfs: fix bitwise vs logical condition
2012-01-20 15:21 ` Dan Carpenter
@ 2012-01-20 16:00 ` Ilya Dryomov
0 siblings, 0 replies; 4+ messages in thread
From: Ilya Dryomov @ 2012-01-20 16:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Chris Mason, linux-btrfs, kernel-janitors
On Fri, Jan 20, 2012 at 06:21:29PM +0300, Dan Carpenter wrote:
> On Fri, Jan 20, 2012 at 04:24:37PM +0200, Ilya Dryomov wrote:
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -2375,12 +2375,11 @@ static int should_balance_chunk(struct btrfs_root *root,
> > struct btrfs_balance_control *bctl = root->fs_info->balance_ctl;
> > struct btrfs_balance_args *bargs = NULL;
> > u64 chunk_type = btrfs_chunk_type(leaf, chunk);
> > + u64 mask = chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK;
> >
> > /* type filter */
> > - if (!((chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) &
> > - (bctl->flags & BTRFS_BALANCE_TYPE_MASK))) {
> > + if (((bctl->flags & BTRFS_BALANCE_TYPE_MASK) & mask) != mask)
>
> I don't know if it matters, but semantically this is not
> equivalent to the original. If mask has no flags set then this will
> pass. This says that every flag in mask but has to be set in
> ->flags but in the original code, only one needed to be.
Yeah, that's why I said "we can strengthen that check".
>
> The original code was equivalent to:
> if (!(chunk_type & bctl->flags & BTRFS_BLOCK_GROUP_TYPE_MASK &
> BTRFS_BALANCE_TYPE_MASK)) {...
>
> It's weird that we have BTRFS_BLOCK_GROUP_TYPE_MASK and
> BTRFS_BALANCE_TYPE_MASK which are the same except that the bitfields
> have been renamed. Can't we just reuse the first definition?
>
> But really, if this isn't a bug, then I don't care. The original is
> fine, or whatever you choose.
This is not a bug.
Thanks,
Ilya
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-01-20 16:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-20 7:54 [patch] Btrfs: fix bitwise vs logical condition Dan Carpenter
2012-01-20 14:24 ` Ilya Dryomov
2012-01-20 15:21 ` Dan Carpenter
2012-01-20 16:00 ` Ilya Dryomov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).