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

[Jens Axboe <axboe@kernel.dk>]

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 Axboe