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@xxxxxxxxxxxx>
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@xxxxxxxx>
Signed-off-by: Max Gurtovoy <maxg@xxxxxxxxxxxx>
---
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);
--
1.8.3.1
