| Message ID | 1569103249-24018-1-git-send-email-maxg@mellanox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] block: add default clause for unsupported T10_PI types | expand |
On 9/21/19 4:00 PM, Max Gurtovoy wrote: > kbuild robot reported the following warning: > > block/t10-pi.c: In function 't10_pi_verify': > block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION' > not handled in switch [-Wswitch] > switch (type) { > ^~~~~~ This commit message is woefully lacking. It doesn't explain anything...? Why aren't we just flagging this as an error? Seems a lot saner than adding a BUG(). diff --git a/block/t10-pi.c b/block/t10-pi.c index 0c0120a672f9..6a1d4128a9d4 100644 --- a/block/t10-pi.c +++ b/block/t10-pi.c @@ -79,6 +79,10 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter, pi->ref_tag == T10_PI_REF_ESCAPE) goto next; break; + default: + pr_err("%s: unsupported protection type: %d\n", + iter->disk_name, type); + return BLK_STS_PROTECTION; } csum = fn(iter->data_buf, iter->interval);
Jens, >> block/t10-pi.c: In function 't10_pi_verify': >> block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION' >> not handled in switch [-Wswitch] >> switch (type) { >> ^~~~~~ > > This commit message is woefully lacking. It doesn't explain > anything...? Why aren't we just flagging this as an error? Seems a > lot saner than adding a BUG(). The fundamental issue is that T10_PI_TYPE0_PROTECTION means "no attached protection information". So it's a block layer bug if we ever end up in this function and the protection type is 0. My main beef with all this is that I don't particularly like introducing a nonsensical switch case to quiesce a compiler warning. We never call t10_pi_verify() with a type of 0 and there are lots of safeguards further up the stack preventing that from ever happening. Adding a Type 0 here gives the reader the false impression that it's valid input to the function. Which it really isn't. Arnd: Any ideas how to handle this?
On 9/21/19 4:54 PM, Martin K. Petersen wrote: > > Jens, > >>> block/t10-pi.c: In function 't10_pi_verify': >>> block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION' >>> not handled in switch [-Wswitch] >>> switch (type) { >>> ^~~~~~ >> >> This commit message is woefully lacking. It doesn't explain >> anything...? Why aren't we just flagging this as an error? Seems a >> lot saner than adding a BUG(). > > The fundamental issue is that T10_PI_TYPE0_PROTECTION means "no attached > protection information". So it's a block layer bug if we ever end up in > this function and the protection type is 0. > > My main beef with all this is that I don't particularly like introducing > a nonsensical switch case to quiesce a compiler warning. We never call > t10_pi_verify() with a type of 0 and there are lots of safeguards > further up the stack preventing that from ever happening. Adding a Type > 0 here gives the reader the false impression that it's valid input to > the function. Which it really isn't. > > Arnd: Any ideas how to handle this? Why not just add the default catch, that logs, and returns the error? Would seem like the cleanest way to handle it to me. Since the compiler knows the type, it'll complain if we have missing cases.
On 9/22/2019 2:29 AM, Jens Axboe wrote: > On 9/21/19 4:54 PM, Martin K. Petersen wrote: >> Jens, >> >>>> block/t10-pi.c: In function 't10_pi_verify': >>>> block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION' >>>> not handled in switch [-Wswitch] >>>> switch (type) { >>>> ^~~~~~ >>> This commit message is woefully lacking. It doesn't explain >>> anything...? Why aren't we just flagging this as an error? Seems a >>> lot saner than adding a BUG(). >> The fundamental issue is that T10_PI_TYPE0_PROTECTION means "no attached >> protection information". So it's a block layer bug if we ever end up in >> this function and the protection type is 0. >> >> My main beef with all this is that I don't particularly like introducing >> a nonsensical switch case to quiesce a compiler warning. We never call >> t10_pi_verify() with a type of 0 and there are lots of safeguards >> further up the stack preventing that from ever happening. Adding a Type >> 0 here gives the reader the false impression that it's valid input to >> the function. Which it really isn't. >> >> Arnd: Any ideas how to handle this? > Why not just add the default catch, that logs, and returns the error? > Would seem like the cleanest way to handle it to me. Since the > compiler knows the type, it'll complain if we have missing cases. what about removing the switch/case and do the following change: diff --git a/block/t10-pi.c b/block/t10-pi.c index 0c0120a..9803c7e 100644 --- a/block/t10-pi.c +++ b/block/t10-pi.c @@ -55,13 +55,14 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter, { unsigned int i; + BUG_ON(type == T10_PI_TYPE0_PROTECTION); + for (i = 0 ; i < iter->data_size ; i += iter->interval) { struct t10_pi_tuple *pi = iter->prot_buf; __be16 csum; - switch (type) { - case T10_PI_TYPE1_PROTECTION: - case T10_PI_TYPE2_PROTECTION: + if (type == T10_PI_TYPE1_PROTECTION || + type == T10_PI_TYPE2_PROTECTION) { if (pi->app_tag == T10_PI_APP_ESCAPE) goto next; @@ -73,12 +74,10 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter, iter->seed, be32_to_cpu(pi->ref_tag)); return BLK_STS_PROTECTION; } - break; - case T10_PI_TYPE3_PROTECTION: + } else if (type == T10_PI_TYPE3_PROTECTION) { if (pi->app_tag == T10_PI_APP_ESCAPE && pi->ref_tag == T10_PI_REF_ESCAPE) goto next; - break; } csum = fn(iter->data_buf, iter->interval);
On 9/22/19 3:38 AM, Max Gurtovoy wrote: > > On 9/22/2019 2:29 AM, Jens Axboe wrote: >> On 9/21/19 4:54 PM, Martin K. Petersen wrote: >>> Jens, >>> >>>>> block/t10-pi.c: In function 't10_pi_verify': >>>>> block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION' >>>>> not handled in switch [-Wswitch] >>>>> switch (type) { >>>>> ^~~~~~ >>>> This commit message is woefully lacking. It doesn't explain >>>> anything...? Why aren't we just flagging this as an error? Seems a >>>> lot saner than adding a BUG(). >>> The fundamental issue is that T10_PI_TYPE0_PROTECTION means "no attached >>> protection information". So it's a block layer bug if we ever end up in >>> this function and the protection type is 0. >>> >>> My main beef with all this is that I don't particularly like introducing >>> a nonsensical switch case to quiesce a compiler warning. We never call >>> t10_pi_verify() with a type of 0 and there are lots of safeguards >>> further up the stack preventing that from ever happening. Adding a Type >>> 0 here gives the reader the false impression that it's valid input to >>> the function. Which it really isn't. >>> >>> Arnd: Any ideas how to handle this? >> Why not just add the default catch, that logs, and returns the error? >> Would seem like the cleanest way to handle it to me. Since the >> compiler knows the type, it'll complain if we have missing cases. > > what about removing the switch/case and do the following change: It's effectively the same thing, I really don't think we need (or should have) a BUG/BUG_ON for this condition. Just return an error? Just include a T10_PI_TYPE0_PROTECTION case in the switch, have it log and return an error. Add a comment on how it's impossible, if need be. I don't think it has to be more complicated than that.
Jens, > It's effectively the same thing, I really don't think we need (or should > have) a BUG/BUG_ON for this condition. Just return an error? > Just include a T10_PI_TYPE0_PROTECTION case in the switch, have it log > and return an error. Add a comment on how it's impossible, if need be. > I don't think it has to be more complicated than that. The additional case statement is inside an iterator loop which would bomb for Type 0 since there is no protection buffer to iterate over. We'd presumably never reach that default: case before dereferencing something bad. t10_pi_verify() is a static function exclusively called by helpers that pass in either 1 or 3 as argument. It seems kind of silly that we have to jump through hoops to silence a compiler warning for this. I would prefer a BUILD_BUG_ON(type == T10_PI_TYPE0_PROTECTION) at the top of the function but that does not satisfy the -Wswitch logic either. Anyway. Enough energy wasted on this. I'm OK with either the default: case or Max' if statement approach. My objection is purely wrt. introducing semantically incorrect and/or unreachable code to silence compiler warnings. Seems backwards.
On 9/22/2019 8:31 PM, Martin K. Petersen wrote: > Jens, > >> It's effectively the same thing, I really don't think we need (or should >> have) a BUG/BUG_ON for this condition. Just return an error? >> Just include a T10_PI_TYPE0_PROTECTION case in the switch, have it log >> and return an error. Add a comment on how it's impossible, if need be. >> I don't think it has to be more complicated than that. > The additional case statement is inside an iterator loop which would > bomb for Type 0 since there is no protection buffer to iterate > over. We'd presumably never reach that default: case before > dereferencing something bad. > > t10_pi_verify() is a static function exclusively called by helpers that > pass in either 1 or 3 as argument. It seems kind of silly that we have > to jump through hoops to silence a compiler warning for this. I would > prefer a BUILD_BUG_ON(type == T10_PI_TYPE0_PROTECTION) at the top of the > function but that does not satisfy the -Wswitch logic either. > > Anyway. Enough energy wasted on this. I'm OK with either the default: > case or Max' if statement approach. My objection is purely > wrt. introducing semantically incorrect and/or unreachable code to > silence compiler warnings. Seems backwards. I agree that enough energy wasted here :) Attached some proposal to fix this warning. Let me know if you want me to send it to the mailing list From 058b2e2da4ada6d27287533a7228abd80de17248 Mon Sep 17 00:00:00 2001 From: Max Gurtovoy <maxg@mellanox.com> Date: Sun, 22 Sep 2019 12:46:55 +0300 Subject: [PATCH 1/1] block: t10-pi: fix -Wswitch warning Changing the switch() statement to symbolic constants made the compiler (at least clang-9, did not check gcc) notice that there is one enum value that is not handled here: block/t10-pi.c:62:11: error: enumeration value 'T10_PI_TYPE0_PROTECTION' not handled in switch [-Werror,-Wswitch] Add a BUG_ON statement if we ever get to t10_pi_verify function with TYPE0 and replace the switch() statement with if/else clause for the valid types. Fixes: 9b2061b1a262 ("block: use symbolic constants for t10_pi type") Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Max Gurtovoy <maxg@mellanox.com> --- block/t10-pi.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/block/t10-pi.c b/block/t10-pi.c index 0c0120a..9803c7e 100644 --- a/block/t10-pi.c +++ b/block/t10-pi.c @@ -55,13 +55,14 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter, { unsigned int i; + BUG_ON(type == T10_PI_TYPE0_PROTECTION); + for (i = 0 ; i < iter->data_size ; i += iter->interval) { struct t10_pi_tuple *pi = iter->prot_buf; __be16 csum; - switch (type) { - case T10_PI_TYPE1_PROTECTION: - case T10_PI_TYPE2_PROTECTION: + if (type == T10_PI_TYPE1_PROTECTION || + type == T10_PI_TYPE2_PROTECTION) { if (pi->app_tag == T10_PI_APP_ESCAPE) goto next; @@ -73,12 +74,10 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter, iter->seed, be32_to_cpu(pi->ref_tag)); return BLK_STS_PROTECTION; } - break; - case T10_PI_TYPE3_PROTECTION: + } else if (type == T10_PI_TYPE3_PROTECTION) { if (pi->app_tag == T10_PI_APP_ESCAPE && pi->ref_tag == T10_PI_REF_ESCAPE) goto next; - break; } csum = fn(iter->data_buf, iter->interval);
On 9/22/19 3:21 PM, Max Gurtovoy wrote: > > On 9/22/2019 8:31 PM, Martin K. Petersen wrote: >> Jens, >> >>> It's effectively the same thing, I really don't think we need (or should >>> have) a BUG/BUG_ON for this condition. Just return an error? >>> Just include a T10_PI_TYPE0_PROTECTION case in the switch, have it log >>> and return an error. Add a comment on how it's impossible, if need be. >>> I don't think it has to be more complicated than that. >> The additional case statement is inside an iterator loop which would >> bomb for Type 0 since there is no protection buffer to iterate >> over. We'd presumably never reach that default: case before >> dereferencing something bad. >> >> t10_pi_verify() is a static function exclusively called by helpers that >> pass in either 1 or 3 as argument. It seems kind of silly that we have >> to jump through hoops to silence a compiler warning for this. I would >> prefer a BUILD_BUG_ON(type == T10_PI_TYPE0_PROTECTION) at the top of the >> function but that does not satisfy the -Wswitch logic either. >> >> Anyway. Enough energy wasted on this. I'm OK with either the default: >> case or Max' if statement approach. My objection is purely >> wrt. introducing semantically incorrect and/or unreachable code to >> silence compiler warnings. Seems backwards. > > I agree that enough energy wasted here :) Agree ;-) > Attached some proposal to fix this warning. > > Let me know if you want me to send it to the mailing list OK, fine with me, I've queued it up.
diff --git a/block/t10-pi.c b/block/t10-pi.c index 0c0120a..57f304a 100644 --- a/block/t10-pi.c +++ b/block/t10-pi.c @@ -79,6 +79,9 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter, pi->ref_tag == T10_PI_REF_ESCAPE) goto next; break; + default: + /* Currently we support only TYPE1/2/3 */ + BUG(); } csum = fn(iter->data_buf, iter->interval);
kbuild robot reported the following warning: block/t10-pi.c: In function 't10_pi_verify': block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION' not handled in switch [-Wswitch] switch (type) { ^~~~~~ Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Max Gurtovoy <maxg@mellanox.com> --- block/t10-pi.c | 3 +++ 1 file changed, 3 insertions(+)