| Message ID | 20181108144458.29012-23-maier@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | zfcp updates for v4.21 | expand |
On 11/8/18 3:44 PM, Steffen Maier wrote: > This was introduced with v4.18 commit 8c3d20aada70 ("scsi: zfcp: fix > missing REC trigger trace for all objects in ERP_FAILED") but would now > suppress helpful -Wswitch compiler warnings when building with W=1 such as > the following forced example: > > drivers/s390/scsi/zfcp_erp.c: In function 'zfcp_erp_handle_failed': > drivers/s390/scsi/zfcp_erp.c:126:2: warning: enumeration value 'ZFCP_ERP_ACTION_REOPEN_PORT_FORCED' not handled in switch [-Wswitch] > switch (want) { > ^~~~~~ > > But then again, only with W=1 we would notice unhandled enum cases. > Without the default cases and a missed unhandled enum case, the code > might perform unforeseen things we might not want... > > As of today, we never run through the removed default case, > so removing it is no functional change. > In the future, we never should run through a default case but > introduce the necessary specific case(s) to handle new functionality. > > Signed-off-by: Steffen Maier <maier@linux.ibm.com> > Reviewed-by: Benjamin Block <bblock@linux.ibm.com> > --- > drivers/s390/scsi/zfcp_erp.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c > index b2845c5b8106..9345fed3bb37 100644 > --- a/drivers/s390/scsi/zfcp_erp.c > +++ b/drivers/s390/scsi/zfcp_erp.c > @@ -151,9 +151,6 @@ static enum zfcp_erp_act_type zfcp_erp_handle_failed( > adapter, ZFCP_STATUS_COMMON_ERP_FAILED); > } > break; > - default: > - need = 0; > - break; > } > > return need; > If you 'should' not run through this code path, doesn't it warrant a WARN_ON() or something? Cheers, Hannes
On 11/16/2018 12:22 PM, Hannes Reinecke wrote: > On 11/8/18 3:44 PM, Steffen Maier wrote: >> would now >> suppress helpful -Wswitch compiler warnings when building with W=1 >> But then again, only with W=1 we would notice unhandled enum cases. that's the only caveat >> Without the default cases and a missed unhandled enum case, the code >> might perform unforeseen things we might not want... this would be a bug that needs fixing >> As of today, we never run through the removed default case, >> so removing it is no functional change. >> In the future, we never should run through a default case but >> introduce the necessary specific case(s) to handle new functionality. that's the fix >> diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c >> index b2845c5b8106..9345fed3bb37 100644 >> --- a/drivers/s390/scsi/zfcp_erp.c >> +++ b/drivers/s390/scsi/zfcp_erp.c >> @@ -151,9 +151,6 @@ static enum zfcp_erp_act_type zfcp_erp_handle_failed( >> adapter, ZFCP_STATUS_COMMON_ERP_FAILED); >> } >> break; >> - default: >> - need = 0; >> - break; >> } >> return need; >> > If you 'should' not run through this code path, doesn't it warrant a > WARN_ON() or something? #include <stdio.h> enum foo { A, B }; int main(int argc, char *argv[]) { enum foo f = argc - 1; switch (f) { case A: printf("A\n"); break; case B: printf("B\n"); break; default: printf("default\n"); break; } return 0; } $ gcc -Wswitch -Wall -g -o Wswitch Wswitch.c Removing case B (while keeping default:) does not warn on build. Removing case B and default: nicely warns on build. Hopefully I haven't missed anything. From above, I conclude: A runtime check would require the introduction of a default case. Adding a default case would trade a static build warning for a runtime WARN_ON(_ONCE) which only appears if one manages to get the code run into the default case that should not happen. I find the static build warning more helpful for future extensions adding more value(s) to the enum. Without a default case, we always getting a build warning for missing switch cases.
diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c index b2845c5b8106..9345fed3bb37 100644 --- a/drivers/s390/scsi/zfcp_erp.c +++ b/drivers/s390/scsi/zfcp_erp.c @@ -151,9 +151,6 @@ static enum zfcp_erp_act_type zfcp_erp_handle_failed( adapter, ZFCP_STATUS_COMMON_ERP_FAILED); } break; - default: - need = 0; - break; } return need;
This was introduced with v4.18 commit 8c3d20aada70 ("scsi: zfcp: fix missing REC trigger trace for all objects in ERP_FAILED") but would now suppress helpful -Wswitch compiler warnings when building with W=1 such as the following forced example: drivers/s390/scsi/zfcp_erp.c: In function 'zfcp_erp_handle_failed': drivers/s390/scsi/zfcp_erp.c:126:2: warning: enumeration value 'ZFCP_ERP_ACTION_REOPEN_PORT_FORCED' not handled in switch [-Wswitch] switch (want) { ^~~~~~ But then again, only with W=1 we would notice unhandled enum cases. Without the default cases and a missed unhandled enum case, the code might perform unforeseen things we might not want... As of today, we never run through the removed default case, so removing it is no functional change. In the future, we never should run through a default case but introduce the necessary specific case(s) to handle new functionality. Signed-off-by: Steffen Maier <maier@linux.ibm.com> Reviewed-by: Benjamin Block <bblock@linux.ibm.com> --- drivers/s390/scsi/zfcp_erp.c | 3 --- 1 file changed, 3 deletions(-)