Re: [PATCH 1/1] block: add default clause for unsupported T10_PI types

[Max Gurtovoy <maxg@xxxxxxxxxxxx>]

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);