| Message ID | 20190314061716.19892-1-sergey.senozhatsky@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] cifs: remove unused status severity defines | expand |
Since this file (smb2status.h) is intended to track the official protocol documentation (albeit smb2status.h probably needs to be updated), in this case the protocol document MS-ERREF. I would prefer to keep it closer to MS-ERREF and leave definitions in even if unused (if nothing else it helps some of us when debugging to recognize what these errors on the wire mean). There is a real danger that we have run into in the past that in removing some protocol definitions (flags, etc.) from the code or forgetting to update our headers to match newer versions of the protocol specifications, that with future code changes we can forget to handle flags (for example) or misparse responses due to not realizing that there are additional flags that need to be parsed. On Thu, Mar 14, 2019 at 1:17 AM Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > > STATUS_SEVERITY_* do not appear to be used by anyone, so > drop them. Besides, the name of __constant_cpu_to_le32() > is misspelled there: __constanst_cpu_to_le32(). > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > --- > fs/cifs/smb2status.h | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/fs/cifs/smb2status.h b/fs/cifs/smb2status.h > index 3d5f62150de4..e9da9c6da277 100644 > --- a/fs/cifs/smb2status.h > +++ b/fs/cifs/smb2status.h > @@ -29,11 +29,6 @@ > * C is set if "customer defined" error, N bit is reserved and MBZ > */ > > -#define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000) > -#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001) > -#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002) > -#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003) > - > struct ntstatus { > /* Facility is the high 12 bits of the following field */ > __le32 Facility; /* low 2 bits Severity, next is Customer, then rsrvd */ > -- > 2.21.0 >
updated it to fix the spelling mistake On Thu, Mar 14, 2019 at 1:54 AM Steve French <smfrench@gmail.com> wrote: > > Since this file (smb2status.h) is intended to track the official > protocol documentation (albeit smb2status.h probably needs to be > updated), in this case the protocol document MS-ERREF. I would prefer > to keep it closer to MS-ERREF and leave definitions in even if unused > (if nothing else it helps some of us when debugging to recognize what > these errors on the wire mean). There is a real danger that we have > run into in the past that in removing some protocol definitions > (flags, etc.) from the code or forgetting to update our headers to > match newer versions of the protocol specifications, that with future > code changes we can forget to handle flags (for example) or misparse > responses due to not realizing that there are additional flags that > need to be parsed. > > On Thu, Mar 14, 2019 at 1:17 AM Sergey Senozhatsky > <sergey.senozhatsky.work@gmail.com> wrote: > > > > STATUS_SEVERITY_* do not appear to be used by anyone, so > > drop them. Besides, the name of __constant_cpu_to_le32() > > is misspelled there: __constanst_cpu_to_le32(). > > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > > --- > > fs/cifs/smb2status.h | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/fs/cifs/smb2status.h b/fs/cifs/smb2status.h > > index 3d5f62150de4..e9da9c6da277 100644 > > --- a/fs/cifs/smb2status.h > > +++ b/fs/cifs/smb2status.h > > @@ -29,11 +29,6 @@ > > * C is set if "customer defined" error, N bit is reserved and MBZ > > */ > > > > -#define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000) > > -#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001) > > -#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002) > > -#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003) > > - > > struct ntstatus { > > /* Facility is the high 12 bits of the following field */ > > __le32 Facility; /* low 2 bits Severity, next is Customer, then rsrvd */ > > -- > > 2.21.0 > > > > > -- > Thanks, > > Steve
On (03/14/19 01:54), Steve French wrote: > Since this file (smb2status.h) is intended to track the official > protocol documentation (albeit smb2status.h probably needs to be > updated), in this case the protocol document MS-ERREF. I would prefer > to keep it closer to MS-ERREF and leave definitions in even if unused > (if nothing else it helps some of us when debugging to recognize what > these errors on the wire mean). There is a real danger that we have > run into in the past that in removing some protocol definitions > (flags, etc.) from the code or forgetting to update our headers to > match newer versions of the protocol specifications, that with future > code changes we can forget to handle flags (for example) or misparse > responses due to not realizing that there are additional flags that > need to be parsed. OK, works for me. -ss
On (03/14/19 02:04), Steve French wrote: [..] > #define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000) Does STATUS_SEVERITY_SUCCESS still use __constant_cpu_to_le32? > -#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001) > -#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002) > -#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003) > +#define STATUS_SEVERITY_INFORMATIONAL cpu_to_le32(0x0001) > +#define STATUS_SEVERITY_WARNING cpu_to_le32(0x0002) > +#define STATUS_SEVERITY_ERROR cpu_to_le32(0x0003) Otherwise looks good. -ss
All of those uses of __constant_cpu_to_le32 apparently (at least according to checkpatch) should be changed (someday) to cpu_to_le32 but I didn't research why the change from __constant_cpu_to_le32 ---> cpu_to_le32 If it has benefit - and checkpatch is right (it warned about __constant_cpu_to_le32 being no longer preferred) ... perhaps would be worth a followup patch to clean the rest of them up? If you have any context on why kernel code has moved away from using the older format of __constant_cpu_to_.... would be useful to know if any benefit to the change On Thu, Mar 14, 2019 at 2:12 AM Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > > On (03/14/19 02:04), Steve French wrote: > [..] > > #define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000) > > Does STATUS_SEVERITY_SUCCESS still use __constant_cpu_to_le32? > > > -#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001) > > -#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002) > > -#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003) > > +#define STATUS_SEVERITY_INFORMATIONAL cpu_to_le32(0x0001) > > +#define STATUS_SEVERITY_WARNING cpu_to_le32(0x0002) > > +#define STATUS_SEVERITY_ERROR cpu_to_le32(0x0003) > > Otherwise looks good. > > -ss
On (03/14/19 02:19), Steve French wrote: > All of those uses of __constant_cpu_to_le32 apparently (at least > according to checkpatch) should be changed (someday) to cpu_to_le32 > but I didn't research why the change from __constant_cpu_to_le32 > ---> cpu_to_le32 Probably historic reasons. Looking at linux 2.4.21 /* * Allow constant folding */ #if defined(__GNUC__) && (__GNUC__ >= 2) && defined(__OPTIMIZE__) # define __swahw32(x) \ (__builtin_constant_p((__u32)(x)) ? \ ___swahw32((x)) : \ __fswahw32((x))) My assumption would be that __GNUC__ < 2 did no support __builtin_constant_p? > If it has benefit - and checkpatch is right (it warned about > __constant_cpu_to_le32 being no longer preferred) ... perhaps would be > worth a followup patch to clean the rest of them up? If you have any > context on why kernel code has moved away from using the older format > of __constant_cpu_to_.... would be useful to know if any benefit to > the change Right, that's what I'm going to do - send out patches and update the rest of __constant_cpu_to_XX users; so, eventually, __constant_cpu_to_XX can be removed. -ss
I am fine with taking a patch to get rid of __constant_cpu_to_XXX (and converting to the same cpu_to_XXX with the "__constant") in fs/cifs (assuming that that is still recommended). On Thu, Mar 14, 2019 at 2:39 AM Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > > On (03/14/19 02:19), Steve French wrote: > > All of those uses of __constant_cpu_to_le32 apparently (at least > > according to checkpatch) should be changed (someday) to cpu_to_le32 > > but I didn't research why the change from __constant_cpu_to_le32 > > ---> cpu_to_le32 > > Probably historic reasons. > > Looking at linux 2.4.21 > > /* > * Allow constant folding > */ > #if defined(__GNUC__) && (__GNUC__ >= 2) && defined(__OPTIMIZE__) > # define __swahw32(x) \ > (__builtin_constant_p((__u32)(x)) ? \ > ___swahw32((x)) : \ > __fswahw32((x))) > > > My assumption would be that __GNUC__ < 2 did no support > __builtin_constant_p? > > > > If it has benefit - and checkpatch is right (it warned about > > __constant_cpu_to_le32 being no longer preferred) ... perhaps would be > > worth a followup patch to clean the rest of them up? If you have any > > context on why kernel code has moved away from using the older format > > of __constant_cpu_to_.... would be useful to know if any benefit to > > the change > > Right, that's what I'm going to do - send out patches and update the rest > of __constant_cpu_to_XX users; so, eventually, __constant_cpu_to_XX > can be removed. > > -ss
From: Sergey Senozhatsky > > cpu_to_le32() is capable enough to detect __builtin_constant_p() > and to use an appropriate compile time ___constant_swahb32() > function. > > So we can use cpu_to_le32() instead of __constant_cpu_to_le32(). Unless any code tries to use them as case statement labels. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On (03/15/19 12:31), David Laight wrote: > From: Sergey Senozhatsky > > > > cpu_to_le32() is capable enough to detect __builtin_constant_p() > > and to use an appropriate compile time ___constant_swahb32() > > function. > > > > So we can use cpu_to_le32() instead of __constant_cpu_to_le32(). > > Unless any code tries to use them as case statement labels. What is the problem? For compile time constants cpu_to_le32() should do the same thing as __constant_cpu_to_le32(). There seems a whole bunch of cpu_to_XX compile time constants which are used in case statement labels include/linux/sunrpc/xdr.h:#define rpc_msg_accepted cpu_to_be32(RPC_MSG_ACCEPTED) include/linux/sunrpc/xdr.h:#define rpc_success cpu_to_be32(RPC_SUCCESS) include/linux/sunrpc/xdr.h:#define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL) include/linux/sunrpc/xdr.h:#define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH) include/linux/sunrpc/xdr.h:#define rpc_proc_unavail cpu_to_be32(RPC_PROC_UNAVAIL) include/linux/sunrpc/xdr.h:#define rpc_garbage_args cpu_to_be32(RPC_GARBAGE_ARGS) include/linux/sunrpc/xdr.h:#define rpc_system_err cpu_to_be32(RPC_SYSTEM_ERR) include/linux/sunrpc/xdr.h:#define rpc_drop_reply cpu_to_be32(RPC_DROP_REPLY) include/linux/sunrpc/xdr.h:#define rpc_mismatch cpu_to_be32(RPC_MISMATCH) include/linux/sunrpc/xdr.h:#define rpc_auth_error cpu_to_be32(RPC_AUTH_ERROR) include/linux/sunrpc/xdr.h:#define rpc_auth_ok cpu_to_be32(RPC_AUTH_OK) include/linux/sunrpc/xdr.h:#define rpc_autherr_badcred cpu_to_be32(RPC_AUTH_BADCRED) include/linux/sunrpc/xdr.h:#define rpc_autherr_rejectedcred cpu_to_be32(RPC_AUTH_REJECTEDCRED) include/linux/sunrpc/xdr.h:#define rpc_autherr_badverf cpu_to_be32(RPC_AUTH_BADVERF) include/linux/sunrpc/xdr.h:#define rpc_autherr_rejectedverf cpu_to_be32(RPC_AUTH_REJECTEDVERF) include/linux/sunrpc/xdr.h:#define rpc_autherr_tooweak cpu_to_be32(RPC_AUTH_TOOWEAK) net/sunrpc/clnt.c 2479 switch (*p) { 2480 case rpc_success: 2481 return 0; 2482 case rpc_prog_unavail: 2483 trace_rpc__prog_unavail(task); 2484 error = -EPFNOSUPPORT; 2485 goto out_err; 2486 case rpc_prog_mismatch: 2487 trace_rpc__prog_mismatch(task); 2488 error = -EPROTONOSUPPORT; 2489 goto out_err; 2490 case rpc_proc_unavail: 2491 trace_rpc__proc_unavail(task); 2492 error = -EOPNOTSUPP; 2493 goto out_err; 2494 case rpc_garbage_args: 2495 trace_rpc__garbage_args(task); 2496 break; 2497 default: 2498 trace_rpc__unparsable(task); 2499 } -ss
diff --git a/fs/cifs/smb2status.h b/fs/cifs/smb2status.h index 3d5f62150de4..e9da9c6da277 100644 --- a/fs/cifs/smb2status.h +++ b/fs/cifs/smb2status.h @@ -29,11 +29,6 @@ * C is set if "customer defined" error, N bit is reserved and MBZ */ -#define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000) -#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001) -#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002) -#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003) - struct ntstatus { /* Facility is the high 12 bits of the following field */ __le32 Facility; /* low 2 bits Severity, next is Customer, then rsrvd */
STATUS_SEVERITY_* do not appear to be used by anyone, so drop them. Besides, the name of __constant_cpu_to_le32() is misspelled there: __constanst_cpu_to_le32(). Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> --- fs/cifs/smb2status.h | 5 ----- 1 file changed, 5 deletions(-)