| Message ID | 20240830014216.3464642-1-yanzhen@vivo.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] sunrpc: Use ERR_CAST() to return | expand |
On Fri, 2024-08-30 at 09:42 +0800, Yan Zhen wrote: > Using ERR_CAST() is more reasonable and safer, When it is necessary > to convert the type of an error pointer and return it. static inline void * __must_check ERR_CAST(__force const void *ptr) { /* cast away the const */ return (void *) ptr; } That function is literally just doing an implicit cast from whatever pointer type it is now, to a 'const void *' and then to a 'void *', which then gets implicitly cast to whatever type the caller is expecting. Exactly how is that "safer" than the current explicit cast? While it is great that ERR_CAST() exists, and I agree that it should be preferred in newer code for the (sole (!)) reason that it documents that we expect this to be an error, I see no reason why it is imperative to apply that change to existing code. Particularly not as a standalone patch. So NACK for now.
From: Trond Myklebust <trondmy@hammerspace.com> Date: Fri, 30 Aug 2024 02:08:09 +0000 > On Fri, 2024-08-30 at 09:42 +0800, Yan Zhen wrote: >> Using ERR_CAST() is more reasonable and safer, When it is necessary >> to convert the type of an error pointer and return it. > > static inline void * __must_check ERR_CAST(__force const void *ptr) > { > /* cast away the const */ > return (void *) ptr; > } > > That function is literally just doing an implicit cast from whatever > pointer type it is now, to a 'const void *' and then to a 'void *', > which then gets implicitly cast to whatever type the caller is > expecting. Exactly how is that "safer" than the current explicit cast? I think we might want to reimplement ERR_CAST() using _Generic() to not cast away const when the argument itself is const. > > While it is great that ERR_CAST() exists, and I agree that it should be > preferred in newer code for the (sole (!)) reason that it documents > that we expect this to be an error, I see no reason why it is > imperative to apply that change to existing code. Particularly not as a > standalone patch. > > So NACK for now. Thanks, Olek
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c index 09f29a95f2bc..8ee87311b348 100644 --- a/net/sunrpc/clnt.c +++ b/net/sunrpc/clnt.c @@ -603,7 +603,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args) xprt = xprt_create_transport(&xprtargs); if (IS_ERR(xprt)) - return (struct rpc_clnt *)xprt; + return ERR_CAST(xprt); /* * By default, kernel RPC client connects from a reserved port.
Using ERR_CAST() is more reasonable and safer, When it is necessary to convert the type of an error pointer and return it. Signed-off-by: Yan Zhen <yanzhen@vivo.com> --- net/sunrpc/clnt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)